#71/feat/analytics-dashboard-for-token-usage#73
#71/feat/analytics-dashboard-for-token-usage#73nitishbhosale7 wants to merge 4 commits intoskyflo-ai:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughIntroduces a complete analytics dashboard feature with a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser as Browser / UI
participant Analytics as Analytics Page
participant API as Backend API
participant DB as Database
User->>Browser: Navigate to /analytics
Browser->>Analytics: Load Analytics component
Analytics->>Analytics: Initialize with default<br/>(last_n_days=30)
Analytics->>API: GET /analytics/metrics?last_n_days=30
API->>DB: Query Messages filtered by<br/>timestamp within period
DB-->>API: Return Message records
API->>API: Aggregate metrics:<br/>• Traverse messages & conversations<br/>• Sum tokens, costs, latencies<br/>• Track approval/denial statuses<br/>• Build daily breakdown<br/>• Compute averages & rates
API-->>Analytics: MetricsAggregation response<br/>(period bounds, totals,<br/>averages, daily data)
Analytics->>Analytics: Update state & render:<br/>• MetricCards (totals, rates)<br/>• Charts (usage, cost, latency)<br/>• Daily breakdown grid
Analytics-->>Browser: Display populated dashboard
Browser-->>User: Show charts & metrics
User->>Analytics: Change time range<br/>(7D / 30D / 90D / Custom)
Analytics->>API: GET /analytics/metrics<br/>with updated parameters
API->>DB: Query with new period filter
DB-->>API: Return filtered messages
API-->>Analytics: New MetricsAggregation
Analytics-->>Browser: Re-render with new data<br/>(animated transitions)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
ui/src/lib/api.ts (1)
82-101: Consider adding a return type interface for type safety.The function returns
Promise<any>, which reduces type safety. Consider defining an interface for the metrics response structure (matching the backend'sMetricsAggregationmodel) to enable compile-time type checking and better IDE support.For example, add a type definition:
interface MetricsAggregation { period_start: string; period_end: string; total_cost: number; total_tokens: number; // ... other fields from backend model } export const getMetrics = async (lastNDays: number = 30): Promise<MetricsAggregation> => { // ... implementation };ui/src/components/analytics/CostTrendChart.tsx (1)
14-17: Consider consolidating duplicate DailyMetrics type definitions.Multiple chart components (CostTrendChart, LatencyChart, TokenUsageChart) each define their own
DailyMetricsinterface. Consider creating a shared type definition that matches the backend model to reduce duplication and improve maintainability.For example, create a shared types file:
// ui/src/types/analytics.ts export interface DailyMetrics { date: string; cost: number; prompt_tokens: number; completion_tokens: number; cached_tokens: number; total_tokens: number; avg_ttft_ms: number | null; avg_ttr_ms: number | null; conversation_count: number; }Then import it in each chart component.
ui/src/components/analytics/CacheEfficiencyGauge.tsx (1)
10-15: Consider adding input validation and precision handling.The component doesn't validate that
hitRateis within the expected 0.0-1.0 range or handle invalid values (NaN, undefined). Additionally,Math.roundtruncates precision which may not be ideal for cache metrics.Consider:
export default function CacheEfficiencyGauge({ hitRate }: CacheEfficiencyGaugeProps) { - const percentage = Math.round(hitRate * 100); + const safeHitRate = Math.max(0, Math.min(1, hitRate || 0)); + const percentage = Math.round(safeHitRate * 100); const data = [ { name: "Hit", value: percentage }, { name: "Miss", value: 100 - percentage }, ];ui/src/components/analytics/Analytics.tsx (2)
13-40: Consider extracting shared types to avoid duplication.The
DailyMetricsandMetricsAggregationinterfaces duplicate the backend models defined inengine/src/api/models/conversation.py. Consider generating or sharing these types to ensure they stay in sync as the API evolves.
66-80: Consider enhancing loading and empty states.The loading state is functional but could benefit from a skeleton loader for better UX. Additionally, when
erroris null butdatais also null (line 74-77), the message "No data available" is shown—this is acceptable but you may want to distinguish between "no data exists" vs "failed to load."ui/src/components/analytics/TimeRangeSelector.tsx (1)
17-31: Clean implementation; consider accessibility improvements.The component is well-structured and purely presentational. For better accessibility, consider adding
aria-pressedto indicate the selected state and wrapping buttons in arole="group"witharia-label.return ( - <div className="flex bg-dark-card border border-white/10 rounded-lg p-1"> + <div className="flex bg-dark-card border border-white/10 rounded-lg p-1" role="group" aria-label="Time range selector"> {ranges.map((range) => ( <button key={range.value} onClick={() => onRangeChange(range.value)} + aria-pressed={selectedRange === range.value} className={`px-4 py-1.5 rounded-md text-sm font-medium transition-all duration-200 ${selectedRange === range.valueui/src/components/analytics/MetricCard.tsx (1)
33-40: Consider handling zero trend as neutral.When
trendis exactly0, it displays as green "↑ 0%" which may be misleading. Consider showing a neutral indicator (e.g., "—" or gray color) for zero change.{trend !== null && trend !== undefined && ( <span - className={`text-sm mb-1 font-medium ${trend >= 0 ? "text-green-500" : "text-red-500" - }`} + className={`text-sm mb-1 font-medium ${ + trend > 0 ? "text-green-500" : trend < 0 ? "text-red-500" : "text-text-secondary" + }`} > - {trend >= 0 ? "↑" : "↓"} {Math.abs(trend)}% + {trend > 0 ? "↑" : trend < 0 ? "↓" : "—"} {Math.abs(trend)}% </span> )}engine/src/api/endpoints/analytics.py (3)
38-41: Consider extracting Depends to module-level for clarity.The static analysis flags B008 about
Depends()in argument defaults. While this is a common FastAPI pattern and works correctly, extracting the dependency to a module-level variable can improve readability and aligns with the linter's recommendation.+# Module-level dependency for current user +current_user_dependency = Depends(fastapi_users.current_user(optional=True)) + @router.get( "/analytics/metrics", dependencies=[rate_limit_dependency], response_model=MetricsAggregation, ) async def get_metrics( last_n_days: int = Query(default=30, ge=1, le=365), - user=Depends(fastapi_users.current_user(optional=True)), + user=current_user_dependency, ) -> MetricsAggregation:
136-160: Sparse daily breakdown may cause chart gaps.The current implementation only returns days that have data. As noted in the comment, this may cause gaps or discontinuities in frontend charts. Consider filling in zero-value entries for all days in the range for smoother chart rendering.
+ # Fill all days in range with zero values for smooth charts + all_dates = [start_date + timedelta(days=i) for i in range(last_n_days + 1)] + for d in all_dates: + if d not in daily_map: + daily_map[d] = { + "cost": 0.0, "prompt_tokens": 0, "completion_tokens": 0, + "cached_tokens": 0, "total_tokens": 0, "ttft_ms_sum": 0, + "ttft_count": 0, "ttr_ms_sum": 0, "ttr_count": 0, "conversation_count": 0, + } + sorted_dates = sorted(daily_map.keys())
187-188: Period-over-period comparison not implemented.The
cost_change_pctandtokens_change_pctfields are placeholders returningNone. The acceptance criteria mention period-over-period trends. Consider implementing this by comparing with the previous period's totals.Would you like me to help implement the period-over-period comparison logic, or should this be tracked as a follow-up issue?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
engine/src/api/endpoints/analytics.py(1 hunks)engine/src/api/models/conversation.py(2 hunks)ui/design_system.md(1 hunks)ui/src/app/analytics/page.tsx(1 hunks)ui/src/components/analytics/Analytics.tsx(1 hunks)ui/src/components/analytics/CacheEfficiencyGauge.tsx(1 hunks)ui/src/components/analytics/CostTrendChart.tsx(1 hunks)ui/src/components/analytics/LatencyChart.tsx(1 hunks)ui/src/components/analytics/MetricCard.tsx(1 hunks)ui/src/components/analytics/TimeRangeSelector.tsx(1 hunks)ui/src/components/analytics/TokenUsageChart.tsx(1 hunks)ui/src/components/navbar/Navbar.tsx(3 hunks)ui/src/lib/api.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
ui/src/app/analytics/page.tsx (2)
ui/src/components/navbar/Navbar.tsx (1)
Navbar(20-101)ui/src/components/analytics/Analytics.tsx (1)
Analytics(42-160)
ui/src/components/analytics/LatencyChart.tsx (2)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-139)ui/src/components/ui/tooltip.tsx (1)
Tooltip(42-42)
ui/src/components/analytics/Analytics.tsx (8)
engine/src/api/models/conversation.py (2)
DailyMetrics(130-139)MetricsAggregation(141-162)ui/src/lib/api.ts (1)
getMetrics(82-101)ui/src/components/analytics/TimeRangeSelector.tsx (1)
TimeRangeSelector(10-33)ui/src/components/analytics/MetricCard.tsx (1)
MetricCard(14-44)ui/src/components/analytics/TokenUsageChart.tsx (1)
TokenUsageChart(25-103)ui/src/components/analytics/CacheEfficiencyGauge.tsx (1)
CacheEfficiencyGauge(10-55)ui/src/components/analytics/CostTrendChart.tsx (1)
CostTrendChart(23-70)ui/src/components/analytics/LatencyChart.tsx (1)
LatencyChart(25-90)
ui/src/components/analytics/TokenUsageChart.tsx (2)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-139)ui/src/components/ui/tooltip.tsx (1)
Tooltip(42-42)
ui/src/components/analytics/CostTrendChart.tsx (1)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-139)
engine/src/api/endpoints/analytics.py (1)
engine/src/api/models/conversation.py (3)
Conversation(12-33)MetricsAggregation(141-162)DailyMetrics(130-139)
🪛 Ruff (0.14.8)
engine/src/api/endpoints/analytics.py
40-40: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
40-40: Do not perform function call fastapi_users.current_user in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
195-198: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (11)
ui/src/app/analytics/page.tsx (1)
1-15: LGTM!The analytics page component is well-structured, follows Next.js conventions, and correctly composes the Navbar and Analytics components with appropriate styling.
ui/src/components/navbar/Navbar.tsx (1)
70-75: LGTM!The Analytics navigation item is properly integrated with the correct icon, tooltip, routing, and active state handling. The implementation is consistent with other navigation items.
ui/src/components/analytics/CostTrendChart.tsx (1)
23-70: LGTM!The chart implementation is well-configured with appropriate formatting, styling, and animation. The currency formatting to 4 decimal places handles small costs appropriately.
ui/src/components/analytics/CacheEfficiencyGauge.tsx (1)
19-53: LGTM with a minor note.The gauge implementation is well-designed with appropriate styling and animation. The absolute positioning of the caption (line 50) should work fine, though you may want to verify it doesn't overlap on very small screen sizes.
ui/src/components/analytics/LatencyChart.tsx (1)
33-89: LGTM!The dual-line chart implementation effectively visualizes TTFT and TTR metrics with clear differentiation through colors and legend. The styling and formatting are appropriate.
ui/design_system.md (1)
1-82: LGTM!The design system documentation is comprehensive and well-organized, providing clear guidance on colors, typography, spacing, effects, and animations. This will help maintain consistency across the analytics dashboard and broader UI.
ui/src/components/analytics/TokenUsageChart.tsx (1)
25-103: LGTM!The stacked area chart effectively visualizes token usage breakdown with appropriate gradients, stacking, and formatting. The k-unit formatting on the Y-axis (line 61) handles large token counts well.
ui/src/components/analytics/Analytics.tsx (1)
42-64: Data fetching and state management look good.The useEffect correctly re-fetches when
timeRangechanges, properly manages loading/error states, and clears errors before each fetch. The pattern is clean and handles errors appropriately.ui/src/components/analytics/MetricCard.tsx (1)
14-43: Well-structured reusable component.The MetricCard component is clean, uses Framer Motion variants correctly for parent-child animation coordination, and handles optional props gracefully.
engine/src/api/models/conversation.py (1)
130-162: Well-defined analytics data models.The
DailyMetricsandMetricsAggregationmodels are well-structured with appropriate field types. The comment oncache_hit_rateaccurately documents the calculation formula. Good use ofOptionalfor fields that may not have data.engine/src/api/endpoints/analytics.py (1)
55-57: Query filters conversations correctly.The query correctly filters by user and
created_at__gtefor the period. However, note that conversations created beforestart_datebut updated within the period won't be included. Verify this is the intended behavior for your use case.
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
ui/src/components/analytics/Analytics.tsx (1)
154-181: Period-over-period trends are still not displayed.This is the same issue flagged in the previous review. The
trendprop on all MetricCards remains hardcoded tonull, so period-over-period comparisons (cost_change_pctandtokens_change_pct) are not shown even though the backend returns these fields.After adding the missing fields to the
MetricsAggregationinterface, wire them to the appropriate MetricCards.🔎 Proposed fix
<MetricCard title="Total Cost" value={`$${displayData.total_cost.toFixed(4)}`} - trend={null} + trend={displayData.cost_change_pct} icon={<MdOutlineAttachMoney />} color="text-green-400" /> <MetricCard title="Total Tokens" value={displayData.total_tokens.toLocaleString()} - trend={null} + trend={displayData.tokens_change_pct} icon={<MdOutlineSwapVert />} color="text-blue-400" />engine/src/api/endpoints/analytics.py (1)
234-241: Use exception chaining for better debugging.This is the same issue flagged in the previous review. Per static analysis hint B904, use
raise ... from eto preserve the exception chain.🔎 Proposed fix
except HTTPException: raise except Exception as e: logger.exception(f"Error getting metrics: {str(e)}") raise HTTPException( status_code=500, detail=f"Error getting metrics: {str(e)}", - ) + ) from e
🧹 Nitpick comments (9)
ui/src/components/analytics/CacheEfficiencyGauge.tsx (1)
19-54: Consider adding accessibility attributes.The chart lacks accessibility attributes that would help screen reader users understand the visualization. Consider adding ARIA labels and a text alternative.
🔎 Suggested improvements
- return ( - <div className="h-[300px] w-full flex flex-col items-center justify-center relative"> + return ( + <div + className="h-[300px] w-full flex flex-col items-center justify-center relative" + role="img" + aria-label={`Cache efficiency gauge showing ${percentage}% hit rate`} + > <ResponsiveContainer width="100%" height="100%"> - <PieChart> + <PieChart aria-hidden="true">ui/src/components/analytics/DatePickerWithRange.tsx (2)
52-76: Add keyboard interaction support for better accessibility.The popover lacks escape key handling and focus management. Users should be able to press Escape to close the picker and have focus returned to the trigger button.
🔎 Suggested improvements
export function DatePickerWithRange({ className, date, setDate, }: DatePickerWithRangeProps) { const [isOpen, setIsOpen] = React.useState(false) + const buttonRef = React.useRef<HTMLButtonElement>(null) + + React.useEffect(() => { + const handleEscape = (e: KeyboardEvent) => { + if (e.key === 'Escape' && isOpen) { + setIsOpen(false) + buttonRef.current?.focus() + } + } + document.addEventListener('keydown', handleEscape) + return () => document.removeEventListener('keydown', handleEscape) + }, [isOpen]) return ( <div className={cn("grid gap-2 relative", className)}> <Button id="date" + ref={buttonRef} variant={"outline"}Also add
role="dialog"andaria-modal="true"to the popover container:- <div className="absolute top-12 right-0 z-50 w-auto p-0 bg-[#0A1525]/50 border-[#1E2D45] rounded-xl border border-[#243147]/60 backdrop-blur-md shadow-lg shadow-blue-900/10"> + <div + role="dialog" + aria-modal="true" + className="absolute top-12 right-0 z-50 w-auto p-0 bg-[#0A1525]/50 border-[#1E2D45] rounded-xl border border-[#243147]/60 backdrop-blur-md shadow-lg shadow-blue-900/10" + >
58-74: Consider responsive positioning for mobile devices.The calendar popover is positioned with
right-0, which may cause it to overflow off-screen on smaller devices or when the button is near the right edge of the viewport.💡 Suggestion
Consider using a responsive positioning strategy that adjusts based on available space:
- <div className="absolute top-12 right-0 z-50 w-auto p-0 bg-[#0A1525]/50 border-[#1E2D45] rounded-xl border border-[#243147]/60 backdrop-blur-md shadow-lg shadow-blue-900/10"> + <div className="absolute top-12 right-0 max-sm:right-auto max-sm:left-0 z-50 w-auto p-0 bg-[#0A1525]/50 border-[#1E2D45] rounded-xl border border-[#243147]/60 backdrop-blur-md shadow-lg shadow-blue-900/10">Or consider using a portal with a positioning library like Floating UI for more robust placement.
ui/src/components/analytics/TimeRangeSelector.tsx (1)
17-42: Consider adding ARIA attributes for better accessibility.While the component uses semantic button elements, it could benefit from ARIA attributes to indicate the currently selected range to screen reader users.
💡 Suggested enhancement
<Button key={range.value} variant={selectedRange === range.value ? "default" : "ghost"} + aria-pressed={selectedRange === range.value} + aria-label={`Select ${range.label} time range`} onClick={() => onRangeChange(range.value)} className={`h-8 px-4 text-sm font-medium transition-all duration-200 ${selectedRange === range.value ? "shadow-lg" : "text-text-secondary hover:text-white hover:bg-white/5" }`} >ui/src/components/ui/calendar.tsx (1)
175-211: Consider formatting the long className string for readability.The className string on lines 203-207 is extremely long and difficult to read or maintain. Consider breaking it into multiple lines or extracting common class groups.
💡 Suggested formatting
className={cn( - "data-[selected-single=true]:bg-primary data-[selected-single=true]:text-primary-foreground data-[range-middle=true]:bg-accent data-[range-middle=true]:text-accent-foreground data-[range-start=true]:bg-primary data-[range-start=true]:text-primary-foreground data-[range-end=true]:bg-primary data-[range-end=true]:text-primary-foreground group-data-[focused=true]/day:border-ring group-data-[focused=true]/day:ring-ring/50 flex aspect-square h-auto w-full min-w-[--cell-size] flex-col gap-1 font-normal leading-none data-[range-end=true]:rounded-md data-[range-middle=true]:rounded-none data-[range-start=true]:rounded-md group-data-[focused=true]/day:relative group-data-[focused=true]/day:z-10 group-data-[focused=true]/day:ring-[3px] [&>span]:text-xs [&>span]:opacity-70", + "flex aspect-square h-auto w-full min-w-[--cell-size] flex-col gap-1 font-normal leading-none", + "data-[selected-single=true]:bg-primary data-[selected-single=true]:text-primary-foreground", + "data-[range-start=true]:bg-primary data-[range-start=true]:text-primary-foreground data-[range-start=true]:rounded-md", + "data-[range-middle=true]:bg-accent data-[range-middle=true]:text-accent-foreground data-[range-middle=true]:rounded-none", + "data-[range-end=true]:bg-primary data-[range-end=true]:text-primary-foreground data-[range-end=true]:rounded-md", + "group-data-[focused=true]/day:border-ring group-data-[focused=true]/day:ring-ring/50 group-data-[focused=true]/day:relative group-data-[focused=true]/day:z-10 group-data-[focused=true]/day:ring-[3px]", + "[&>span]:text-xs [&>span]:opacity-70", defaultClassNames.day, className )}ui/src/components/analytics/Analytics.tsx (1)
12-12: Remove unused import.The
Calendarimport on line 12 is not used anywhere in this file. Consider removing it to keep the imports clean.🔎 Proposed fix
-import { Calendar } from "@/components/ui/calendar";engine/src/api/endpoints/analytics.py (3)
36-58: Remove unnecessary initial assignment.The assignment
period_end_dt = datetime.now()on line 36 is immediately overwritten in both branches (lines 49 and 58), making this initial assignment unnecessary.🔎 Proposed fix
- period_end_dt = datetime.now() - if start_date: # use custom range s_date = start_date
106-109: Consider removing redundant date filter.The database query already filters conversations with
created_at__gte=period_start(line 67), so all returned conversations are guaranteed to be within the period. The additional checkif conv_date < loop_start_date: continueappears redundant.🔎 Proposed fix
Remove lines 108-109 and line 73:
- # Update start_date for the loop filter - loop_start_date = period_start.date() - # Aggregation structures daily_map: Dict[date, Dict[str, Any]] = defaultdict(for conv in conversations: conv_date = conv.created_at.date() - if conv_date < loop_start_date: - continue - daily = daily_map[conv_date]
172-196: Consider filling in missing days for better chart rendering.The current implementation returns sparse data (only days with activity). As noted in the comments (lines 174-176), filling in zero values for all days in the period would provide better chart continuity and user experience. Without this, charts may show gaps or appear disconnected for periods with no activity.
Proposed approach
After building the sparse
daily_breakdown, iterate through all days in the range and insert zero-valuedDailyMetricsfor missing dates:# After line 196, before line 198 # Fill in missing dates with zero values all_dates = set() current = period_start.date() end = period_end.date() while current <= end: all_dates.add(current) current += timedelta(days=1) existing_dates = {dm.date for dm in daily_breakdown} missing_dates = all_dates - existing_dates for missing_date in sorted(missing_dates): daily_breakdown.append( DailyMetrics( date=missing_date, cost=0.0, prompt_tokens=0, completion_tokens=0, cached_tokens=0, total_tokens=0, avg_ttft_ms=None, avg_ttr_ms=None, conversation_count=0, ) ) # Sort the complete breakdown daily_breakdown.sort(key=lambda dm: dm.date)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
engine/src/api/endpoints/__init__.pyengine/src/api/endpoints/analytics.pyengine/src/api/models/conversation.pyui/package.jsonui/src/app/analytics/page.tsxui/src/components/analytics/Analytics.tsxui/src/components/analytics/CacheEfficiencyGauge.tsxui/src/components/analytics/DatePickerWithRange.tsxui/src/components/analytics/MetricCard.tsxui/src/components/analytics/TimeRangeSelector.tsxui/src/components/ui/calendar.tsxui/src/lib/api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/components/analytics/MetricCard.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
ui/src/app/analytics/page.tsx (2)
ui/src/components/navbar/Navbar.tsx (1)
Navbar(20-101)ui/src/components/analytics/Analytics.tsx (1)
Analytics(53-215)
ui/src/components/analytics/DatePickerWithRange.tsx (1)
ui/src/components/ui/calendar.tsx (1)
Calendar(213-213)
engine/src/api/endpoints/analytics.py (1)
engine/src/api/models/conversation.py (3)
Conversation(12-33)MetricsAggregation(141-167)DailyMetrics(130-139)
🪛 Ruff (0.14.10)
engine/src/api/endpoints/analytics.py
25-25: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
26-26: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call fastapi_users.current_user in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
238-241: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (14)
engine/src/api/endpoints/__init__.py (1)
9-9: LGTM!The analytics router import follows the established pattern used by other routers in this file.
ui/src/app/analytics/page.tsx (1)
1-15: LGTM!The analytics page component follows Next.js 14 app router conventions and maintains consistency with the application's layout patterns. The flex layout with Navbar and scrollable content area is appropriate for a dashboard.
ui/package.json (3)
33-33: LGTM!Adding lucide-react is appropriate for the new analytics UI components. It's a well-maintained icon library used in the Calendar component.
39-39: LGTM!The react-day-picker addition is required for the custom date range selection feature in the analytics dashboard. It's a well-established library for date picking functionality.
16-16: Version update is compatible—no action needed.The
@radix-ui/react-slotbump from ^1.1.0 to ^1.2.4 is safe. The single usage inbutton.tsxfollows a standard pattern and is unaffected by the minor behavioral changes in the newer version (ref handling optimizations and Fragment ref skipping).ui/src/components/ui/calendar.tsx (2)
14-173: LGTM with observation on complexity.The Calendar component provides comprehensive customization of react-day-picker with extensive styling and component overrides. The implementation correctly merges default classes with custom styling and provides proper component overrides for Root, Chevron, DayButton, and WeekNumber.
The component is quite complex with extensive Tailwind classes, which may impact maintainability. However, this is typical for shadcn/ui-style components that prioritize customization and design system integration.
213-213: LGTM!The exports correctly provide both the Calendar wrapper and CalendarDayButton for consumers who need custom day button rendering.
ui/src/components/analytics/Analytics.tsx (3)
53-59: LGTM!State management is well-structured with appropriate types and initial values.
60-90: LGTM!The data fetching logic correctly handles both standard time ranges and custom date ranges, with appropriate guards for incomplete selections and proper error handling.
92-106: LGTM!Loading and error states are clearly rendered with appropriate styling.
engine/src/api/endpoints/analytics.py (2)
1-17: LGTM!Imports are appropriate, and the previously flagged duplicate
check_conversation_authorizationfunction has been removed.
18-28: LGTM!The endpoint definition follows standard FastAPI patterns. The static analysis warnings (B008) about
QueryandDependsin function defaults are false positives—this is the idiomatic way to declare dependencies and query parameters in FastAPI.engine/src/api/models/conversation.py (2)
130-139: LGTM!The
DailyMetricsmodel is well-structured with appropriate types for each field.
141-167: LGTM!The
MetricsAggregationmodel is comprehensive and well-documented with clear field groupings. It includes all necessary metrics for the analytics dashboard.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
ui/src/components/analytics/Analytics.tsx (3)
32-50: Add missing period-over-period fields to MetricsAggregation interface.The backend's
MetricsAggregationmodel includescost_change_pctandtokens_change_pctfields, but they're missing from this frontend interface. The interface should match the backend contract for type safety.🔎 Proposed fix
interface MetricsAggregation { period_start: string; period_end: string; total_cost: number; total_tokens: number; total_prompt_tokens: number; total_completion_tokens: number; total_cached_tokens: number; total_conversations: number; total_approvals: number; total_rejections: number; avg_ttft_ms: number | null; avg_ttr_ms: number | null; avg_cost_per_conversation: number; avg_tokens_per_conversation: number; cache_hit_rate: number; approval_acceptance_rate: number; daily_breakdown: DailyMetrics[]; + cost_change_pct: number | null; + tokens_change_pct: number | null; }Based on past review feedback.
153-180: Wire period-over-period trends to MetricCards.The backend returns
cost_change_pctandtokens_change_pctinMetricsAggregation, but alltrendprops are hardcoded tonull. Wire these values to display period-over-period comparisons as intended by the acceptance criteria.🔎 Proposed fix
First, add the missing fields to the interface (see previous comment), then:
<MetricCard title="Total Cost" value={`$${displayData.total_cost.toFixed(4)}`} - trend={null} + trend={displayData.cost_change_pct ?? null} icon={<MdOutlineAttachMoney />} color="text-green-400" /> <MetricCard title="Total Tokens" value={displayData.total_tokens.toLocaleString()} - trend={null} + trend={displayData.tokens_change_pct ?? null} icon={<MdOutlineSwapVert />} color="text-blue-400" />Based on past review feedback.
167-173: Fix falsy check for zero approval rate.The condition on line 169 uses a falsy check, which will display "N/A" when the acceptance rate is exactly 0. A 0% acceptance rate is a valid metric and should be displayed as "0%" rather than "N/A".
🔎 Proposed fix
<MetricCard title="Tool Approval Acceptance Rate" - value={displayData.approval_acceptance_rate ? `${Math.round(displayData.approval_acceptance_rate * 100)}%` : "N/A"} + value={displayData.approval_acceptance_rate != null ? `${Math.round(displayData.approval_acceptance_rate * 100)}%` : "N/A"} trend={null} icon={<MdOutlineThumbUpOffAlt />} color="text-yellow-400" />Based on past review feedback.
🧹 Nitpick comments (4)
ui/src/components/analytics/DatePickerWithRange.tsx (4)
5-5: Remove or clarify the inline comment.The comment suggests uncertainty about using
DateRangefromreact-day-picker. This type is correct for compatibility with the Analytics component (which also uses this type). If you're confident in this choice, remove the comment. Otherwise, consider defining your own type ifreact-day-pickeris not otherwise needed.
19-60: Move theme definition outside the component.The
darkThemeobject is recreated on every render, causing unnecessary work. Since the theme is static and doesn't depend on props or state, move it outside the component body.🔎 Proposed fix
+const darkTheme = createTheme({ + palette: { + mode: 'dark', + primary: { + main: '#3b82f6', // blue-500 + }, + background: { + paper: '#0A1525', // card bg + default: '#020817', // bg-background + }, + }, + components: { + MuiTextField: { + styleOverrides: { + root: { + '& .MuiOutlinedInput-root': { + backgroundColor: 'rgba(255, 255, 255, 0.03)', + '& fieldset': { + borderColor: 'rgba(255, 255, 255, 0.1)', + }, + '&:hover fieldset': { + borderColor: 'rgba(255, 255, 255, 0.2)', + }, + '&.Mui-focused fieldset': { + borderColor: '#3b82f6', + }, + }, + '& .MuiInputLabel-root': { + color: 'rgba(255, 255, 255, 0.7)', + }, + '& .MuiInputBase-input': { + color: 'white', + }, + '& .MuiSvgIcon-root': { + color: 'rgba(255, 255, 255, 0.7)', + } + }, + }, + }, + }, +}); + export function DatePickerWithRange({ className, date, setDate, }: DatePickerWithRangeProps) { - const darkTheme = createTheme({ - palette: { - mode: 'dark', - primary: { - main: '#3b82f6', // blue-500 - }, - background: { - paper: '#0A1525', // card bg - default: '#020817', // bg-background - }, - }, - components: { - MuiTextField: { - // ... (keep MuiTextField styles if they were correct, but I'll re-include them to be safe) - styleOverrides: { - root: { - '& .MuiOutlinedInput-root': { - backgroundColor: 'rgba(255, 255, 255, 0.03)', - '& fieldset': { - borderColor: 'rgba(255, 255, 255, 0.1)', - }, - '&:hover fieldset': { - borderColor: 'rgba(255, 255, 255, 0.2)', - }, - '&.Mui-focused fieldset': { - borderColor: '#3b82f6', - }, - }, - '& .MuiInputLabel-root': { - color: 'rgba(255, 255, 255, 0.7)', - }, - '& .MuiInputBase-input': { - color: 'white', - }, - '& .MuiSvgIcon-root': { - color: 'rgba(255, 255, 255, 0.7)', - } - }, - }, - }, - }, - }); return (
71-156: Extract duplicated DatePicker configuration.The two
DatePickercomponents have nearly identicalslotPropsconfigurations (lines 77-112 and 120-155), resulting in ~70 lines of duplication. This violates DRY and makes maintenance harder.🔎 Proposed fix
+const datePickerSlotProps = { + textField: { + size: "small" as const, + sx: { width: 150 } + }, + popper: { + sx: { + '& .MuiPaper-root': { + border: '1px solid rgba(255, 255, 255, 0.1)', + backgroundColor: '#3b82f61a', + color: 'white', + }, + '& .MuiPickersDay-root': { + color: 'white', + '&:hover': { + backgroundColor: 'rgba(255, 255, 255, 0.1)', + }, + '&.Mui-selected': { + backgroundColor: '#3b82f6', + '&:hover': { + backgroundColor: '#2563eb', + }, + }, + }, + '& .MuiPickersCalendarHeader-label': { + color: 'white', + }, + '& .MuiSvgIcon-root': { + color: 'white', + }, + '& .MuiDayCalendar-weekDayLabel': { + color: 'rgba(255, 255, 255, 0.7)', + } + } + } +}; + export function DatePickerWithRange({ className, date, setDate, }: DatePickerWithRangeProps) { return ( <ThemeProvider theme={darkTheme}> <LocalizationProvider dateAdapter={AdapterDateFns}> <div className={cn("grid gap-2 flex-row md:flex", className)}> <DatePicker label="Start Date" value={date?.from || null} onChange={(newValue) => { setDate({ from: newValue || undefined, to: date?.to }); }} - slotProps={{ - textField: { - size: "small", - sx: { width: 150 } - }, - popper: { - sx: { - '& .MuiPaper-root': { - border: '1px solid rgba(255, 255, 255, 0.1)', - backgroundColor: '#3b82f61a', - color: 'white', - }, - // ... rest of styles - } - } - }} + slotProps={datePickerSlotProps} /> <DatePicker label="End Date" value={date?.to || null} onChange={(newValue) => { setDate({ from: date?.from, to: newValue || undefined }); }} - slotProps={{ - textField: { - size: "small", - sx: { width: 150 } - }, - popper: { - sx: { - '& .MuiPaper-root': { - border: '1px solid rgba(255, 255, 255, 0.1)', - backgroundColor: '#3b82f61a', - color: 'white', - }, - // ... rest of styles - } - } - }} + slotProps={datePickerSlotProps} /> </div> </LocalizationProvider> </ThemeProvider> ) }
74-75: Consider adding date range validation.The component allows selecting an end date before the start date without validation or warning. This could lead to invalid date ranges being passed to the API.
Consider adding validation in the
onChangehandlers:onChange={(newValue) => { const newFrom = newValue || undefined; if (newFrom && date?.to && newFrom > date.to) { // Option 1: Clear the end date setDate({ from: newFrom, to: undefined }); // Option 2: Show a warning/error } else { setDate({ from: newFrom, to: date?.to }); } }}Alternatively, use MUI's
minDate/maxDateprops to restrict the selectable range:
- Add
minDate={date?.from}to the End Date picker- Add
maxDate={date?.to}to the Start Date pickerAlso applies to: 117-118
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ui/package-lock.jsonis excluded by!**/package-lock.jsonui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
engine/src/api/endpoints/analytics.pyui/package.jsonui/src/components/analytics/Analytics.tsxui/src/components/analytics/DatePickerWithRange.tsxui/src/components/analytics/MetricCard.tsxui/src/components/analytics/TimeRangeSelector.tsxui/src/components/analytics/TokenUsageChart.tsxui/src/components/ui/calendar.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
ui/src/components/analytics/Analytics.tsx (3)
engine/src/api/models/conversation.py (2)
DailyMetrics(130-139)MetricsAggregation(141-167)ui/src/lib/api.ts (1)
getMetrics(82-115)ui/src/components/analytics/DatePickerWithRange.tsx (1)
DatePickerWithRange(62-161)
ui/src/components/analytics/TokenUsageChart.tsx (1)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-139)
engine/src/api/endpoints/analytics.py (1)
engine/src/api/models/conversation.py (3)
Conversation(12-33)MetricsAggregation(141-167)DailyMetrics(130-139)
🪛 Ruff (0.14.10)
engine/src/api/endpoints/analytics.py
25-25: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
26-26: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call fastapi_users.current_user in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (4)
ui/package.json (1)
13-16: Dependencies appropriately support the analytics UI feature.The added dependencies (@emotion for styling, @mui/material and @mui/x-date-pickers for UI components, lucide-react for icons, react-day-picker for date selection) align well with the PR's goal of implementing an analytics dashboard with date range selection and charts.
Also applies to: 37-37, 43-43
engine/src/api/endpoints/analytics.py (1)
23-241: Endpoint implementation structure is sound.The metrics aggregation logic correctly handles date ranges, aggregates conversation data, and computes daily breakdowns. Note that previously flagged issues (tool approval counting, period-over-period calculations, exception chaining) are tracked in past review comments.
The static analysis hints (B008) regarding
Query()andDepends()in function defaults are false positives—this is the standard and recommended FastAPI pattern for dependency injection.ui/src/components/ui/calendar.tsx (1)
36-79: Well-structured calendar component with consistent theming.The Calendar component properly wraps MUI's DateCalendar with a dark theme that aligns with the analytics dashboard styling requirements. Props forwarding and className merging are handled correctly.
ui/src/components/analytics/TokenUsageChart.tsx (1)
25-106: Clean and well-configured token usage chart.The stacked area chart properly visualizes the three token types with appropriate gradients, responsive layout, and date formatting. The date handling aligns correctly with the backend's Pydantic serialization (Python
dateobjects are automatically serialized to ISO strings, whichnew Date()parses correctly).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ui/src/lib/api.ts (1)
83-116: Add input validation for date ranges in getMetrics.The past review comment about input validation has not been addressed. The function should validate:
- Date range validation: When using custom dates, both
startDateandendDateshould be provided together, andstartDatemust be beforeendDate.- Parameter precedence: When both
lastNDaysand date fields are provided in the options object, all parameters will be sent to the backend, creating ambiguity about which takes precedence.🔎 Proposed validation logic
} else { + // Validate date range + if ((options.startDate && !options.endDate) || (!options.startDate && options.endDate)) { + throw new Error("Both startDate and endDate must be provided for custom range"); + } + if (options.startDate && options.endDate && options.startDate > options.endDate) { + throw new Error("startDate must be before endDate"); + } + // Prevent mixing lastNDays with custom dates + if (options.lastNDays && (options.startDate || options.endDate)) { + throw new Error("Cannot use both lastNDays and custom date range"); + } const params = []; if (options.lastNDays) params.push(`last_n_days=${options.lastNDays}`);ui/src/components/analytics/TimeRangeSelector.tsx (1)
5-15: Remove duplicate interface declaration.This issue was flagged in a previous review but remains unaddressed. The
TimeRangeSelectorPropsinterface is declared twice (lines 5-8 and lines 12-15), with the import statement placed between them. Remove the duplicate declaration and move the import to the top of the file following conventional organization.🔎 Proposed fix
"use client"; import React from "react"; +import { Button } from "../ui/button"; interface TimeRangeSelectorProps { selectedRange: number | "custom"; onRangeChange: (range: number | "custom") => void; } -import { Button } from "../ui/button"; - -interface TimeRangeSelectorProps { - selectedRange: number | "custom"; - onRangeChange: (range: number | "custom") => void; -} - export default function TimeRangeSelector({ selectedRange, onRangeChange }: TimeRangeSelectorProps) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ui/src/components/analytics/Analytics.tsxui/src/components/analytics/CacheEfficiencyGauge.tsxui/src/components/analytics/CostTrendChart.tsxui/src/components/analytics/DatePickerWithRange.tsxui/src/components/analytics/LatencyChart.tsxui/src/components/analytics/MetricCard.tsxui/src/components/analytics/TimeRangeSelector.tsxui/src/components/analytics/TokenUsageChart.tsxui/src/lib/api.tsui/src/types/analytics.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- ui/src/components/analytics/TokenUsageChart.tsx
- ui/src/components/analytics/CacheEfficiencyGauge.tsx
- ui/src/components/analytics/Analytics.tsx
- ui/src/components/analytics/CostTrendChart.tsx
- ui/src/components/analytics/LatencyChart.tsx
🔇 Additional comments (3)
ui/src/components/analytics/MetricCard.tsx (1)
14-44: LGTM! Clean metric card implementation.The component correctly displays metrics with optional trends and uses appropriate styling. The trend indicator logic (lines 33-40) properly shows positive/negative changes with color coding.
Note: The
motion.divdefines animation variants (lines 17-20), so ensure the parent component providesinitialandanimateprops to trigger the animations.ui/src/components/analytics/TimeRangeSelector.tsx (1)
17-60: LGTM! Time range selector implementation is correct.The component properly renders preset range buttons (7D, 30D, 90D) and a Custom option with appropriate active/inactive styling. The button states are clearly differentiated with proper hover effects.
ui/src/components/analytics/DatePickerWithRange.tsx (1)
108-126: The parent component (Analytics.tsx) already validates that bothdateRange.fromanddateRange.toare present before callinggetMetrics. At line 35, it checksif (dateRange?.from && dateRange?.to)and only proceeds with the API call if both dates are defined. If either is missing, it returns early with "Waiting for full range selection". Partial date ranges cannot reach the API.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/src/components/analytics/TimeRangeSelector.tsxui/src/types/analytics.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/components/analytics/TimeRangeSelector.tsx
🔇 Additional comments (2)
ui/src/types/analytics.ts (2)
33-43: LGTM!DailyMetricstypes are correct.The
DailyMetricsinterface correctly models the backend contract. The optional latency fields (avg_ttft_ms?,avg_ttr_ms?) properly match the backend'sOptional[float]types, and all other fields align with the API response structure.
1-45: Well-structured types file.The overall organization is clean, with clear section comments and consistent naming conventions. Once the optionality issue in
MetricsAggregationis addressed, these types will correctly support the analytics dashboard feature.
|
@KaranJagtiani . I have fixed all the issues raised by the code rabbit. Can you please review the changes. |
KaranJagtiani
left a comment
KaranJagtiani
left a comment
There was a problem hiding this comment.
Remove package.lock.json as we solely rely on yarn. Don't run npm install.
KaranJagtiani
left a comment
KaranJagtiani
left a comment
There was a problem hiding this comment.
@nitishbhosale7 Make sure to run yarn install and clean up package-lock.json and the yarn.lock files.
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In @engine/src/api/endpoints/analytics.py:
- Around line 101-105: The loop currently increments
daily_map[...]["conversation_count"] for every message (using
conv.messages_json), overcounting conversations; change the logic to mark each
conversation once per day by deriving the conversation's day (e.g., from the
first/created message timestamp or a conv-level timestamp) and using a per-day
set to track seen conversation IDs (or conv identifiers) before incrementing
conversation_count; update the block that builds daily_map (references:
conversations, messages_json, daily_map, conversation_count) to add the conv id
to the day's set and only increment conversation_count when the id was not
already seen, then use those sets when constructing DailyMetrics.
- Around line 104-105: The code accesses msg.get("timestamp") and divides by
1000 without a null check, which will raise a TypeError if timestamp is missing;
update the loop in analytics.py where daily_map and daily are used (look for the
lines referencing
daily_map[date.fromtimestamp(msg.get("timestamp")/1000).strftime("%Y-%m-%d")]
and daily["conversation_count"]) to first retrieve the timestamp into a local
variable (e.g., ts = msg.get("timestamp")), validate that ts is not None (and is
numeric), and only perform the division/date conversion and increment
daily["conversation_count"] when ts is present; if ts is missing, skip this
message (or handle it via a fallback path such as logging or using a default) to
prevent the TypeError.
In @ui/src/components/analytics/Analytics.tsx:
- Around line 164-169: The null check for displayData.approval_acceptance_rate
in the MetricCard props is using || which makes the condition always true;
update the condition (in the MetricCard where title is "Tool Approval Acceptance
Rate") to use a proper null/undefined check such as
`displayData.approval_acceptance_rate != null` or combine with && if keeping two
checks, so that when approval_acceptance_rate is null or undefined the value
prop becomes "N/A" and otherwise formats the percentage.
- Around line 170-175: The Average Conversation Cost value divides
displayData.total_cost by displayData.total_conversations without guarding
against zero; update the value passed to MetricCard so you compute a safe
average (e.g., const avg = displayData.total_conversations ?
displayData.total_cost / displayData.total_conversations : 0) and pass the
formatted string `$${avg.toFixed(2)}` (remove the chained .toLocaleString()
after .toFixed()) to avoid "$Infinity" when there are no conversations.
- Around line 128-135: The chip accesses displayData.total_conversations without
guarding for null, causing a potential runtime error when displayData is null;
update the render to check displayData before referencing total_conversations
(e.g., wrap the div or the span in a conditional like displayData && ... or use
a safe fallback value), modifying the code around the DatePickerWithRange and
the chat-bubble div in Analytics (references: displayData, total_conversations,
DatePickerWithRange) so the UI renders safely when displayData is null.
In @ui/src/components/analytics/CostTrendChart.tsx:
- Around line 27-76: The XAxis tickFormatter and Tooltip labelFormatter use new
Date(value) which can parse ISO strings as UTC and cause day shifts; parse the
date string safely (e.g., construct a local Date from the ISO components or use
a library/utility that treats the date as local) inside XAxis's tickFormatter
and Tooltip's labelFormatter to ensure consistent local-day display, and remove
the invalid label="Cost" prop from the Area component (and drop the redundant
stackId="1" since there's only one Area); target the tickFormatter in the XAxis,
labelFormatter in the Tooltip, and the Area element to apply these changes.
In @ui/src/components/analytics/DatePickerWithRange.tsx:
- Around line 69-84: The onSelect handler in DatePickerWithRange currently
preserves date.to when the start (val) is cleared, producing an invalid {from:
undefined, to: <date>} range; update the onSelect logic (the onSelect callback
that calls setDate and setIsStartOpen) so that when val is undefined it also
clears the to value (i.e., setDate should set to: undefined when val is falsy),
while preserving the existing behavior of clearing to when val > date.to and
still closing the start picker with setIsStartOpen(false).
In @ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx:
- Around line 21-27: The comment says days with no data should be filtered but
the code in TimetoFirstTokenAreaChart only maps zeros; update the chartData
computation to actually filter out empty days (e.g., use data.filter(d =>
d.conversation_count > 0).map(...)) so only days with conversation_count > 0
remain and keep the avg_ttft_ms/avg_ttr_ms fallback, or alternatively remove the
misleading comment if you prefer to keep zero-filled entries.
- Around line 39-77: The X axis tickFormatter and Tooltip labelFormatter
currently call new Date(label) which is ambiguous for "YYYY-MM-DD" strings;
instead parse the date string by splitting into year, month, day and construct a
local Date via new Date(year, monthIndex, day) inside the XAxis tickFormatter
and Tooltip labelFormatter so dates render consistently across timezones; also
remove the incorrect label prop and the unnecessary stackId prop from the Area
component (keep the name prop and stroke/fill/animation props).
In @ui/src/components/analytics/TotalResponseTimeAreaChart.tsx:
- Around line 77-86: The Area component is using an incorrect/redundant prop:
remove the label="TTR (Total Response Time)" prop from the <Area ... />
declaration (the Recharts Area uses the name prop for legend/tooltip and the
element already has name="TTR (Total Response Time)"); keep the existing name,
stroke, fill, dataKey, stackId and animationDuration as-is.
In @ui/src/components/ui/calendar.tsx:
- Around line 189-206: The data-day attribute on the Button currently uses
day.date.toLocaleDateString(), which is locale-dependent; change data-day on the
Button to an ISO-stable YYYY-MM-DD value (i.e., use the date's toISOString() and
take the date portion before the 'T', or call a small helper like
formatIsoDate(day.date)) so CSS selectors/tests are stable across locales.
- Around line 175-207: The CalendarDayButton component applies the wrong class
key: replace usage of defaultClassNames.day with defaultClassNames.day_button
when composing the Button's className (inside CalendarDayButton where Button is
rendered) so the clickable button gets the correct v9.13.0 styles; update the
className concat to include defaultClassNames.day_button (and remove or stop
using defaultClassNames.day for the Button) while keeping other className and
props intact.
🧹 Nitpick comments (9)
ui/src/components/ui/calendar.tsx (2)
14-125: PotentialclassNamesmerge footgun:...classNamescan override required layout hooks
Because...classNamesis spread last (Line 125), a consumer can accidentally break core layout (e.g.,day,nav,months) without noticing. Consider documenting “safe override” expectations or shallow-merging specific keys instead of full override.
184-187: Avoid scroll-jank when auto-focusing days
Callingfocus()in an effect (Line 185) can scroll the page/container unexpectedly. Preferref.current?.focus({ preventScroll: true })when supported, with a safe fallback.Proposed tweak
React.useEffect(() => { - if (modifiers.focused) ref.current?.focus() + if (!modifiers.focused) return + // Avoid scroll jumps when navigating months/ranges. + try { + ref.current?.focus({ preventScroll: true }) + } catch { + ref.current?.focus() + } }, [modifiers.focused])ui/src/components/analytics/MetricCard.tsx (1)
14-36: Framer Motionvariantswon’t animate unlessinitial/animateare set (here or by parent).Right now the card defines
variants(Line 17-20) but doesn’t specifyinitial/animate; if the parent isn’t driving variants, this will render statically.Proposed tweak (make it self-contained, still compatible with parent overrides)
export default function MetricCard({ title, value, icon, color }: MetricCardProps) { return ( <motion.div variants={{ hidden: { y: 20, opacity: 0 }, visible: { y: 0, opacity: 1 }, }} + initial="hidden" + animate="visible" className="bg-navbar rounded-lg border border-slate-700/60 p-8 inline-block transition-colors group" >ui/src/components/analytics/CostTrendChart.tsx (1)
46-62: ConsiderIntl.NumberFormatfor currency ticks/tooltip.
tickFormatter={(value) => \$${value}`}` (Line 51) can get noisy (scientific notation / too many decimals). A shared formatter improves consistency across charts.ui/src/components/analytics/DatePickerWithRange.tsx (1)
78-85: Avoid callingnew Date()insidedisabledfor every cell render.This is minor, but you can hoist
const today = new Date()once per render and reference it in bothdisabledcallbacks.Also applies to: 120-126
ui/src/components/analytics/TotalResponseTimeAreaChart.tsx (1)
23-27: Remove unused data normalization.Line 25 normalizes
avg_ttft_msbut the chart only rendersavg_ttr_ms. This is unnecessary and could be confusing.♻️ Proposed cleanup
const chartData = data.map((d) => ({ ...d, - avg_ttft_ms: d.avg_ttft_ms || 0, avg_ttr_ms: d.avg_ttr_ms || 0, }));engine/src/api/endpoints/analytics.py (3)
68-68: Remove unused variables.Lines 68 and 171 assign variables that are never used, as flagged by static analysis.
♻️ Proposed cleanup
Remove Line 68:
- # Update start_date for the loop filter - loop_start_date = period_start.date() -Remove Line 171:
- - sorted_dates = sorted(daily_map.keys()) current_date = period_start.date()Also applies to: 171-171
165-165: Remove debug print statement.Line 165 contains a debug
print()that should be removed or converted to proper logging before production.♻️ Proposed fix
- print('daily',daily) + # Debug: logger.debug(f"Daily stats: {daily}")Or simply remove it entirely.
179-191: Consider using date object instead of string for better type safety.Line 181 passes
formatted_current_date(a string) toDailyMetrics(date=...), but the Pydantic model definesdate: date. While Pydantic will auto-convert ISO format strings to date objects, using the actual date object (current_dateinstead offormatted_current_date) would be more explicit and type-safe.♻️ Proposed improvement
daily_breakdown.append( DailyMetrics( - date=formatted_current_date, + date=current_date, cost=stats["cost"], prompt_tokens=stats["prompt_tokens"], completion_tokens=stats["completion_tokens"], cached_tokens=stats["cached_tokens"], total_tokens=stats["total_tokens"], avg_ttft_ms=avg_ttft, avg_ttr_ms=avg_ttr, conversation_count=stats["conversation_count"], ) )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
engine/src/api/endpoints/analytics.pyengine/src/api/models/conversation.pyui/package.jsonui/src/components/analytics/Analytics.tsxui/src/components/analytics/AnimatedCounter.tsxui/src/components/analytics/CostTrendChart.tsxui/src/components/analytics/DatePickerWithRange.tsxui/src/components/analytics/MetricCard.tsxui/src/components/analytics/TimetoFirstTokenAreaChart.tsxui/src/components/analytics/TokenUsageChart.tsxui/src/components/analytics/TotalResponseTimeAreaChart.tsxui/src/components/ui/calendar.tsxui/src/components/ui/popover.tsxui/src/types/analytics.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/src/components/analytics/TokenUsageChart.tsx
- ui/src/types/analytics.ts
- ui/package.json
🧰 Additional context used
🧬 Code graph analysis (6)
ui/src/components/analytics/CostTrendChart.tsx (3)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-139)ui/src/types/analytics.ts (1)
DailyMetrics(33-43)ui/src/components/ui/tooltip.tsx (1)
Tooltip(42-42)
ui/src/components/analytics/TotalResponseTimeAreaChart.tsx (3)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-139)ui/src/types/analytics.ts (1)
DailyMetrics(33-43)ui/src/components/ui/tooltip.tsx (1)
Tooltip(42-42)
engine/src/api/endpoints/analytics.py (3)
engine/src/api/models/conversation.py (3)
Conversation(12-33)MetricsAggregation(141-160)DailyMetrics(130-139)ui/src/types/analytics.ts (2)
MetricsAggregation(3-30)DailyMetrics(33-43)mcp/tools/jenkins.py (1)
get(99-102)
ui/src/components/analytics/MetricCard.tsx (1)
ui/src/components/analytics/AnimatedCounter.tsx (1)
AnimatedCounter(11-67)
ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx (3)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-139)ui/src/types/analytics.ts (1)
DailyMetrics(33-43)ui/src/components/ui/tooltip.tsx (1)
Tooltip(42-42)
ui/src/components/analytics/Analytics.tsx (2)
ui/src/types/analytics.ts (1)
MetricsAggregation(3-30)ui/src/lib/api.ts (1)
getMetrics(83-116)
🪛 Biome (2.1.2)
ui/src/components/analytics/AnimatedCounter.tsx
[error] 35-35: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 42-42: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 47-47: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🪛 Ruff (0.14.10)
engine/src/api/endpoints/analytics.py
25-25: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
26-26: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call fastapi_users.current_user in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
68-68: Local variable loop_start_date is assigned to but never used
Remove assignment to unused variable loop_start_date
(F841)
171-171: Local variable sorted_dates is assigned to but never used
Remove assignment to unused variable sorted_dates
(F841)
🔇 Additional comments (5)
ui/src/components/analytics/AnimatedCounter.tsx (1)
11-66: [rewritten review comment]
[classification tag]ui/src/components/ui/popover.tsx (1)
1-33: LGTM! Well-structured Radix UI wrapper.The Popover component is correctly implemented following standard shadcn/ui patterns. The forwardRef typing, Portal usage, and className composition are all appropriate.
ui/src/components/analytics/Analytics.tsx (2)
29-72: LGTM! Well-structured data fetching with proper guards.The useEffect correctly handles both preset and custom date ranges, with UTC normalization to avoid timezone issues. The guard for incomplete custom ranges prevents unnecessary API calls.
98-106: LGTM! Well-designed color coding for approval rates.The thresholds provide clear visual feedback on approval rate quality with appropriate color gradations.
engine/src/api/models/conversation.py (1)
130-160: LGTM! Well-defined Pydantic models for analytics.The
DailyMetricsandMetricsAggregationmodels are correctly typed with appropriate use ofOptionalfor nullable fields. The structure aligns well with the API requirements.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @engine/src/api/endpoints/analytics.py:
- Around line 236-240: The current exception handler logs the full error but
returns str(e) to the client; change the HTTPException raised in the metrics
handler (the block that calls logger.exception(...) and raises HTTPException) to
return a generic message like "Internal server error while retrieving metrics"
instead of including str(e), while keeping the logger.exception(...) call
unchanged so the detailed error stays in server logs.
- Line 169: Remove the stray debug print statement print('daily', daily) from
analytics.py; either delete it entirely or replace it with a structured logger
call (e.g., logger.debug("daily=%s", daily) or the project's standard logging
utility) inside the function that computes "daily" so production logs aren't
polluted or sensitive data leaked.
- Around line 107-109: The loop that updates daily_map currently does msg_date =
date.fromtimestamp(timestamp/1000); daily =
daily_map[msg_date.strftime("%Y-%m-%d")]; daily["conversation_count"] += 1 which
increments conversation_count per message; change it to track unique
conversation IDs per day (e.g., add a "conversation_ids" set to each daily_map
entry and add conversation_id to that set instead of incrementing), then when
constructing the final DailyMetrics use conversation_count =
len(daily["conversation_ids"]) (and remove or ignore the set in the serialized
output). Ensure updates touch the same daily_map creation code and the
DailyMetrics assembly logic so conversation_count reflects unique conversations
per day.
In @ui/src/components/analytics/Analytics.tsx:
- Around line 35-58: Validate the normalized UTC dates before calling
getMetrics: after computing fromDate and toDate (the variables created in the
timeRange === "custom" branch), check that fromDate < toDate (or decide behavior
for equality) and if not, abort the request — call setLoading(false) and return
(or set an error state/notification) instead of calling getMetrics with an
invalid range; ensure this check happens before assigning result so malformed
requests are never sent to getMetrics.
🧹 Nitpick comments (11)
engine/src/api/endpoints/analytics.py (5)
27-33: Consider usingcurrent_user(active=True)instead ofoptional=Truewith manual check.Using
optional=Truethen manually raising a 401 duplicates FastAPI Users' built-in behavior. Usingcurrent_user(active=True)(the default) will automatically return 401 for unauthenticated requests.Proposed simplification
- user=Depends(fastapi_users.current_user(optional=True)), + user=Depends(fastapi_users.current_user()), ) -> MetricsAggregation: try: - if not user: - # If no user, return empty metrics or raise error depending on policy. - # Assuming we return empty for unauthenticated to avoid error, or 403. - raise HTTPException(status_code=401, detail="Authentication required for metrics") - period_end_dt = datetime.now()
68-68: Remove unused variableloop_start_date.This variable is assigned but never referenced. As indicated by static analysis.
- # Update start_date for the loop filter - loop_start_date = period_start.date()
175-175: Remove unused variablesorted_dates.This variable is computed but never used.
- sorted_dates = sorted(daily_map.keys())
183-195: Passdateobject instead of string toDailyMetrics.date.The
DailyMetricsmodel definesdate: date, but a string is passed. While Pydantic will parse it, passing the correct type is cleaner and avoids relying on coercion.Proposed fix
daily_breakdown.append( DailyMetrics( - date=formatted_current_date, + date=current_date, cost=stats["cost"], prompt_tokens=stats["prompt_tokens"], completion_tokens=stats["completion_tokens"],
209-209: Consider defaultingapproval_acceptance_ratetoNoneor0.0instead of1.0.Returning 100% acceptance when there are no approvals/rejections may mislead users. A null value or 0.0 more clearly indicates "no data."
- approval_acceptance_rate = (total_approvals / (total_approvals + total_rejections)) if (total_approvals + total_rejections) > 0 else 1.0 + approval_acceptance_rate = (total_approvals / (total_approvals + total_rejections)) if (total_approvals + total_rejections) > 0 else 0.0ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx (3)
22-26: Consider filtering null values instead of normalizing to zero.Normalizing
nullorundefinedlatency values to0can be misleading on charts, as it shows zero latency rather than missing data. Consider either:
- Filtering out days with no latency data, or
- Leaving values as
nulland configuring Recharts to handle gaps (viaconnectNulls={false})Additionally,
avg_ttr_msis normalized but never rendered in this component (onlyavg_ttft_msis visualized). Ifavg_ttr_mswon't be used, remove its normalization to keep the code focused.♻️ Option 1: Filter out null values
- const chartData = data.map((d) => ({ - ...d, - avg_ttft_ms: d.avg_ttft_ms || 0, - avg_ttr_ms: d.avg_ttr_ms || 0, - })); + const chartData = data + .filter((d) => d.avg_ttft_ms != null) + .map((d) => ({ + ...d, + avg_ttft_ms: d.avg_ttft_ms!, + }));
62-62: Excessive precision for millisecond values.Using
toFixed(4)for millisecond values produces unnecessarily precise output like "123.4567ms". For latency metrics, 0-2 decimal places is typically sufficient for readability.♻️ Suggested change
- formatter={(value: number) => [`${value.toFixed(4)}ms`, "TTFT (Time to First Token)"]} + formatter={(value: number) => [`${value.toFixed(2)}ms`, "TTFT (Time to First Token)"]}
73-92: Remove commented-out code.The commented-out
Linecomponents should be removed. Version control preserves history, so there's no need to keep this dead code in the source.🗑️ Proposed cleanup
animationDuration={1500} /> - {/* <Line - type="monotone" - dataKey="avg_ttft_ms" - name="TTFT (Time to First Token)" - stroke="#f59e0b" - strokeWidth={2} - dot={false} - activeDot={{ r: 4 }} - animationDuration={1500} - /> */} - {/* <Line - type="monotone" - dataKey="avg_ttr_ms" - name="TTR (Total Response Time)" - stroke="#ec4899" - strokeWidth={2} - dot={false} - activeDot={{ r: 4 }} - animationDuration={1500} - /> */} </AreaChart>ui/src/components/analytics/Analytics.tsx (3)
74-80: Consider showing loading state during refetches.The condition
loading && !dataprevents the loading indicator from displaying when refetching (e.g., user changes time range). This can confuse users, as they see stale data without feedback that new data is loading.Consider adding a loading overlay or skeleton UI to indicate when data is refreshing.
90-96: Remove uncertain implementation comments and redundant assignment.Lines 90-95 contain comments questioning the implementation approach, suggesting uncertainty. These should be resolved or removed before merging.
Line 96 assigns
datatodisplayDatawithout transformation—this intermediary variable appears unnecessary.♻️ Proposed cleanup
- // If we are in custom mode and waiting for data, show empty or previous data? - // Let's show data if available, or loading. - - // If no data yet (e.g. custom mode selected but no dates), show prompt? - // Or just empty state. - - const displayData = data; + const displayData = data;Or simply use
datadirectly throughout the component instead ofdisplayData.
98-106: Extract approval acceptance rate color logic into a function.The deeply nested ternary operator (9 levels) for determining color is difficult to read and maintain. Extract this into a named helper function for clarity.
♻️ Proposed refactor
+ const getApprovalRateColor = (rate: number | null | undefined): string => { + if (rate == null) return "text-text-secondary"; + if (rate < 0.5) return "text-red-500"; + if (rate < 0.6) return "text-orange-600"; + if (rate < 0.7) return "text-orange-400"; + if (rate < 0.8) return "text-yellow-400"; + if (rate < 0.9) return "text-lime-400"; + return "text-green-400"; + }; + const rate = displayData?.approval_acceptance_rate; - const approvalAcceptanceRateColor = - rate == null ? "text-text-secondary" : - rate < 0.5 ? "text-red-500" : - rate < 0.6 ? "text-orange-600" : - rate < 0.7 ? "text-orange-400" : - rate < 0.8 ? "text-yellow-400" : - rate < 0.9 ? "text-lime-400" : - "text-green-400"; + const approvalAcceptanceRateColor = getApprovalRateColor(rate);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
engine/src/api/endpoints/analytics.pyui/src/components/analytics/Analytics.tsxui/src/components/analytics/TimetoFirstTokenAreaChart.tsxui/src/components/analytics/TotalResponseTimeAreaChart.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/src/components/analytics/TotalResponseTimeAreaChart.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx (3)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-139)ui/src/types/analytics.ts (1)
DailyMetrics(33-43)ui/src/components/ui/tooltip.tsx (1)
Tooltip(42-42)
engine/src/api/endpoints/analytics.py (2)
engine/src/api/models/conversation.py (3)
Conversation(12-33)MetricsAggregation(141-160)DailyMetrics(130-139)ui/src/types/analytics.ts (2)
MetricsAggregation(3-30)DailyMetrics(33-43)
🪛 Ruff (0.14.10)
engine/src/api/endpoints/analytics.py
25-25: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
26-26: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call fastapi_users.current_user in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
68-68: Local variable loop_start_date is assigned to but never used
Remove assignment to unused variable loop_start_date
(F841)
175-175: Local variable sorted_dates is assigned to but never used
Remove assignment to unused variable sorted_dates
(F841)
🔇 Additional comments (8)
engine/src/api/endpoints/analytics.py (1)
60-64: Verify filtering strategy: query filters conversations byupdated_at, but messages are processed by their individualtimestamp.A conversation updated within the period may contain messages from outside the period window. Consider whether to:
- Filter messages by timestamp within the loop (current behavior partially does this via
msg_date)- Or query by
created_atinstead, depending on intended semanticsThis may be intentional, but verify it aligns with expected behavior for the analytics use case.
ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx (1)
1-20: LGTM!Imports and type definitions are clean and properly structured.
ui/src/components/analytics/Analytics.tsx (6)
1-27: LGTM!State initialization and imports are properly structured.
108-144: LGTM!Header section with responsive layout and conditional rendering is well-structured.
146-178: LGTM!Metric cards section properly handles null values and division by zero. Responsive grid and animations are well-implemented.
180-204: LGTM!Charts sections are well-organized with responsive layouts and consistent styling. Data props are correctly passed to all chart components.
208-212: LGTM!Empty state provides clear guidance for users in custom mode without a selected date range.
38-48: The UTC normalization in this code is correctly implemented. The code extracts local date components (getFullYear(),getMonth(),getDate()which return local values) and constructs a UTC Date to ensure consistent serialization. WhentoISOString().split("T")[0]is called, it produces the calendar date string that matches what the user selected, regardless of timezone. For example, a PST user selecting "Jan 1, 2025" results in the date string "2025-01-01" being sent to the backend, not a shifted date. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @engine/src/api/endpoints/analytics.py:
- Around line 178-189: DailyMetrics.date is a date-typed field but the code
passes formatted_current_date (a string); replace the string with the actual
date object (e.g., current_date) when constructing DailyMetrics in the
daily_breakdown append, and ensure the map/lookup uses date objects consistently
by storing and accessing keys in daily_map as date objects (or convert keys
reliably before lookup) so created DailyMetrics receives a date, not a formatted
string; update any nearby lookup around daily_map and where
formatted_current_date is used to use the date object instead.
- Line 164: Remove the stray debug print statement print('daily', daily) in
analytics.py and either delete it or replace it with a proper logger call (e.g.,
logger.debug with a clear message and the daily variable) so debugging output
uses the module logger instead of printing to stdout; update the call site where
the print appears (the handler/function that uses the daily variable) to use
logger.debug("daily: %s", daily) or remove entirely if not needed.
In @engine/src/api/models/conversation.py:
- Around line 130-138: The DailyMetrics model declares date: date but
analytics.py constructs formatted_current_date =
current_date.strftime("%Y-%m-%d") (a string); fix by making types consistent:
either change DailyMetrics.date to str or pass a date object into DailyMetrics
(e.g., use current_date or current_date.date() instead of
formatted_current_date). Locate the DailyMetrics class in conversation.py and
the formatted_current_date/current_date usage in analytics.py and apply the
chosen consistent approach across where DailyMetrics instances are created.
🧹 Nitpick comments (6)
ui/src/components/analytics/ConversationIndicator.tsx (1)
10-17: Consider defensive handling for edge cases.If
totalConversationsisNaNor comes from an API response that could benull/undefined, calling.toLocaleString()on it may produce unexpected results. Since the parent component (Analytics.tsx) guards withdisplayData && (...), this is likely safe, but adding a fallback would be more robust.🔧 Optional defensive fix
const ConversationIndicator = ({ totalConversations }: { totalConversations: number }) => { + const safeValue = typeof totalConversations === 'number' && !isNaN(totalConversations) + ? totalConversations + : 0; return ( <TooltipProvider> <Tooltip> <TooltipTrigger asChild> <div className="flex align-middle gap-2 items-center w-min mx-auto rounded-lg p-2 text-slate-400 bg-blue-400/5 hover:bg-blue-400/20 hover:text-blue-400 transition-all duration-300 text-lg"> <MdOutlineChatBubbleOutline className="text-2xl" /> - <span>{totalConversations.toLocaleString()}</span> + <span>{safeValue.toLocaleString()}</span> </div> </TooltipTrigger>ui/src/components/analytics/CacheEfficiencyGauge.tsx (1)
19-26: Dead code:percentage === nullcheck is unreachable.
percentageis always anumber(result ofMath.round()), so thepercentage === nullbranch will never execute. This can be safely removed.🧹 Remove dead code
const cacheEfficiencyColor = - percentage === null ? "#94a3b8" : - percentage < 50 ? "#ef4444" : + percentage < 50 ? "#ef4444" : percentage < 60 ? "#ea580c" : percentage < 70 ? "#fb923c" : percentage < 80 ? "#facc15" : percentage < 90 ? "#a3e635" : "#4ade80";ui/src/components/analytics/Analytics.tsx (2)
13-18: Unused imports: Tooltip components are not used in this file.The
Tooltip,TooltipContent,TooltipTrigger, andTooltipProviderimports are declared but never used in this component. Consider removing them to keep the imports clean.🧹 Remove unused imports
import { type DateRange } from "react-day-picker" -import { - Tooltip, - TooltipContent, - TooltipTrigger, - TooltipProvider, -} from "@/components/ui/tooltip"; import { DatePickerWithRange } from "./DatePickerWithRange";
175-180: Minor: Duplicated calculation for average conversation cost.The
MetricsAggregationalready includesavg_cost_per_conversationfrom the backend. You're recalculating it here astotal_cost / total_conversations. Consider using the pre-computed value for consistency:value={displayData.avg_cost_per_conversation > 0 ? `$${displayData.avg_cost_per_conversation.toFixed(2)}` : "$0.00"}engine/src/api/endpoints/analytics.py (2)
68-68: Remove unused variables.As noted by static analysis,
loop_start_dateandsorted_datesare assigned but never used. Remove them to clean up the code.🧹 Remove unused variables
- # Update start_date for the loop filter - loop_start_date = period_start.date()- sorted_dates = sorted(daily_map.keys()) current_date = period_start.date()Also applies to: 170-170
100-163: Consider performance implications for large datasets.The current implementation iterates through all messages in memory. For users with many conversations, this could become slow. Consider:
- Adding pagination or date-based partitioning for very large datasets
- Caching aggregated metrics
- Using database-level aggregation if message data is stored in a queryable format
This is acceptable for an initial implementation but worth monitoring.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
engine/src/api/endpoints/analytics.pyengine/src/api/models/conversation.pyui/src/components/analytics/Analytics.tsxui/src/components/analytics/CacheEfficiencyGauge.tsxui/src/components/analytics/ConversationIndicator.tsxui/src/types/analytics.ts
🧰 Additional context used
🧬 Code graph analysis (4)
engine/src/api/endpoints/analytics.py (3)
engine/src/api/models/conversation.py (3)
Conversation(12-33)MetricsAggregation(140-157)DailyMetrics(130-138)ui/src/types/analytics.ts (2)
MetricsAggregation(3-26)DailyMetrics(29-38)mcp/tools/jenkins.py (1)
get(99-102)
ui/src/components/analytics/ConversationIndicator.tsx (1)
ui/src/components/ui/tooltip.tsx (4)
TooltipProvider(42-42)Tooltip(42-42)TooltipTrigger(42-42)TooltipContent(42-42)
ui/src/types/analytics.ts (1)
engine/src/api/models/conversation.py (2)
MetricsAggregation(140-157)DailyMetrics(130-138)
engine/src/api/models/conversation.py (1)
ui/src/types/analytics.ts (2)
DailyMetrics(29-38)MetricsAggregation(3-26)
🪛 Ruff (0.14.10)
engine/src/api/endpoints/analytics.py
25-25: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
26-26: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call fastapi_users.current_user in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
68-68: Local variable loop_start_date is assigned to but never used
Remove assignment to unused variable loop_start_date
(F841)
170-170: Local variable sorted_dates is assigned to but never used
Remove assignment to unused variable sorted_dates
(F841)
🔇 Additional comments (6)
ui/src/components/analytics/CacheEfficiencyGauge.tsx (1)
10-67: LGTM!The gauge component is well-implemented with proper value clamping, responsive container, and clean visualization. The color gradient provides intuitive feedback on cache efficiency.
ui/src/types/analytics.ts (1)
3-38: LGTM! Types align well with backend models.The interfaces correctly represent the JSON-serialized shapes from the backend. The use of
stringfor date/datetime fields is appropriate since JSON doesn't have native date types.engine/src/api/models/conversation.py (1)
140-157: LGTM!The
MetricsAggregationmodel is well-structured with comprehensive metrics coverage including totals, averages, derived metrics, and approval tracking. The optionalapproval_acceptance_ratecorrectly handles the case when no approvals/rejections exist.ui/src/components/analytics/Analytics.tsx (1)
35-78: LGTM! Well-structured data fetching logic.The
useEffecthandles both preset time ranges and custom date ranges correctly. UTC normalization for custom dates avoids timezone-related issues. The dependency array is appropriate.engine/src/api/endpoints/analytics.py (2)
23-28: Static analysis false positives for FastAPI patterns.The B008 warnings about
Query()andDepends()in argument defaults are false positives. This is the idiomatic FastAPI pattern for dependency injection and query parameter definitions. No changes needed.
29-33: LGTM! Good error handling pattern.The endpoint correctly handles authentication, re-raises
HTTPExceptionwithout wrapping, and logs unexpected errors before returning a 500 response. The 401 for unauthenticated users is appropriate.Also applies to: 225-231
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @engine/src/api/endpoints/analytics.py:
- Around line 205-223: The response is missing period-over-period fields: add
float fields cost_change_pct and tokens_change_pct to the MetricsAggregation
model in engine/src/api/models/conversation.py, then in the analytics endpoint
where MetricsAggregation is constructed (the return block using total_cost and
total_tokens in engine/src/api/endpoints/analytics.py) fetch the previous period
totals (previous_total_cost, previous_total_tokens), compute percentage change
as (current - previous) / previous * 100 with safe handling for previous == 0
(use None or 0 and document choice), assign the results to cost_change_pct and
tokens_change_pct, and include those values when constructing and returning
MetricsAggregation; ensure you use the existing variable names total_cost and
total_tokens so the new fields reflect period-over-period comparisons.
- Around line 59-98: Remove unused variable assignments to clean up the
analytics endpoint: delete the assignment to loop_start_date (symbol
loop_start_date) after computing period_start since it is never referenced, and
remove the assignment to sorted_dates (symbol sorted_dates) where the date
sorting is computed but not used; ensure any logic that relied on these
variables is either removed or replaced with the actual used iteration over
daily_map keys so no references remain.
- Around line 225-232: The current except block logs the full exception but
includes str(e) in the HTTPException detail, risking sensitive info exposure;
change the HTTPException detail to a generic message (e.g., "Internal server
error while retrieving metrics") while keeping logger.exception(f"Error getting
metrics: {str(e)}") for internal logs, and remove the raw exception string from
the raise of HTTPException in the exception handler around the metrics endpoint
(the except Exception block that calls logger.exception and raises
HTTPException).
- Around line 35-57: The period end calculation is inconsistent: when a custom
range is provided the code sets period_end to end-of-day
(datetime.combine(e_date, datetime.max.time())) but when using last_n_days it
uses datetime.now(); change the last_n_days branch to compute e_date as
date.today() and set period_end = datetime.combine(e_date, datetime.max.time())
and update period_end_dt accordingly (matching the custom range behavior), and
remove the redundant fallback "days = last_n_days if last_n_days else 30" since
last_n_days already defaults to 30; touch symbols: start_date, last_n_days,
e_date, s_date, period_start, period_end, period_end_dt.
🧹 Nitpick comments (1)
engine/src/api/endpoints/analytics.py (1)
100-164: Message processing logic is correct.The logic properly:
- Handles None cases for
messages_json- Filters messages by timestamp (assuming milliseconds)
- Tracks tool approvals/rejections only for segments requiring approval
- Accumulates token usage, cost, and latency with defensive None checks
The implementation is sound, though extracting the message processing into a helper function would improve readability for this 60+ line loop.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
engine/src/api/endpoints/analytics.py
🧰 Additional context used
🧬 Code graph analysis (1)
engine/src/api/endpoints/analytics.py (1)
engine/src/api/models/conversation.py (3)
Conversation(12-33)MetricsAggregation(140-157)DailyMetrics(130-138)
🪛 Ruff (0.14.10)
engine/src/api/endpoints/analytics.py
25-25: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
26-26: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call fastapi_users.current_user in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
68-68: Local variable loop_start_date is assigned to but never used
Remove assignment to unused variable loop_start_date
(F841)
171-171: Local variable sorted_dates is assigned to but never used
Remove assignment to unused variable sorted_dates
(F841)
🔇 Additional comments (4)
engine/src/api/endpoints/analytics.py (4)
1-14: LGTM! Clean imports and router setup.The imports are well-organized and the router initialization follows FastAPI conventions.
18-28: Endpoint definition looks good.The static analysis warnings (B008) about
QueryandDependsin function defaults are false positives—this is the standard FastAPI pattern for dependency injection and query parameter declaration.
29-33: LGTM! Proper authentication check.Returns a 401 status with a clear message for unauthenticated requests.
167-204: Aggregation and daily breakdown logic is solid.The implementation correctly:
- Ensures all dates in the period have entries (lines 172-190), which is essential for continuous chart rendering
- Performs zero-division checks for all average calculations
- Computes cache hit rate and approval acceptance rate
The cache hit rate formula (
cached_tokens / prompt_tokens) aligns with the standard LLM prompt-caching metric used by OpenAI and Azure, where cached tokens represent the fraction of input tokens served from cache.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @engine/src/api/endpoints/analytics.py:
- Around line 37-48: Add validation to ensure start_date is not after end_date
inside the branch that sets s_date/e_date: if start_date (s_date) > e_date,
immediately return a 400 Bad Request with a clear error message (e.g.
"start_date must be on or before end_date") instead of continuing to compute
period_start/period_end; update the handler that uses variables s_date, e_date,
period_start, period_end (in analytics.py) to perform this check and return an
appropriate HTTP error (HTTPException or JSON response) when the condition
fails.
🧹 Nitpick comments (3)
engine/src/api/endpoints/analytics.py (3)
23-33: Consider usingoptional=Falseto simplify authentication handling.Using
optional=Truethen manually raising 401 adds unnecessary code. FastAPI will automatically return 401 whenoptional=Falseand the user is not authenticated.♻️ Proposed simplification
async def get_metrics( last_n_days: Optional[int] = Query(default=30, ge=1, le=365), start_date: Optional[date] = Query(default=None), end_date: Optional[date] = Query(default=None), - user=Depends(fastapi_users.current_user(optional=True)), + user=Depends(fastapi_users.current_user()), ) -> MetricsAggregation: try: - if not user: - # If no user, return empty metrics or raise error depending on policy. - # Assuming we return empty for unauthenticated to avoid error, or 403. - raise HTTPException(status_code=401, detail="Authentication required for metrics") - period_end_dt = datetime.now()
100-106: Add defensive handling for malformed timestamp data.If
timestampis not a valid number (e.g., a string or malformed value),date.fromtimestamp(timestamp/1000)will raise an exception. Consider wrapping in a try/except to skip malformed messages gracefully.♻️ Proposed defensive handling
for msg in messages: timestamp = msg.get("timestamp") if not timestamp: continue - msg_date = date.fromtimestamp(timestamp/1000) + try: + msg_date = date.fromtimestamp(timestamp / 1000) + except (TypeError, ValueError, OSError): + # Skip messages with invalid timestamps + continue if msg_date > period_end_dt.date() or msg_date < period_start.date(): continue
170-188: Minor duplication: default stats structure is defined twice.The default stats dictionary on line 172 duplicates the structure from the
defaultdictlambda on lines 70-80. Consider extracting to a helper function.♻️ Proposed refactor
+def _empty_daily_stats() -> Dict[str, Any]: + return { + "cost": 0.0, + "prompt_tokens": 0, + "completion_tokens": 0, + "cached_tokens": 0, + "total_tokens": 0, + "ttft_ms_sum": 0, + "ttft_count": 0, + "ttr_ms_sum": 0, + "ttr_count": 0, + } # In get_metrics: - daily_map: Dict[date, Dict[str, Any]] = defaultdict( - lambda: { - "cost": 0.0, - ... - } - ) + daily_map: Dict[date, Dict[str, Any]] = defaultdict(_empty_daily_stats) # And later: - stats = daily_map.get(current_date, {"cost": 0.0, ...}) + stats = daily_map.get(current_date, _empty_daily_stats())
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
engine/src/api/endpoints/analytics.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-10T15:42:06.735Z
Learnt from: nitishbhosale7
Repo: skyflo-ai/skyflo PR: 73
File: engine/src/api/endpoints/analytics.py:35-57
Timestamp: 2026-01-10T15:42:06.735Z
Learning: In the analytics metrics endpoint (engine/src/api/endpoints/analytics.py), the period_end calculation intentionally differs between modes: last_n_days uses datetime.now() for a rolling real-time window, while custom date ranges use datetime.max.time() for day-aligned boundaries. This is by design to support different use cases.
Applied to files:
engine/src/api/endpoints/analytics.py
🪛 Ruff (0.14.10)
engine/src/api/endpoints/analytics.py
25-25: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
26-26: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call fastapi_users.current_user in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (6)
engine/src/api/endpoints/analytics.py (6)
1-11: LGTM!Imports are appropriate and all appear to be used within the module.
59-65: Verify filtering logic aligns with requirements.The query filters by
updated_atbut messages are later filtered by their individualtimestamp. A conversation that has messages within the period but was last updated (e.g., metadata change) outside the period would be missed. If this is intentional for performance, consider documenting this behavior.
68-96: LGTM!The aggregation structures are well-designed using
defaultdictfor efficient per-day accumulation.
108-122: LGTM!The approval/rejection counting logic correctly identifies tool executions that require approval and tracks their completion status.
124-162: LGTM!Token usage and latency accumulation logic is correct with proper null checks for optional latency values.
203-221: Verify: Period-over-period comparison fields are not implemented.The PR objectives specify
cost_change_pctandtokens_change_pctfor period-over-period comparisons, but these are not calculated or included in the response. Confirm if this is intentional or if implementation is pending.
KaranJagtiani
left a comment
KaranJagtiani
left a comment
There was a problem hiding this comment.
Squash all your commits into one.
nitishbhosale7
force-pushed
the
#71/feat/analytics-dashboard-for-token-usage
branch
from
16c8369 to
629e0c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/src/components/chat/ChatInput.tsx (1)
17-37:handleSubmitis called with a KeyboardEvent but typed as FormEvent (likely TS error).
Both textareas callhandleSubmit(e)fromonKeyDown;ethere is aReact.KeyboardEvent, not aReact.FormEvent.Minimal fix (don’t pass the keyboard event)
- onKeyDown={(e) => { + onKeyDown={(e) => { if (e.key === "Enter" && !e.shiftKey) { e.preventDefault(); - handleSubmit(e); + handleSubmit(); } }}Apply the same change in the non-
topSlottextarea block as well.engine/src/api/agent/graph.py (1)
385-397: Verify MCPClient resource cleanup—explicit close call removal creates resource leak risk.The removal of
mcp_client.close()from theclose()method creates a resource leak. MCPClient's__aexit__is a no-op (line 24-25 in mcp_client.py), and ToolExecutor.close() only nullifies the reference without invoking cleanup (line 65-67 in tool_executor.py). TheStreamableHttpTransportcreated in MCPClient._get_client() has no explicit cleanup mechanism. Since graph.py directly owns the MCPClient instance (created at line 67 in graph.py) but passes it to ToolExecutor withowns_client=False, the cleanup responsibility falls back to graph.py. Either restore the explicitawait self.mcp_client.close()call in graph.py's close() method, or implement proper resource cleanup in MCPClient's__aexit__to ensure the transport is closed.
🤖 Fix all issues with AI agents
In @deployment/install.yaml:
- Around line 1019-1022: The production MCP Deployment in
deployment/install.yaml is missing health checks—add both readinessProbe and
livenessProbe to the MCP container using an HTTP GET to path /mcp/v1/health on
port "http" (port 8888) and copy the probe configuration (initialDelaySeconds,
periodSeconds, timeoutSeconds, successThreshold, failureThreshold) used in
deployment/local.install.yaml (lines ~995–1006) so the probes match exactly;
place the probes under the MCP container spec in the same structure as the local
file.
In @engine/src/api/agent/model_node.py:
- Around line 120-132: The helper emit_ttft_if_needed can raise a TypeError when
event_callback is None; update emit_ttft_if_needed to check that event_callback
is truthy before awaiting it (i.e., ensure event_callback is not None and
callable) and only call await event_callback(...) when that check passes; keep
the existing logic that computes ttft_duration and sets new_ttft_emitted when an
event is emitted, referencing emit_ttft_if_needed, event_callback, ttft_emitted,
new_ttft_emitted, start_time, now_ms, and run_id to locate the change.
In @engine/src/api/config/settings.py:
- Line 31: MCP_SERVER_URL currently includes a /mcp prefix but the FastMCP
server is mounted at root; update the engine config by changing MCP_SERVER_URL
in settings.py from "http://127.0.0.1:8888/mcp" to "http://127.0.0.1:8888" and
also update any health-check paths that use "/mcp/v1/health" to use "/v1/health"
(and similarly adjust any references to the /mcp prefix elsewhere), or
alternatively modify the FastMCP server mounting to serve under the "/mcp"
prefix so that the MCP_SERVER_URL and Kubernetes health probes (which reference
/mcp/v1/health) match the running server.
In @engine/src/api/endpoints/agent.py:
- Around line 151-162: The cached_tokens value coming from event may be
str|float|None and can cause TypeErrors in persistence.record_token_usage;
coerce it the same way as other token fields before persisting (e.g., replace
cached_tokens=event.get("cached_tokens") with a safe numeric cast like
cached_tokens=int(event.get("cached_tokens") or 0) or float(...) as
appropriate), so update the call to persistence.record_token_usage in the branch
handling event_type == "token.usage" (referencing variables event, event_run_id,
conversation_id) and keep the subsequent await
persistence.apply_usage_snapshot(...) call unchanged.
In @engine/src/api/services/conversation_persistence.py:
- Around line 8-32: The _usage_buffers dict can grow indefinitely if runs never
call finalize_usage_snapshot/_clear_usage_buffer; add eviction to prevent a
memory leak by implementing a bounded buffer or TTL cleanup: modify the class
that defines _usage_buffers to store metadata (e.g., timestamp) for each key
created by _usage_key, update _get_usage_buffer to enforce a max entries limit
(evict oldest entries via timestamp) or remove entries older than a TTL on
access, and ensure _clear_usage_buffer and finalize_usage_snapshot still remove
entries when runs complete; reference _usage_buffers, _usage_key,
_get_usage_buffer, _clear_usage_buffer, and finalize_usage_snapshot when making
changes.
In @ui/src/components/analytics/DatePickerWithRange.tsx:
- Around line 120-126: In the disabled callback inside DatePickerWithRange (the
disabled={(day) => {...}} arrow function), normalize both dates to date-only
before comparing to avoid time-of-day effects: convert `day` and `new Date()` to
midnight-only Dates (e.g., new Date(day.getFullYear(), day.getMonth(),
day.getDate()) and new Date(today.getFullYear(), today.getMonth(),
today.getDate())) or otherwise strip time via setHours(0,0,0,0), then use those
normalized values in the comparisons (`if (date?.from && dayOnly < fromOnly)`
and `if (dayOnly > todayOnly)`).
- Around line 78-84: The disabled predicate in DatePickerWithRange.tsx compares
full Date objects (day > new Date()), causing time-of-day inconsistencies;
change the comparisons to use date-only comparisons by normalizing both operands
to midnight (e.g., convert day and date?.to and the current date to their
date-only equivalents or use a startOfDay utility) before comparing so
future-day checks and end-date bounds are based on calendar days rather than
timestamps.
In @ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx:
- Around line 21-27: The TimetoFirstTokenAreaChart component lacks handling for
an empty data array; update the TimetoFirstTokenAreaChart function to check if
the incoming data prop is empty (data.length === 0) and return an appropriate
empty-state UI/message instead of rendering the AreaChart, or alternatively
ensure the parent verifies displayData.daily_breakdown has entries before
rendering this component; reference the TimetoFirstTokenAreaChart function and
the data prop when adding the guard and the user-facing empty-state message.
In @ui/src/components/chat/ChatInterface.tsx:
- Around line 308-338: The 100ms setTimeout introduces a race; replace the
delayed callback with a deterministic, immediate finalization while keeping the
idempotent guard. Remove setTimeout and run the block synchronously: check
hasFinalizedRef.current and return if true, set hasFinalizedRef.current = true,
call setIsStreaming(false), update setCurrentMessage and setMessages exactly as
in the callback, and then clear isApprovalActionRef.current and
approvalDecisionRef.current; if you need to defer to the end of the current tick
use Promise.resolve().then(...) instead of setTimeout to avoid races.
In @ui/src/components/chat/ChatMessages.tsx:
- Around line 66-115: renderAssistantSegments currently shows TokenUsageDisplay
only on hover (visible={isHovered}), which excludes touch and keyboard users;
update it to support keyboard and touch by adding focus and toggle state: add a
focus state (e.g., focusedMessageId or a per-message isToggled flag) and wire
onFocus/onBlur and tabIndex=0 to the assistant message container returned by
renderAssistantSegments (or add an explicit toggle button with onClick and
onKeyDown handlers), then change the TokenUsageDisplay visible prop to combine
hovered || focused || toggled (e.g., visible={isHovered || isFocused ||
isToggled}); also add appropriate aria attributes (aria-expanded/aria-controls)
to the container or button for accessibility and ensure the same handlers used
for hover are updated to manage the new focus/toggle state.
- Around line 75-107: Currently the map assumes any non-"text" segment has
seg.toolExecution and renders ToolVisualization, which will crash for unknown
kinds; update the branch to explicitly check seg.kind === "tool" (or the exact
tool kind used) before rendering ToolVisualization and only access
seg.toolExecution when that check passes, and add a safe fallback render for
unknown kinds (e.g., a neutral div or null) that uses seg.id and a safe
representation of content so the UI won’t crash; keep existing props on
ToolVisualization (isExpanded, onApprovalAction, disableApprovalActions) when
rendering the tool case.
In @ui/src/components/chat/TokenUsageDisplay.tsx:
- Around line 62-180: TokenUsageDisplay's tooltips use non-focusable divs as
TooltipTrigger and the container is only visually hidden when visible is false;
make the tooltip triggers keyboard-accessible by replacing the TooltipTrigger
children divs or adding attributes so they are focusable (use a semantic button
or add tabIndex={0} and role="button" and aria-haspopup="true" to the
TooltipTrigger asChild element) for every occurrence of TooltipTrigger, and
ensure the wrapper respects accessibility by adding aria-hidden={!visible} on
the motion.div (or conditionally not rendering the whole
TooltipProvider/motion.div when visible is false) so screen readers don't see
hidden content; locate these changes around the TooltipTrigger usages and the
motion.div that uses the visible prop in TokenUsageDisplay.
🧹 Nitpick comments (13)
ui/src/components/analytics/DatePickerWithRange.tsx (1)
31-40: Move GlassWrapper outside the component to avoid recreation on every render.The
GlassWrappercomponent is currently defined insideDatePickerWithRange, causing it to be recreated on every render. This is inefficient and can impact performance.♻️ Move GlassWrapper outside the component
+// Wraps content in the liquid glass effect structure +const GlassWrapper = ({ children }: { children: React.ReactNode }) => ( + <div className="lg-wrapper"> + <div className="lg-effect" /> + <div className="lg-tint" /> + <div className="lg-shine" /> + <div className="relative z-10 p-2"> + {children} + </div> + </div> +) + export function DatePickerWithRange({ className, date, setDate, }: DatePickerWithRangeProps) { const [isStartOpen, setIsStartOpen] = React.useState(false) const [isEndOpen, setIsEndOpen] = React.useState(false) - // Wraps content in the liquid glass effect structure - const GlassWrapper = ({ children }: { children: React.ReactNode }) => ( - <div className="lg-wrapper"> - <div className="lg-effect" /> - <div className="lg-tint" /> - <div className="lg-shine" /> - <div className="relative z-10 p-2"> - {children} - </div> - </div> - ) - return (ui/src/components/analytics/TimeRangeSelector.tsx (2)
12-17: Move ranges array outside the component for better performance.The
rangesarray is currently recreated on every render. Since it's static data, it should be defined as a constant outside the component.♻️ Move ranges array outside component
+const RANGES: { label: string; value: number | "custom" }[] = [ + { label: "7D", value: 7 }, + { label: "30D", value: 30 }, + { label: "90D", value: 90 }, +]; + export default function TimeRangeSelector({ selectedRange, onRangeChange }: TimeRangeSelectorProps) { - const ranges: { label: string; value: number | "custom" }[] = [ - { label: "7D", value: 7 }, - { label: "30D", value: 30 }, - { label: "90D", value: 90 }, - - ]; - return ( <div className="flex bg-dark-card border max-lg:flex-col items-center border-white/10 rounded-lg p-1 gap-1"> <div className="flex gap-1"> - {ranges.map((range) => ( + {RANGES.map((range) => (
22-34: Consider extracting button styling logic to reduce duplication.The button styling logic is duplicated between the predefined ranges and the custom button. Consider extracting this into a helper function or deriving the className more cleanly.
♻️ Example refactor to reduce duplication
export default function TimeRangeSelector({ selectedRange, onRangeChange }: TimeRangeSelectorProps) { + const getButtonClassName = (isSelected: boolean) => + `h-8 px-4 text-sm font-medium transition-all duration-200 ${ + isSelected + ? "shadow-lg text-blue-400 bg-blue-400/5" + : "text-text-secondary hover:text-blue-400 hover:bg-blue-400/5 duration-300" + }`; + return ( <div className="flex bg-dark-card border max-lg:flex-col items-center border-white/10 rounded-lg p-1 gap-1"> <div className="flex gap-1"> {RANGES.map((range) => ( <Button key={range.value} variant={selectedRange === range.value ? "default" : "ghost"} onClick={() => onRangeChange(range.value)} - className={`h-8 px-4 text-sm font-medium transition-all duration-200 ${selectedRange === range.value - ? "shadow-lg text-blue-400 bg-blue-400/5" - : "text-text-secondary hover:text-blue-400 hover:bg-blue-400/5 duration-300" - }`} + className={getButtonClassName(selectedRange === range.value)} > {range.label} </Button> ))} </div> <div> <Button - key={"custom"} + key="custom" variant={selectedRange === "custom" ? "default" : "ghost"} onClick={() => onRangeChange("custom")} - className={`h-8 px-4 text-sm font-medium transition-all duration-200 ${selectedRange === "custom" - ? "shadow-lg text-blue-400 bg-blue-400/5" - : "text-text-secondary hover:text-blue-400 hover:bg-blue-400/5 duration-300" - }`} + className={getButtonClassName(selectedRange === "custom")} > - {"Custom"} + Custom </Button> </div> </div>Also applies to: 38-48
ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx (2)
22-26: Remove unused avg_ttr_ms normalization.The component normalizes
avg_ttr_msbut never uses it in the chart (onlyavg_ttft_msis rendered). This is dead code that should be removed for clarity.Additionally, setting
nullorundefinedlatencies to0may misrepresent the data—days with no conversations will appear identical to days with 0ms latency. Consider either filtering out days with no data or using a sentinel value that Recharts can handle gracefully.♻️ Proposed refactor to remove unused normalization
- const chartData = data.map((d) => ({ - ...d, - avg_ttft_ms: d.avg_ttft_ms || 0, - avg_ttr_ms: d.avg_ttr_ms || 0, - })); + const chartData = data.map((d) => ({ + ...d, + avg_ttft_ms: d.avg_ttft_ms ?? 0, + }));Note: Changed
||to??(nullish coalescing) to preserve explicit0values while defaulting onlynull/undefined.
62-62: Consider reducing decimal precision in the tooltip.Using
.toFixed(4)for millisecond values provides precision down to 0.0001ms, which is likely excessive for TTFT metrics that typically range in the hundreds or thousands of milliseconds. Consider reducing to 1-2 decimal places for better readability.♻️ Proposed adjustment
- formatter={(value: number) => [`${value.toFixed(4)}ms`, "TTFT (Time to First Token)"]} + formatter={(value: number) => [`${value.toFixed(2)}ms`, "TTFT (Time to First Token)"]}ui/src/components/analytics/MetricCard.tsx (1)
7-12: Tightencolorprop to avoid Tailwind purge / arbitrary class strings.If
coloris meant to be a Tailwind class (e.g.,text-emerald-400), consider a union of allowed values (or aMetricCardColormap) rather thanstring; otherwise Tailwind may drop the class if it’s not statically discoverable.engine/src/api/endpoints/analytics.py (2)
69-73: Minor: Query filter usesupdated_atbut messages are filtered bytimestamp.The conversation query filters by
updated_at, but individual messages are then filtered by theirtimestamp. This could lead to edge cases where:
- A conversation updated outside the period (e.g., title change) but with messages inside the period would be excluded.
- A conversation updated inside the period but with all messages outside would contribute zero metrics.
This is acceptable for the initial implementation, but consider filtering conversations by message timestamps directly if precision becomes important.
Also applies to: 98-103
95-159: Performance consideration for large datasets.The current implementation iterates through all conversations and messages in Python. For users with many conversations or lengthy chat histories, this could become slow. Consider:
- Adding pagination or limiting the number of conversations processed.
- Moving aggregation to the database layer using Tortoise ORM's
annotate()andaggregate()for better scalability.- Adding caching for frequently requested time periods.
This is acceptable for the initial implementation.
engine/src/api/services/mcp_client.py (1)
27-31: Cache the Client instance to avoid overhead from repeated instantiation.Currently,
_get_client()creates a newClientinstance with a newStreamableHttpTransporton every call tolist_tools_raw()andcall_tool(). This is inefficient because:
- StreamableHttpTransport does not reuse connections across multiple instantiations (known limitation in the fastmcp library).
- Other parts of the codebase (tool_executor.py, integrations.py) cache MCPClient instances, establishing a pattern of reuse.
Cache the client as an instance variable initialized in
__init__, similar to how the codebase handles it elsewhere.ui/src/components/chat/ChatMessages.tsx (1)
50-55: Avoid per-renderfilterwork forassistantMessageCount(minor perf / rerender sensitivity).
With long chats,messages.filter(...)on every render + hover state updates can make this component more expensive than it needs to be; consideruseMemooffmessages/currentMessage(or compute once fromallMessages).ui/src/components/chat/TokenUsageDisplay.tsx (1)
27-46: Consolidate number formatting helpers (tiny duplication).
formatNumber()andformatNumberWithCommas()overlap; consider a single formatter that supports “compact” vs “full” modes.ui/src/components/ui/calendar.tsx (1)
175-211: Let day buttons honorbuttonVariant(theme consistency).
CalendarDayButtonhardcodesvariant="ghost", sobuttonVariantonly affects nav buttons. Consider threadingbuttonVariantthrough (or documenting the intentional difference).engine/src/api/services/conversation_persistence.py (1)
98-131: Exception handling silently swallows errors when fetching conversation.The
except Exceptionblock at line 109 silently ignores all errors when fetching the conversation (including database connectivity issues). Consider logging the error for debugging purposes.Suggested improvement
try: conversation = await Conversation.get(id=conversation_id) except Exception: + import logging + logging.getLogger(__name__).debug( + f"Failed to fetch conversation {conversation_id} for usage snapshot" + ) if finalize: self._clear_usage_buffer(conversation_id, run_id) return
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
engine/uv.lockis excluded by!**/*.lockui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (53)
deployment/install.shdeployment/install.yamldeployment/local.install.yamldeployment/mcp/Dockerfiledeployment/mcp/entrypoint.shdocs/architecture.mdengine/.env.exampleengine/pyproject.tomlengine/src/api/agent/graph.pyengine/src/api/agent/model_node.pyengine/src/api/agent/state.pyengine/src/api/config/settings.pyengine/src/api/endpoints/__init__.pyengine/src/api/endpoints/agent.pyengine/src/api/endpoints/analytics.pyengine/src/api/models/conversation.pyengine/src/api/services/conversation_persistence.pyengine/src/api/services/mcp_client.pyengine/src/api/services/tool_executor.pymcp/.env.examplemcp/README.mdmcp/main.pyui/.env.exampleui/design_system.mdui/package.jsonui/src/app/analytics/page.tsxui/src/components/CommandHistory.tsxui/src/components/analytics/Analytics.tsxui/src/components/analytics/AnimatedCounter.tsxui/src/components/analytics/CacheEfficiencyGauge.tsxui/src/components/analytics/CostTrendChart.tsxui/src/components/analytics/DatePickerWithRange.tsxui/src/components/analytics/MetricCard.tsxui/src/components/analytics/TimeRangeSelector.tsxui/src/components/analytics/TimetoFirstTokenAreaChart.tsxui/src/components/analytics/TokenUsageChart.tsxui/src/components/analytics/TotalResponseTimeAreaChart.tsxui/src/components/chat/ChatInput.tsxui/src/components/chat/ChatInterface.tsxui/src/components/chat/ChatMessages.tsxui/src/components/chat/TokenUsageDisplay.tsxui/src/components/chat/index.tsui/src/components/navbar/Navbar.tsxui/src/components/ui/calendar.tsxui/src/components/ui/markdown-components.tsxui/src/components/ui/popover.tsxui/src/components/ui/tooltip.tsxui/src/lib/api.tsui/src/lib/services/sseService.tsui/src/types/analytics.tsui/src/types/chat.tsui/src/types/events.tsui/tailwind.config.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- ui/src/components/analytics/TokenUsageChart.tsx
- ui/src/components/analytics/CostTrendChart.tsx
- ui/src/lib/api.ts
- ui/src/app/analytics/page.tsx
- ui/src/components/navbar/Navbar.tsx
- ui/src/components/ui/popover.tsx
- ui/src/components/analytics/Analytics.tsx
- ui/src/components/analytics/TotalResponseTimeAreaChart.tsx
- ui/src/components/analytics/CacheEfficiencyGauge.tsx
- ui/package.json
- engine/src/api/endpoints/init.py
- ui/src/types/analytics.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-10T15:42:06.735Z
Learnt from: nitishbhosale7
Repo: skyflo-ai/skyflo PR: 73
File: engine/src/api/endpoints/analytics.py:35-57
Timestamp: 2026-01-10T15:42:06.735Z
Learning: In the analytics metrics endpoint (engine/src/api/endpoints/analytics.py), the period_end calculation intentionally differs between modes: last_n_days uses datetime.now() for a rolling real-time window, while custom date ranges use datetime.max.time() for day-aligned boundaries. This is by design to support different use cases.
Applied to files:
engine/src/api/endpoints/analytics.py
🧬 Code graph analysis (18)
ui/src/components/analytics/MetricCard.tsx (1)
ui/src/components/analytics/AnimatedCounter.tsx (1)
AnimatedCounter(11-67)
ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx (2)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-138)ui/src/types/analytics.ts (1)
DailyMetrics(29-38)
ui/src/components/analytics/DatePickerWithRange.tsx (2)
ui/src/components/ui/popover.tsx (3)
Popover(33-33)PopoverTrigger(33-33)PopoverContent(33-33)ui/src/components/ui/calendar.tsx (1)
Calendar(213-213)
ui/src/components/ui/calendar.tsx (1)
ui/src/components/ui/button.tsx (2)
props(44-53)ButtonProps(37-41)
ui/src/components/CommandHistory.tsx (1)
ui/src/components/ui/tooltip.tsx (4)
TooltipProvider(42-42)Tooltip(42-42)TooltipTrigger(42-42)TooltipContent(42-42)
engine/src/api/endpoints/agent.py (1)
engine/src/api/services/conversation_persistence.py (6)
record_token_usage(34-52)apply_usage_snapshot(133-134)record_ttft(54-61)append_text_segment(160-204)record_ttr(63-70)finalize_usage_snapshot(136-137)
ui/src/components/chat/ChatMessages.tsx (3)
ui/src/components/ui/markdown-components.tsx (1)
markdownComponents(14-140)ui/src/components/chat/ToolVisualization.tsx (1)
ToolVisualization(108-414)ui/src/components/chat/TokenUsageDisplay.tsx (1)
TokenUsageDisplay(48-182)
engine/src/api/endpoints/analytics.py (2)
engine/src/api/models/conversation.py (3)
Conversation(12-33)MetricsAggregation(140-157)DailyMetrics(130-138)ui/src/types/analytics.ts (2)
MetricsAggregation(3-26)DailyMetrics(29-38)
engine/src/api/services/tool_executor.py (2)
engine/src/api/services/mcp_client.py (1)
MCPClient(13-117)engine/src/api/services/integrations.py (1)
_get_mcp_client(30-33)
engine/src/api/agent/graph.py (2)
engine/src/api/services/mcp_client.py (1)
MCPClient(13-117)engine/src/api/utils/helpers.py (1)
get_state_value(11-17)
deployment/install.yaml (1)
kubernetes-controller/controllers/skyfloai_controller.go (3)
r(282-301)r(303-350)r(109-127)
ui/src/lib/services/sseService.ts (2)
ui/src/types/chat.ts (1)
TokenUsage(93-100)ui/src/types/events.ts (2)
TokenUsageEvent(85-95)TTFTEvent(97-102)
engine/src/api/services/conversation_persistence.py (1)
engine/src/api/models/conversation.py (2)
Conversation(12-33)TokenUsageMetrics(57-66)
engine/src/api/models/conversation.py (1)
ui/src/types/analytics.ts (2)
DailyMetrics(29-38)MetricsAggregation(3-26)
ui/src/components/chat/ChatInput.tsx (2)
ui/src/types/chat.ts (1)
TokenUsage(93-100)ui/src/components/chat/TokenUsageDisplay.tsx (1)
TokenUsageDisplay(48-182)
ui/src/components/chat/ChatInterface.tsx (1)
ui/src/types/chat.ts (1)
TokenUsage(93-100)
engine/src/api/agent/model_node.py (2)
engine/src/api/endpoints/agent.py (1)
event_callback(126-258)engine/src/api/utils/helpers.py (1)
get_state_value(11-17)
engine/src/api/services/mcp_client.py (2)
engine/src/api/services/tool_executor.py (1)
list_tools(326-340)engine/src/api/services/integrations.py (1)
_get_mcp_client(30-33)
🪛 Biome (2.1.2)
ui/src/components/analytics/AnimatedCounter.tsx
[error] 35-35: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 42-42: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 47-47: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🪛 Ruff (0.14.10)
engine/src/api/endpoints/analytics.py
36-36: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
37-37: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
38-38: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
38-38: Do not perform function call fastapi_users.current_user in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (66)
ui/src/components/analytics/DatePickerWithRange.tsx (1)
69-77: Well-implemented date range validation logic.The logic correctly maintains a valid date range by clearing the end date when a new start date is selected that falls after the current end date. This prevents invalid ranges and provides good UX.
ui/src/components/analytics/TimeRangeSelector.tsx (1)
20-21: Verify responsive layout behavior on mobile devices.The outer container uses
max-lg:flex-colto stack elements vertically on smaller screens, but the innerdivat Line 21 wrapping the range buttons doesn't have responsive width adjustments. This might cause layout issues on mobile when the buttons need to fill the available width.Consider testing the component on mobile viewports to ensure the button groups display correctly, or add
max-lg:w-fullto the inner div if needed.ui/src/components/analytics/MetricCard.tsx (1)
16-21:variantswon’t animate unlessinitial/animate(or parent variants) are wired.If the parent doesn’t control variants, this will render with no entry animation.
Proposed fix
<motion.div + initial="hidden" + animate="visible" variants={{ hidden: { y: 20, opacity: 0 }, visible: { y: 0, opacity: 1 }, }} className="bg-navbar rounded-lg border border-slate-700/60 p-8 inline-block transition-colors group" >ui/.env.example (1)
3-3: LGTM! Version bump coordinated across the project.The version update to 0.4.0 is consistent with other components (engine, mcp, deployment scripts) and reflects the new analytics dashboard feature set introduced in this PR.
mcp/.env.example (1)
3-3: LGTM! Version bump is consistent with the project-wide 0.4.0 release.The MCP server version update aligns with the coordinated version bumps across all components for this release.
deployment/install.sh (2)
3-3: LGTM! Version update follows semantic versioning.The version increment from v0.3.2 to v0.4.0 appropriately reflects the addition of new analytics dashboard features and the MCP transport migration.
201-201: MCP port 8888 and /mcp path are consistently configured across all deployment files; no port migration from 8081 occurred.Port 8888 and the
/mcppath suffix are correctly and consistently applied throughout the deployment stack:
- MCP service:
port: 8888in K8s manifests- Environment configs:
MCP_SERVER_URL=http://skyflo-ai-mcp:8888/mcp- MCP runtime:
--port 8888in entrypoint.sh andEXPOSE 8888in DockerfileThe port 8081 references in deployment manifests are for the k8s-controller health probe (
--health-probe-bind-address=:8081), not MCP. There is no evidence of a prior port change from 8081 to 8888 for the MCP service.Likely an incorrect or invalid review comment.
docs/architecture.md (2)
26-26: LGTM! Documentation accurately reflects the FastMCP transport migration.The updated description correctly specifies the "FastMCP Streamable HTTP transport" and automatic tool discovery, aligning with the dependency migration from
mcptofastmcpin the codebase.
76-76: LGTM! New feature documentation aligns with PR objectives.The addition of "Token Usage & Metrics" feature documentation accurately reflects the new analytics dashboard functionality introduced in this PR, including real-time tracking of token consumption, TTFT (Time To First Token), and TTR (Total Response Time) latency metrics.
engine/pyproject.toml (1)
32-32: The fastmcp migration is complete; consider whether the version constraint should remain pinned at 2.12.3 or be updated to leverage newer versions.All old
mcpimports have been successfully replaced withfastmcpimports inmcp_client.py, and no lingering references to the old package exist in the codebase. The local variable names (mcp,self._mcp) inintegrations.pycorrectly refer toMCPClientinstances, not the old package.Note: The pinned version
2.12.3is 2 minor releases behind the current stable version2.14.2. Breaking changes were introduced in versions 2.13-2.14 related to protocol updates and MCP SDK compatibility. If staying on 2.12.3 is intentional for stability, consider documenting this; otherwise, evaluate whether a version range update is appropriate.mcp/README.md (1)
3-3: LGTM!Documentation updates are consistent with the transport migration from SSE to Streamable HTTP. The startup command example correctly reflects the new port (8888) and transport mode.
Also applies to: 18-18, 69-73
mcp/main.py (1)
27-32: LGTM!Clean simplification of the server startup. The removal of SSE transport option and hardcoding HTTP transport aligns with the migration to Streamable HTTP transport across the codebase.
ui/design_system.md (1)
1-81: LGTM!Comprehensive design system documentation that will help maintain UI consistency. The color palette, typography, spacing, and effects documentation aligns with the existing Tailwind configuration and provides a useful reference for developers.
engine/src/api/services/mcp_client.py (2)
49-74: LGTM!The
_parse_content_itemmethod robustly handles content normalization with appropriate fallbacks for non-text content and JSON parsing failures.
98-113: LGTM!The
call_toolmethod properly uses the new client, aggregates error states from all content items, and maintains backward compatibility with the inferred parameters logic.engine/src/api/models/conversation.py (2)
57-66: LGTM!Well-structured
TokenUsageMetricsmodel with sensible defaults for all fields, enabling optional usage across the codebase.
130-157: LGTM!The
DailyMetricsandMetricsAggregationmodels are well-defined and align with the corresponding TypeScript interfaces inui/src/types/analytics.ts. The use of Python'sdatetype forDailyMetrics.datewill serialize correctly to the string format expected by the UI.engine/src/api/endpoints/analytics.py (3)
35-38: Static analysis hints are false positives for FastAPI patterns.The B008 warnings about
Query()andDepends()in argument defaults are expected in FastAPI. This is the standard, idiomatic pattern for dependency injection and query parameter declaration in FastAPI endpoints.
44-66: LGTM!The period calculation logic correctly handles both custom date ranges and rolling last-N-days windows. Based on learnings, the intentional difference in
period_endcalculation (day-aligned vs real-time) is by design to support different use cases.
186-218: LGTM!Proper handling of edge cases with division guards for all average calculations. The
approval_acceptance_ratecorrectly returnsNonewhen there are no approvals or rejections, matching the schema'sOptional[float]type.ui/src/components/ui/tooltip.tsx (1)
8-42: LGTM!Clean refactoring to Radix UI Tooltip primitives. The implementation:
- Properly uses
Portalfor z-index layering.- Sets sensible defaults (
delayDuration=100,sideOffset=4).- Maintains styling consistency with the dark theme via Tailwind classes.
- Uses
forwardRefcorrectly for the content component.ui/src/components/chat/index.ts (1)
7-7: LGTM!The export follows the existing barrel pattern and makes TokenUsageDisplay publicly available for use in chat components.
deployment/mcp/Dockerfile (1)
77-77: LGTM!The EXPOSE port change aligns with the entrypoint.sh update and coordinated port migration to 8888.
engine/.env.example (2)
3-3: LGTM!Version bump to 0.4.0 is appropriate for this feature release introducing the analytics dashboard.
18-18: Verify mcp/main.py explicitly configures the/mcppath; ensure consistency between server and client.The MCP server currently relies on FastMCP's default HTTP path (
/mcp/with trailing slash), but no explicit path configuration is set inmcp/main.py. The.env.examplespecifies/mcpwithout a trailing slash, and client code strips trailing slashes before connecting, creating a potential path mismatch.Actions needed:
- Add explicit path configuration to
mcp/main.py:mcp.run(transport="http", path="/mcp")- Or update
.env.exampletoMCP_SERVER_URL=http://skyflo-ai-mcp:8888/mcp/and remove.rstrip("/")inmcp_client.pyGood: Clients correctly use
MCP_SERVER_URLfrom config rather than hardcoded URLs; no bypass URLs found.ui/tailwind.config.ts (1)
29-29: LGTM!The new primary-cyan color token follows the existing naming convention and supports the analytics dashboard theming requirements.
deployment/mcp/entrypoint.sh (1)
5-5: Transport migration from SSE to HTTP streaming is complete.Verification confirms:
- No SSE transport references remain in the codebase
- All MCP clients (integrations.py, tool_executor.py, agent/graph.py) use
StreamableHttpTransportfrom FastMCPMCP_SERVER_URLis correctly configured tohttp://127.0.0.1:8888/mcp- No hardcoded references to the old port for MCP server communication exist (the port 8081 references found are for the Kubernetes controller's health probe binding, which is unrelated)
The migration is complete and consistent across all client implementations.
ui/src/types/events.ts (2)
85-102: LGTM! Well-structured event types for analytics telemetry.The new
TokenUsageEventandTTFTEventinterfaces are correctly defined with appropriate field types and literals. The token usage event captures all necessary metrics (prompt, completion, total, and optional cached tokens) along with source attribution, and the TTFT event properly tracks latency with run context.
147-148: Event union correctly updated.The new event types are properly added to the discriminated union, maintaining type safety across the event system.
ui/src/components/ui/markdown-components.tsx (1)
73-73: LGTM! Styling enhancement for list markers.The addition of
marker:text-primary-cyanapplies consistent brand colors to unordered list markers in rendered markdown. This is a cosmetic improvement with no functional impact.engine/src/api/agent/state.py (1)
24-24: LGTM! Appropriate field addition for TTFT tracking.The
ttft_emittedboolean flag correctly supports tracking whether a time-to-first-token event has been emitted during the agent's execution flow. The default value ofFalseis appropriate for initial state.deployment/local.install.yaml (1)
993-993: Verify MCP port standardization across infrastructure.The MCP service port update from 8081 to 8888 in deployment/local.install.yaml (lines 993, 1036) should be verified across all infrastructure components including:
- Dockerfile EXPOSE directives
- Entrypoint scripts and container initialization logic
- Environment variable configurations (MCP_SERVER_URL settings)
- Any remaining stale references to port 8081 in deployment manifests
Run the verification script to ensure consistency across all infrastructure components before merging.
engine/src/api/services/tool_executor.py (2)
65-67: Confirm resource cleanup is not needed.The
close()method no longer closes the MCPClient when owned. Based on the MCPClient implementation using transient clients per operation, this appears correct. However, confirm that no cleanup is needed for the StreamableHttpTransport or any other resources.
45-49: MCPClient instantiation is correct; event callbacks are not passed to MCPClient.The MCPClient class never accepted event callback parameters—its
__init__takes no arguments beyondself. Event callbacks are properly managed at the ToolExecutor level via thesse_publishparameter and remain unaffected by this instantiation. All MCPClient usages in the codebase consistently instantiate without parameters, confirming the approach is correct.deployment/install.yaml (1)
992-992: LGTM - MCP port update is consistent.The containerPort change from 8081 to 8888 aligns with the MCP server migration to Streamable HTTP transport on port 8888.
ui/src/types/chat.ts (3)
45-45: LGTM - TokenUsage integration in ChatMessage.Adding optional
tokenUsageto ChatMessage enables token tracking per message, aligning with the analytics dashboard objectives.
84-84: LGTM - TokenUsage integration in ChatInputProps.Adding optional
tokenUsageto ChatInputProps enables token usage display in the chat input component.
93-100: LGTM - Well-defined TokenUsage interface.The interface correctly captures all token metrics mentioned in the PR objectives. The optional
ttftandttrfields appropriately reflect that latency metrics may not always be available.ui/src/components/CommandHistory.tsx (2)
12-12: LGTM - Correct TooltipProvider integration.The TooltipProvider wrapper is correctly added to provide context for the Radix-based tooltip components used throughout the component.
Also applies to: 62-62
122-140: LGTM - Tooltip implementations improve UX.The tooltip wrappers for action buttons (favorite, run, copy, delete) follow the correct Radix tooltip pattern and enhance usability by providing clear labels for icon-only buttons.
Also applies to: 148-160, 162-174, 176-188
ui/src/lib/services/sseService.ts (3)
7-10: LGTM - Imports for token analytics.The new imports for TokenUsageEvent, TTFTEvent, and TokenUsage enable the SSE service to handle analytics-related events.
27-28: LGTM - New callbacks for analytics events.The optional callbacks
onTokenUsageandonTTFTextend the service API while maintaining backward compatibility. The source parameter distinguishes between "turn_check" and "main" token usage tracking.
395-413: LGTM - Event handlers for token analytics.The handlers correctly map SSE events to callbacks:
token.usageevent →onTokenUsagecallback with TokenUsage datattftevent →onTTFTcallback with duration and run_idThe nullish coalescing operator on Line 402 safely defaults
cached_tokensto 0 when undefined.ui/src/components/chat/ChatInput.tsx (1)
130-137: TokenUsageDisplay placement looks fine; verify it doesn’t “stick” stale usage across turns.
Since it renders whenevertokenUsageexists and stays visible whenhasMessages, confirm the parent clears/updatestokenUsageper run to avoid showing old numbers after a new message starts.Also applies to: 229-236
engine/src/api/endpoints/agent.py (2)
138-147: Good: don’t leakcostover SSE fortoken.usage.
Maskingcostinpublish_payloadwhile still persisting it is the right tradeoff for chat streaming.
248-256: Verifydurationunits forcompleted→ttr_msconversion.
duration_ms = int(duration * 1000)assumes seconds; if upstream already emits ms, this will inflate values 1000×. Please confirm the event contract forcompleted.duration.ui/src/components/ui/calendar.tsx (1)
14-173: Please verify react-day-picker v9.13.0 component slot keys you’re overriding (Root,Chevron,DayButton,WeekNumber).
This wrapper relies on exact slot/component names andDayButtonexport behavior; confirm against the installedreact-day-picker@9.13.0typings/docs to avoid silent theming regressions on upgrade.engine/src/api/agent/graph.py (3)
67-73: LGTM - MCPClient initialization aligned with new constructor signature.The change from
MCPClient(event_callback=self.event_callback)toMCPClient()correctly matches the updated constructor inengine/src/api/services/mcp_client.pywhere__init__now takes no parameters.
129-134: LGTM - Entry node correctly initializes ttft_emitted state.The
ttft_emitted: Falseinitialization ensures TTFT tracking starts fresh for each workflow invocation, and theawaiting_approvalreset logic is correctly consolidated into a single return statement.
148-149: LGTM - ttft_emitted propagation from model node result.Correctly carries over the
ttft_emittedflag from the inner result toupdated_state, ensuring the TTFT emission state persists through downstream nodes.ui/src/components/chat/ChatInterface.tsx (7)
17-69: LGTM - Well-structured token usage utility functions.The utility functions correctly handle edge cases:
createEmptyUsageprovides consistent defaultsmapTokenUsagesafely coerces values and handles bothttft_ms/ttftvariantsaccumulateUsagecorrectly accumulates counts while preferring the latest ttft/ttr valueshasUsageMetricsproperly checks for any non-zero metric
88-105: LGTM - Live usage state management with ref synchronization.Good pattern using both state and ref to maintain synchronization between React state updates and immediate access needs. The
updateLiveUsagecallback correctly updates both.
593-620: LGTM - Token usage callback correctly filters by source and accumulates.The
onTokenUsagecallback properly:
- Filters to only process
"main"source events- Accumulates token counts additively
- Preserves existing ttft/ttr values
622-629: LGTM - TTFT callback updates both live usage and current message.The
onTTFTcallback correctly propagates the duration to both the live usage state and the current message's tokenUsage.
649-672: LGTM - Message hydration correctly maps token_usage to tokenUsage.The hydration logic properly:
- Creates a base message with
tokenUsagemapped from rawtoken_usage- Preserves segments for assistant messages
- Returns the appropriate message structure
979-994: Verify aggregatedUsage doesn't double-count current message during streaming.The logic accumulates from
currentMessage.tokenUsagewhen streaming with a current message, but also falls back toliveUsagewhen streaming without a current message. Ensure that whencurrentMessageexists and is streaming, itstokenUsagereflects the accumulated live data to avoid discrepancies.
996-998: LGTM - Footer usage display condition is appropriate.Only shows footer usage when streaming or at least one assistant message exists, and there are actual metrics to display.
engine/src/api/services/conversation_persistence.py (3)
34-52: LGTM - Token usage recording with defensive guards.The
max(x, 0)guards prevent negative values from corrupting the accumulated totals. The null-coalescing withor 0handles None inputs gracefully.
54-70: LGTM - TTFT and TTR recording methods.Simple and correct - only sets the value if
duration_msis not None, preserving any previously set value if called with None.
160-203: LGTM - append_text_segment extended with optional run_id for usage propagation.The implementation correctly:
- Preserves backward compatibility with optional
run_id- Only applies usage snapshot when
run_idis provided- Uses
TokenUsageMetricsfor consistent serializationengine/src/api/agent/model_node.py (6)
8-8: LGTM - Added litellm cost calculation imports.The
completion_costandcost_per_tokenfunctions are correctly imported for calculating token costs.
35-103: LGTM - decide_next_speaker extended with token usage telemetry.The function correctly:
- Accepts optional
event_callbackandconversation_idfor telemetry- Emits
token.usageevents withsource: "turn_check"to differentiate from main generation- Handles cost calculation failures gracefully with debug logging
- Uses a local
get_valhelper for safe attribute/dict access
165-170: LGTM - Stream options configured for usage tracking.Adding
stream_options: {"include_usage": True}enables token usage data in streaming responses, anddrop_params: Trueensures unsupported parameters are handled gracefully.
269-308: LGTM - Token usage emission with cost calculation for streaming.The implementation correctly:
- Extracts cached tokens from
prompt_tokens_detailsif available- Uses
cost_per_tokenfor streaming cost calculation (appropriate for per-token pricing)- Handles cost calculation failures gracefully
- Emits comprehensive usage event with all metrics
376-376: LGTM - Return value correctly includes ttft_emitted state.The function now returns a triple
(assistant_messages, tool_calls, ttft_emitted or new_ttft_emitted), correctly propagating whether TTFT was emitted in this or a previous call.
516-549: LGTM - ModelNode.call correctly handles the new triple return and passes telemetry parameters.The implementation:
- Extracts
start_timeandttft_emittedfrom state- Passes them to
run_model_turn- Updates state with
ttft_emitted: Truewhen TTFT was emitted- Passes
event_callbackandconversation_idtodecide_next_speaker
nitishbhosale7
force-pushed
the
#71/feat/analytics-dashboard-for-token-usage
branch
2 times, most recently
from
b0a289d to
6fec11a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @ui/src/components/analytics/AnimatedCounter.tsx:
- Around line 11-67: The component violates the Rules of Hooks by calling
useSpring, useEffect, and useTransform after early returns; move all React hooks
to the top level of AnimatedCounter so they are executed on every render: always
declare the spring (useSpring), transform (useTransform) and effect (useEffect)
before any conditional returns, then compute parsing/matching logic and
conditionally render a fallback span when parsing fails; ensure the effect
updates the spring target (count.set(parsedValue)) only when parsedValue is a
valid number and keep the rounded/string formatting logic inside the transform
callback (or guard its inputs) so hooks never run conditionally.
In @ui/src/components/analytics/TokenUsageChart.tsx:
- Around line 68-97: The Area components for cached_tokens, prompt_tokens, and
completion_tokens include invalid label props; remove the label attributes from
the Area elements (the three occurrences currently set on the Area with dataKey
"cached_tokens", the Area with dataKey "prompt_tokens", and the Area with
dataKey "completion_tokens") and rely on the existing name props for legend
labels instead.
🧹 Nitpick comments (8)
ui/package.json (1)
66-66: Reconsider TypeScript version pinning.The TypeScript version was changed from
^5to5.9.3(exact pin). Exact pinning can cause version conflicts when other dependencies require newer TypeScript versions or when different environments have different TS installations.Recommended: Use caret range for flexibility
- "typescript": "5.9.3" + "typescript": "^5.9.3"This allows patch and minor updates while maintaining compatibility.
ui/src/components/chat/TokenUsageDisplay.tsx (2)
27-35: Consider handling round thousands more elegantly.The
formatNumberfunction always displays one decimal place (e.g.,1.0Kinstead of1K). While not incorrect, displaying round numbers without decimals can improve readability.Optional: Conditionally display decimals
function formatNumber(num: number): string { if (num >= 1000000) { - return (num / 1000000).toFixed(1) + "M"; + const val = num / 1000000; + return (val % 1 === 0 ? val.toFixed(0) : val.toFixed(1)) + "M"; } if (num >= 1000) { - return (num / 1000).toFixed(1) + "K"; + const val = num / 1000; + return (val % 1 === 0 ? val.toFixed(0) : val.toFixed(1)) + "K"; } return num.toLocaleString(); }This displays
1Kfor 1000 and1.5Kfor 1500.
77-107: Non-null assertions are safe but could be more defensive.Lines 82, 87, 98, and 103 use non-null assertion operators (
!) on optional values. While these are safe because of thehasTTFTandhasTTRguards, TypeScript-wise it would be more explicit to use default values.Optional: Replace non-null assertions with defaults
{hasTTFT && ( <Tooltip> <TooltipTrigger asChild> <div className="flex items-center gap-1.5 hover:text-white/60 transition-colors cursor-default"> <MdTimer className="w-3.5 h-3.5" /> - <span className="tabular-nums">{formatTime(usage.ttft!)}</span> + <span className="tabular-nums">{formatTime(usage.ttft ?? 0)}</span> </div> </TooltipTrigger> <TooltipContent side="top"> <p className="text-white text-xs font-medium"> - Time to First Token: {formatTime(usage.ttft!)} + Time to First Token: {formatTime(usage.ttft ?? 0)} </p> </TooltipContent> </Tooltip> )}Apply the same pattern for
usage.ttron lines 98 and 103.ui/src/components/analytics/CacheEfficiencyGauge.tsx (1)
19-37: Remove dead code and clean up empty lines.Line 20 checks
percentage === null, but this condition can never be true sincepercentageis assigned fromMath.round()on line 12, which always returns a number. Additionally, lines 36-37 contain unnecessary empty lines.♻️ Proposed cleanup
const cacheEfficiencyColor = - percentage === null - ? "#94a3b8" - : percentage < 50 + percentage < 50 ? "#ff7373" : percentage < 60 ? "#ff9257" : percentage < 70 ? "#ffa55c" : percentage < 80 ? "#fcde60" : percentage < 90 ? "#c6ff6b" : "#73ffa6"; const COLORS = [cacheEfficiencyColor, "#333333"]; - - return (ui/src/components/analytics/DatePickerWithRange.tsx (1)
30-40: Move GlassWrapper outside the render function.Defining
GlassWrapperinside the component body (lines 31-40) causes it to be recreated on every render, which breaks React's reconciliation and optimization mechanisms. This can lead to performance issues and unexpected behavior.♻️ Proposed refactor
Move
GlassWrapperoutside the component definition:+// Wraps content in the liquid glass effect structure +const GlassWrapper = ({ children }: { children: React.ReactNode }) => ( + <div className="lg-wrapper"> + <div className="lg-effect" /> + <div className="lg-tint" /> + <div className="lg-shine" /> + <div className="relative z-10 p-2"> + {children} + </div> + </div> +) + export function DatePickerWithRange({ className, date, setDate, }: DatePickerWithRangeProps) { const [isStartOpen, setIsStartOpen] = React.useState(false) const [isEndOpen, setIsEndOpen] = React.useState(false) - // Wraps content in the liquid glass effect structure - const GlassWrapper = ({ children }: { children: React.ReactNode }) => ( - <div className="lg-wrapper"> - <div className="lg-effect" /> - <div className="lg-tint" /> - <div className="lg-shine" /> - <div className="relative z-10 p-2"> - {children} - </div> - </div> - ) - return (ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx (1)
39-46: Use consistent date formatting across charts.The X-axis date formatting on line 41 uses
String(value), which may produce inconsistent or unclear date displays. For consistency withTokenUsageChart.tsx(lines 43-46) and better UX, format dates asMM/DD.♻️ Proposed fix for consistent date formatting
<XAxis dataKey="date" - tickFormatter={(value) => String(value)} + tickFormatter={(value) => { + const date = new Date(value); + return `${date.getMonth() + 1}/${date.getDate()}`; + }} stroke="#9ca3af" tick={{ fill: "#9ca3af", fontSize: 12 }} tickLine={false} axisLine={false} />ui/src/components/ui/calendar.tsx (2)
184-186: Consider cleanup in the focus effect.The
useEffecthook focuses the ref whenmodifiers.focusedbecomes true, but if the component unmounts immediately after scheduling the focus, the ref might be stale. While this is unlikely to cause issues in practice, consider adding a check.♻️ Proposed defensive enhancement
const ref = React.useRef<HTMLButtonElement>(null) React.useEffect(() => { - if (modifiers.focused) ref.current?.focus() + if (modifiers.focused && ref.current) { + ref.current.focus() + } }, [modifiers.focused])
193-193: Specify locale fortoLocaleDateStringto ensure consistency.Using
toLocaleDateString()without a locale argument may produce inconsistent output across different user environments. For data attributes that might be used for testing or selection, consider using a standardized format like ISO date string.♻️ Proposed fix
- data-day={day.date.toLocaleDateString()} + data-day={day.date.toISOString().split('T')[0]}This ensures a consistent
YYYY-MM-DDformat regardless of user locale.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
engine/src/api/endpoints/__init__.pyengine/src/api/endpoints/analytics.pyengine/src/api/models/conversation.pyengine/src/api/services/conversation_persistence.pyui/design_system.mdui/package.jsonui/src/app/analytics/page.tsxui/src/components/analytics/Analytics.tsxui/src/components/analytics/AnimatedCounter.tsxui/src/components/analytics/CacheEfficiencyGauge.tsxui/src/components/analytics/CostTrendChart.tsxui/src/components/analytics/DatePickerWithRange.tsxui/src/components/analytics/MetricCard.tsxui/src/components/analytics/TimeRangeSelector.tsxui/src/components/analytics/TimetoFirstTokenAreaChart.tsxui/src/components/analytics/TokenUsageChart.tsxui/src/components/analytics/TotalResponseTimeAreaChart.tsxui/src/components/chat/TokenUsageDisplay.tsxui/src/components/navbar/Navbar.tsxui/src/components/ui/calendar.tsxui/src/components/ui/popover.tsxui/src/lib/api.tsui/src/types/analytics.tsui/tailwind.config.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- ui/src/components/navbar/Navbar.tsx
- ui/src/components/analytics/Analytics.tsx
- ui/src/components/ui/popover.tsx
- ui/tailwind.config.ts
- ui/src/lib/api.ts
- ui/src/components/analytics/TimeRangeSelector.tsx
- ui/src/components/analytics/TotalResponseTimeAreaChart.tsx
- engine/src/api/endpoints/init.py
- ui/src/components/analytics/CostTrendChart.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-10T15:42:06.735Z
Learnt from: nitishbhosale7
Repo: skyflo-ai/skyflo PR: 73
File: engine/src/api/endpoints/analytics.py:35-57
Timestamp: 2026-01-10T15:42:06.735Z
Learning: In the analytics metrics endpoint (engine/src/api/endpoints/analytics.py), the period_end calculation intentionally differs between modes: last_n_days uses datetime.now() for a rolling real-time window, while custom date ranges use datetime.max.time() for day-aligned boundaries. This is by design to support different use cases.
Applied to files:
engine/src/api/endpoints/analytics.py
🧬 Code graph analysis (8)
ui/src/app/analytics/page.tsx (2)
ui/src/components/navbar/Navbar.tsx (1)
Navbar(20-101)ui/src/components/analytics/Analytics.tsx (1)
Analytics(19-213)
ui/src/components/analytics/MetricCard.tsx (1)
ui/src/components/analytics/AnimatedCounter.tsx (1)
AnimatedCounter(11-67)
ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx (2)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-138)ui/src/types/analytics.ts (1)
DailyMetrics(29-38)
engine/src/api/models/conversation.py (1)
ui/src/types/analytics.ts (2)
DailyMetrics(29-38)MetricsAggregation(3-26)
engine/src/api/endpoints/analytics.py (1)
engine/src/api/models/conversation.py (3)
Conversation(12-33)MetricsAggregation(140-157)DailyMetrics(130-138)
ui/src/components/analytics/DatePickerWithRange.tsx (2)
ui/src/components/ui/popover.tsx (3)
Popover(33-33)PopoverTrigger(33-33)PopoverContent(33-33)ui/src/components/ui/calendar.tsx (1)
Calendar(213-213)
engine/src/api/services/conversation_persistence.py (1)
engine/src/api/models/conversation.py (2)
Conversation(12-33)TokenUsageMetrics(57-66)
ui/src/types/analytics.ts (1)
engine/src/api/models/conversation.py (2)
MetricsAggregation(140-157)DailyMetrics(130-138)
🪛 Biome (2.1.2)
ui/src/components/analytics/AnimatedCounter.tsx
[error] 35-35: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 42-42: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 47-47: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🪛 Ruff (0.14.10)
engine/src/api/endpoints/analytics.py
36-36: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
37-37: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
38-38: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
38-38: Do not perform function call fastapi_users.current_user in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (26)
ui/src/app/analytics/page.tsx (1)
1-15: LGTM! Clean page composition.The analytics page implementation is straightforward and follows Next.js conventions. The layout structure with Navbar and content area matches the existing pattern in the codebase.
ui/package.json (1)
15-15: New dependencies align with feature requirements.The added dependencies support the analytics dashboard features:
@radix-ui/react-popoverfor date picker popover UI@radix-ui/react-slotupdate (minor version) for Radix UI compatibilitylucide-reactfor additional icon optionsreact-day-pickerfor date range selectionAlso applies to: 17-17, 34-34, 40-40
ui/src/components/analytics/MetricCard.tsx (1)
1-37: LGTM! Well-structured metric card component.The MetricCard implementation is clean and follows best practices:
- Proper TypeScript typing
- Smooth framer-motion animations
- Animated counter for value display
- Optional color customization with sensible defaults
- Hover effects for visual polish
ui/design_system.md (1)
1-81: Excellent addition to project documentation.The design system documentation provides clear guidance on colors, typography, spacing, effects, and animations. This will help maintain consistency across the analytics dashboard and future UI development.
ui/src/components/chat/TokenUsageDisplay.tsx (2)
48-60: Good defensive programming with early returns.The component properly checks for data availability before rendering, preventing unnecessary DOM elements when there's no data to display.
62-181: Well-implemented token usage display with rich tooltips.The rendering logic is clean and user-friendly:
- Comprehensive tooltips for each metric
- Conditional rendering of cached tokens
- Smooth animation tied to visibility prop
- Consistent styling with hover effects
- Special highlighting for cached tokens (cyan color)
ui/src/components/ui/calendar.tsx (1)
14-25: LGTM: Calendar component structure is well-designed.The component properly extends DayPicker props, handles defaults appropriately, and provides a clean API with the custom
buttonVariantprop.engine/src/api/endpoints/analytics.py (7)
29-43: LGTM: Endpoint structure and authentication.The endpoint is properly configured with rate limiting and authentication. The authentication check correctly returns 401 when the user is missing.
44-66: LGTM: Date range calculation logic.The dual-mode period calculation (custom range vs. last_n_days) is well-structured and handles both use cases appropriately. Based on learnings, the intentional difference in
period_endbetween modes is by design.
167-185: LGTM: Daily breakdown construction fills all days.The loop correctly constructs a
DailyMetricsentry for every day in the period, ensuring the daily_breakdown is complete even for days with no activity. This is good for consistent UI rendering.
186-218: LGTM: Period aggregation with safe division.Period-level averages and derived metrics handle division by zero appropriately by checking denominators before division. The
cache_hit_rateandapproval_acceptance_ratecalculations are correct.
220-227: LGTM: Error handling is appropriate.The error handling re-raises
HTTPExceptionas-is and wraps unexpected exceptions with logging and a 500 response.
35-38: Static analysis warnings (B008) are false positives.Ruff flags the use of
QueryandDependsin function defaults. This is the standard FastAPI pattern and is not an issue. These warnings can be safely ignored.
95-159: Timestamp unit assumption is correct.The system consistently uses milliseconds for all timestamps. The
now_ms()function (defined inengine/src/api/utils/clock.py) explicitly returns "Wall-clock epoch time in whole milliseconds (UTC)" and is used throughout the codebase for timestamp creation. The division by 1000 on line 101 correctly converts milliseconds to seconds for date extraction.ui/src/types/analytics.ts (2)
3-26: LGTM: MetricsAggregation interface aligns with backend model.The interface correctly mirrors the backend
MetricsAggregationPydantic model, with appropriate type mappings (dates as strings, nullability preserved).
29-38: LGTM: DailyMetrics interface matches backend model.The
avg_ttft_msandavg_ttr_msfields are correctly marked as optional and nullable, matching the backend'sOptional[float]typing.engine/src/api/models/conversation.py (4)
57-67: LGTM: TokenUsageMetrics model is well-defined.All fields have sensible defaults, and the model structure supports both token counts and latency metrics appropriately.
130-138: LGTM: DailyMetrics model structure is correct.The model appropriately uses
datefor the date field and makes latency metrics optional, reflecting that not all days may have latency data.
140-158: LGTM: MetricsAggregation model is comprehensive.The model includes all necessary aggregations: totals, averages, derived metrics (cache_hit_rate, approval_acceptance_rate), and the daily_breakdown list. Field types and nullability are appropriate.
86-86: LGTM: MessageRead extension with token_usage.Adding the optional
token_usagefield toMessageReadallows messages to carry usage metrics when available, which aligns with the usage tracking functionality.engine/src/api/services/conversation_persistence.py (6)
34-52: LGTM: Defensive accumulation with non-negative enforcement.The
record_token_usagemethod correctly accumulates token counts and costs, usingmax(value or 0, 0)to ensure non-negative values. The conditional handling ofcached_tokensis appropriate since it's optional.
54-70: LGTM: Latency recording overwrites as expected.The
record_ttftandrecord_ttrmethods correctly overwrite the latency values rather than accumulating them, which is the expected behavior for time-to-first-token and total-response-time metrics.
72-96: LGTM: Snapshot and clear methods are well-structured.The
_snapshot_usagemethod creates a copy of the buffer data, and_clear_usage_bufferproperly removes the key from the dictionary. These methods support the finalize-on-demand pattern.
98-137: LGTM: Usage application logic is sound.The
_apply_usage_to_latest_assistantmethod correctly:
- Validates that the last message is an assistant message
- Serializes usage via
TokenUsageMetrics(...).model_dump()- Conditionally clears the buffer when
finalize=True- Handles errors gracefully
The public
apply_usage_snapshotandfinalize_usage_snapshotmethods provide a clear API.
198-202: LGTM: Run-aware usage propagation in append_text_segment.The updated
append_text_segmentmethod correctly applies the usage snapshot to the latest assistant message when arun_idis provided, aligning with the PR's run-aware usage tracking objectives.
9-9: No shared thread-safety concern exists for_usage_buffersacross requests.
ConversationPersistenceServiceis instantiated fresh for each request (seeengine/src/api/endpoints/agent.py:331, 426), meaning each request has its own_usage_buffersinstance. The dictionary is not shared across requests, so concurrent access from different requests is not a concern.If concurrent access within a single request is needed, that would require additional investigation, but the current design isolates buffers per request instance.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @engine/src/api/endpoints/analytics.py:
- Around line 69-74: The current logic builds a Conversation query using
updated_at but then filters messages by msg_date, causing total_conversations to
include conversations with no messages in the period; update the computation so
total_conversations only counts conversations that have at least one message in
the msg_date range (e.g., query Conversations by joining or prefiltering via the
Message model using msg_date__gte/msg_date__lte, or post-filter the fetched
conversations to count only those where any message satisfies the msg_date
range), and adjust any other counts that use the same pattern (reference
Conversation, Message, msg_date and total_conversations in your changes).
- Line 194: The cache_hit_rate calculation can exceed 1.0 when
total_cached_tokens > total_prompt_tokens; clamp the computed value into [0.0,
1.0] by replacing the expression that sets cache_hit_rate with a bounded result
(e.g., compute the ratio when total_prompt_tokens > 0, then apply min(max(ratio,
0.0), 1.0)) or alternatively enforce the invariant earlier in
ConversationPersistenceService.record_token_usage() so total_cached_tokens never
exceeds total_prompt_tokens; update references to total_cached_tokens and
total_prompt_tokens accordingly to ensure the displayed percentage cannot be
>100%.
In @engine/src/api/services/conversation_persistence.py:
- Around line 8-9: The _usage_buffers dict in ConversationPersistenceService can
grow without bound; change self._usage_buffers to an OrderedDict and add
configurable MAX_BUFFER_SIZE and BUFFER_TTL_SECONDS constants, store a
_created_at timestamp on each buffer entry when creating or updating it,
implement a private _evict_stale_buffers method that removes entries older than
BUFFER_TTL_SECONDS and trims oldest entries down to MAX_BUFFER_SIZE (LRU style),
and call _evict_stale_buffers whenever you create/update/remove entries (e.g.,
in the methods that add to _usage_buffers and in finalize_usage_snapshot) so
abandoned or errored conversation/run buffers are eventually evicted.
- Around line 107-112: The except block that swallows all exceptions when
calling Conversation.get(conversation_id) should at minimum log the caught
exception; change the handler to capture the exception (e.g., except Exception
as e) and call the component logger with the error and stack (for example
self.logger.error or logging.getLogger(__name__).error) including exc_info=True,
then keep the existing finalize -> self._clear_usage_buffer(conversation_id,
run_id) and return; reference the Conversation.get call and the
_clear_usage_buffer method in conversation_persistence.py so the error is
recorded before returning.
In @ui/src/components/analytics/CacheEfficiencyGauge.tsx:
- Around line 19-32: The conditional branch checking `percentage === null` in
the `cacheEfficiencyColor` assignment is dead code because `percentage` is
computed with `Math.round(safeHitRate * 100)` and is always a number; remove
that unreachable null check and simplify the color selection (e.g., convert the
chained ternaries in `cacheEfficiencyColor` to only numeric range checks), or
replace the whole chain with a small lookup/helper that maps numeric
`percentage` ranges to color strings; update any related logic that might have
assumed a null state so `percentage` is always treated as a number.
- Around line 69-71: The CSS class text-text-secondary used in
CacheEfficiencyGauge.tsx is not defined in Tailwind; open ui/tailwind.config.ts
and add a color entry under theme.extend.colors (e.g., "text-secondary":
"<hex-or-rgb-value>" or better add "secondary": { DEFAULT: "<value>" } so
Tailwind generates text-secondary) to ensure the text-text-secondary utility
resolves; save and rebuild to verify the Cache Hit Rate label picks up the new
color.
🧹 Nitpick comments (15)
ui/package.json (1)
34-34: Consider consolidating icon libraries.The project already uses
react-icons(line 44). Addinglucide-reactintroduces a second icon library, which increases bundle size. Consider using icons fromreact-icons(which includes Lucide icons viareact-icons/lu) to avoid redundancy.ui/design_system.md (1)
1-81: Good documentation addition.This design system documentation provides valuable reference for maintaining visual consistency across the analytics dashboard and future UI work. The color tokens align correctly with
tailwind.config.ts.Consider adding:
- Usage examples for the Liquid Glass utilities
- Guidelines on when to use
react-iconsvs other icon sourcesui/src/components/analytics/TimeRangeSelector.tsx (1)
11-53: LGTM! Clean implementation of time range selector.The component correctly handles both preset ranges and custom selection with appropriate styling for active/inactive states.
Minor suggestions for cleanup:
- Line 39: The
key={"custom"}attribute is unnecessary since this button is not rendered in a loop.- Lines 16-17: Trailing blank line inside the array could be removed for consistency.
🧹 Optional cleanup
const ranges: { label: string; value: number | "custom" }[] = [ { label: "7D", value: 7 }, { label: "30D", value: 30 }, { label: "90D", value: 90 }, - ];<Button - key={"custom"} variant={selectedRange === "custom" ? "default" : "ghost"}ui/src/components/analytics/CostTrendChart.tsx (1)
61-70: Minor cleanup: Remove unnecessary props on Area.The
stackIdprop is only needed when stacking multiple areas; for a single series it's unnecessary. Similarly,label="Cost"appears redundant sincename="Cost"already provides the legend label.Suggested cleanup
<Area type="monotone" dataKey="cost" name="Cost" - label="Cost" stroke="#10b981" - stackId="1" fill="url(#colorCost)" animationDuration={1500} />ui/src/components/analytics/AnimatedCounter.tsx (1)
3-3: Remove unused import.
useRefis imported but not used in this component.Suggested fix
-import { useEffect, useRef } from "react"; +import { useEffect } from "react";ui/src/lib/api.ts (1)
83-97: Clarify parameter precedence when conflicting options are provided.If callers pass
{ lastNDays: 7, startDate: someDate, endDate: someDate }, all three parameters are sent to the backend. Consider either:
- Documenting expected backend behavior when both are provided
- Adding client-side precedence (e.g., date range takes priority over lastNDays)
Option: Prioritize date range over lastNDays
} else { const params = []; - if (options.lastNDays) params.push(`last_n_days=${options.lastNDays}`); - if (options.startDate) params.push(`start_date=${options.startDate.toISOString().split("T")[0]}`); - if (options.endDate) params.push(`end_date=${options.endDate.toISOString().split("T")[0]}`); + // Date range takes precedence over lastNDays + if (options.startDate && options.endDate) { + params.push(`start_date=${options.startDate.toISOString().split("T")[0]}`); + params.push(`end_date=${options.endDate.toISOString().split("T")[0]}`); + } else if (options.lastNDays) { + params.push(`last_n_days=${options.lastNDays}`); + } queryParams = params.join("&"); }ui/src/components/analytics/Analytics.tsx (2)
170-175: Redundant calculation:avg_cost_per_conversationis already provided by the API.The
MetricsAggregationresponse includesavg_cost_per_conversation. Recalculating it here could lead to inconsistencies if the backend logic differs.♻️ Suggested fix
<MetricCard title="Average Conversation Cost" - value={displayData.total_conversations > 0 ? `$${(displayData.total_cost / displayData.total_conversations).toFixed(2)}` : "$0.00"} + value={`$${displayData.avg_cost_per_conversation.toFixed(2)}`} icon={<MdOutlineMoney />} color="text-purple-400" />
92-105: Consider extracting color threshold logic to a helper function.The deeply nested ternary for
approvalAcceptanceRateColoris functional but harder to read and maintain.♻️ Suggested refactor
const getApprovalRateColor = (percentage: number | null): string => { if (percentage === null) return "text-[#94a3b8]"; if (percentage < 50) return "text-[#ff7373]"; if (percentage < 60) return "text-[#ff9257]"; if (percentage < 70) return "text-[#ffa55c]"; if (percentage < 80) return "text-[#fcde60]"; if (percentage < 90) return "text-[#c6ff6b]"; return "text-[#73ffa6]"; }; const approvalAcceptanceRateColor = getApprovalRateColor(percentage);ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx (2)
22-26: Unusedavg_ttr_msnormalization.The
avg_ttr_msfield is normalized but never used in this chart (onlyavg_ttft_msis rendered). This appears to be dead code.♻️ Suggested fix
const chartData = data.map((d) => ({ ...d, avg_ttft_ms: d.avg_ttft_ms || 0, - avg_ttr_ms: d.avg_ttr_ms || 0, }));
39-46: Inconsistent date formatting compared to other charts.
TokenUsageChartformats dates asMM/DD, but this chart usesString(value)which displays the raw date string. Consider aligning the formatting for consistency.♻️ Suggested fix
<XAxis dataKey="date" - tickFormatter={(value) => String(value)} + tickFormatter={(value) => { + const date = new Date(value); + return `${date.getMonth() + 1}/${date.getDate()}`; + }} stroke="#9ca3af" tick={{ fill: "#9ca3af", fontSize: 12 }} tickLine={false} axisLine={false} />ui/src/components/analytics/DatePickerWithRange.tsx (1)
78-84: Potentially confusing date picker constraint logic.The start date picker disables dates after the current end date (line 80), but lines 71-72 already handle this case by clearing the end date when a later start is selected. The disabled constraint prevents users from selecting a new start date that would trigger the clear behavior.
Consider removing line 80 to allow users to freely select start dates, relying on the existing logic to clear the end date when needed.
♻️ Suggested fix
disabled={(day) => { - // Disable if after end date - if (date?.to && day > date.to) return true; // Disable if future date if (day > new Date()) return true; return false; }}engine/src/api/endpoints/analytics.py (1)
222-227: Consider sanitizing error details in production.The error detail includes
str(e)which could expose internal implementation details. For production, consider using a generic message while logging the full error.♻️ Suggested fix
except Exception as e: logger.exception(f"Error getting metrics: {str(e)}") raise HTTPException( status_code=500, - detail=f"Error getting metrics: {str(e)}", + detail="An error occurred while retrieving metrics", ) from eui/src/components/analytics/TotalResponseTimeAreaChart.tsx (3)
64-81: RedundantLineandAreacomponents with duplicate legend entries.Both
LineandArearenderavg_ttr_mswith the samename, causing duplicate legend entries. TheAreacomponent already provides the stroke and fill, making theLineredundant.♻️ Suggested fix - remove redundant Line
- <Line - type="monotone" - dataKey="avg_ttr_ms" - name="TTR (Total Response Time)" - stroke="#ec4899" - strokeWidth={2} - dot={false} - activeDot={{ r: 4 }} - animationDuration={1500} - /> <Area type="monotone" dataKey="avg_ttr_ms" stroke="#ec4899" fill="url(#colorTTR)" name="TTR (Total Response Time)" animationDuration={1500} />
23-27: Unusedavg_ttft_msnormalization.Similar to the inverse issue in
TimetoFirstTokenAreaChart, this chart normalizesavg_ttft_msbut only rendersavg_ttr_ms.♻️ Suggested fix
const chartData = data.map((d) => ({ ...d, - avg_ttft_ms: d.avg_ttft_ms || 0, avg_ttr_ms: d.avg_ttr_ms || 0, }));
39-46: Same date formatting inconsistency as other charts.Apply the same
MM/DDformat asTokenUsageChartfor visual consistency across the dashboard.♻️ Suggested fix
<XAxis dataKey="date" - tickFormatter={(value) => String(value)} + tickFormatter={(value) => { + const date = new Date(value); + return `${date.getMonth() + 1}/${date.getDate()}`; + }} stroke="#9ca3af" tick={{ fill: "#9ca3af", fontSize: 12 }} tickLine={false} axisLine={false} />
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (23)
engine/src/api/endpoints/__init__.pyengine/src/api/endpoints/analytics.pyengine/src/api/models/conversation.pyengine/src/api/services/conversation_persistence.pyui/design_system.mdui/package.jsonui/src/app/analytics/page.tsxui/src/components/analytics/Analytics.tsxui/src/components/analytics/AnimatedCounter.tsxui/src/components/analytics/CacheEfficiencyGauge.tsxui/src/components/analytics/CostTrendChart.tsxui/src/components/analytics/DatePickerWithRange.tsxui/src/components/analytics/MetricCard.tsxui/src/components/analytics/TimeRangeSelector.tsxui/src/components/analytics/TimetoFirstTokenAreaChart.tsxui/src/components/analytics/TokenUsageChart.tsxui/src/components/analytics/TotalResponseTimeAreaChart.tsxui/src/components/navbar/Navbar.tsxui/src/components/ui/calendar.tsxui/src/components/ui/popover.tsxui/src/lib/api.tsui/src/types/analytics.tsui/tailwind.config.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-10T15:42:06.735Z
Learnt from: nitishbhosale7
Repo: skyflo-ai/skyflo PR: 73
File: engine/src/api/endpoints/analytics.py:35-57
Timestamp: 2026-01-10T15:42:06.735Z
Learning: In the analytics metrics endpoint (engine/src/api/endpoints/analytics.py), the period_end calculation intentionally differs between modes: last_n_days uses datetime.now() for a rolling real-time window, while custom date ranges use datetime.max.time() for day-aligned boundaries. This is by design to support different use cases.
Applied to files:
engine/src/api/endpoints/analytics.py
📚 Learning: 2025-12-30T03:29:03.071Z
Learnt from: KaranJagtiani
Repo: skyflo-ai/skyflo PR: 0
File: :0-0
Timestamp: 2025-12-30T03:29:03.071Z
Learning: Enforce the "No Redundant Comments Rule" in code reviews: Flag any comments that are self-explanatory or simply restate what the code already expresses. Comments should only exist if they explain *why* something is done, not *what* is being done when the code itself is clear.
Applied to files:
ui/src/components/analytics/Analytics.tsx
🧬 Code graph analysis (13)
ui/src/components/analytics/CostTrendChart.tsx (2)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-138)ui/src/types/analytics.ts (1)
DailyMetrics(29-38)
engine/src/api/endpoints/analytics.py (2)
engine/src/api/models/conversation.py (3)
Conversation(12-33)MetricsAggregation(140-157)DailyMetrics(130-138)ui/src/types/analytics.ts (2)
MetricsAggregation(3-26)DailyMetrics(29-38)
ui/src/app/analytics/page.tsx (2)
ui/src/components/navbar/Navbar.tsx (1)
Navbar(20-101)ui/src/components/analytics/Analytics.tsx (1)
Analytics(19-213)
ui/src/components/analytics/MetricCard.tsx (1)
ui/src/components/analytics/AnimatedCounter.tsx (1)
AnimatedCounter(11-73)
ui/src/types/analytics.ts (1)
engine/src/api/models/conversation.py (2)
MetricsAggregation(140-157)DailyMetrics(130-138)
ui/src/lib/api.ts (2)
engine/src/api/models/conversation.py (1)
MetricsAggregation(140-157)ui/src/types/analytics.ts (1)
MetricsAggregation(3-26)
ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx (2)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-138)ui/src/types/analytics.ts (1)
DailyMetrics(29-38)
ui/src/components/analytics/Analytics.tsx (3)
engine/src/api/models/conversation.py (1)
MetricsAggregation(140-157)ui/src/types/analytics.ts (1)
MetricsAggregation(3-26)ui/src/lib/api.ts (1)
getMetrics(83-116)
ui/src/components/analytics/DatePickerWithRange.tsx (2)
ui/src/components/ui/popover.tsx (3)
Popover(33-33)PopoverTrigger(33-33)PopoverContent(33-33)ui/src/components/ui/calendar.tsx (1)
Calendar(213-213)
ui/src/components/analytics/TokenUsageChart.tsx (2)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-138)ui/src/types/analytics.ts (1)
DailyMetrics(29-38)
ui/src/components/analytics/TotalResponseTimeAreaChart.tsx (2)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-138)ui/src/types/analytics.ts (1)
DailyMetrics(29-38)
engine/src/api/models/conversation.py (1)
ui/src/types/analytics.ts (2)
DailyMetrics(29-38)MetricsAggregation(3-26)
engine/src/api/services/conversation_persistence.py (1)
engine/src/api/models/conversation.py (2)
Conversation(12-33)TokenUsageMetrics(57-66)
🪛 Biome (2.1.2)
ui/src/components/analytics/AnimatedCounter.tsx
[error] 48-48: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 53-53: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🪛 Ruff (0.14.10)
engine/src/api/endpoints/analytics.py
36-36: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
37-37: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
38-38: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
38-38: Do not perform function call fastapi_users.current_user in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (21)
ui/tailwind.config.ts (1)
29-29: LGTM!The new
primary-cyancolor token is properly defined and aligns with the design system documentation. The naming convention is consistent with standalone tokens in the config.ui/src/components/navbar/Navbar.tsx (1)
70-75: LGTM!The Analytics navigation item follows the established pattern consistently, using the same props and positioning logic as other nav items. The route
/analyticsaligns with the new analytics page in this PR.ui/package.json (1)
66-66: No changes needed. TypeScript version 5.9.3 exists on npm and is the current latest stable release.ui/src/components/ui/calendar.tsx (2)
14-173: LGTM! Well-structured calendar component with comprehensive theming.The Calendar component properly integrates with react-day-picker v9, providing:
- Comprehensive classNames configuration for consistent theming
- Custom component slots (Root, Chevron, DayButton, WeekNumber)
- RTL support via CSS selectors
- Proper prop forwarding and merging
175-211: LGTM! CalendarDayButton with proper focus management.The focus handling via
useRefanduseEffectcorrectly synchronizes the focused modifier with DOM focus state. The data attributes provide a clean API for styling different day states.engine/src/api/models/conversation.py (2)
57-66: LGTM! Well-defined TokenUsageMetrics model.The model provides sensible defaults and appropriate types for token/latency tracking. The use of
Optional[int]for latency metrics correctly handles cases where timing data may not be available.
130-157: Verify:conversation_countfield may be missing from DailyMetrics.Per the PR objectives,
DailyMetricsshould include aconversation_countfield for daily conversation tracking. The current implementation omits this field, which also appears absent from the UI type definition.If this is intentional (e.g., decided against during implementation), please confirm. Otherwise, consider adding the field:
📝 Suggested addition if needed
class DailyMetrics(BaseModel): date: date cost: float prompt_tokens: int completion_tokens: int cached_tokens: int total_tokens: int avg_ttft_ms: Optional[float] avg_ttr_ms: Optional[float] + conversation_count: int = 0ui/src/components/ui/popover.tsx (1)
1-33: LGTM! Standard Radix UI Popover wrapper.This follows the established shadcn/ui pattern with:
- Proper forwardRef implementation
- Portal wrapping for correct stacking context
- Comprehensive animation classes for all positioning scenarios
- Sensible defaults for alignment and offset
ui/src/app/analytics/page.tsx (1)
1-15: LGTM! Clean page composition following existing patterns.The page correctly:
- Uses client-side rendering (required for the stateful Analytics component)
- Maintains consistent layout structure with Navbar + content area
- Handles overflow appropriately for the scrollable analytics dashboard
This aligns with the Navbar integration shown in the relevant snippets where
/analyticsis registered as a route.engine/src/api/endpoints/__init__.py (1)
9-9: LGTM!The analytics router import and registration follow the existing pattern used by other routers in this file.
Also applies to: 19-19
ui/src/components/analytics/CostTrendChart.tsx (1)
21-77: LGTM!The chart implementation correctly uses Recharts with appropriate dark theme styling, gradient fills, and responsive container. The date formatting and tooltip configuration are well done.
ui/src/components/analytics/MetricCard.tsx (1)
14-37: LGTM on component structure.Clean implementation with proper TypeScript props interface, appropriate styling, and good use of AnimatedCounter for the value display.
ui/src/lib/api.ts (1)
99-116: LGTM on the API implementation.Good use of
cache: "no-store"for real-time analytics data, proper error handling with informative error messages, and correct endpoint construction.ui/src/components/analytics/TokenUsageChart.tsx (1)
21-99: LGTM! Well-structured stacked area chart implementation.The chart correctly uses
stackId="1"for stacking, defines gradient fills for visual distinction, and handles axis formatting appropriately. The component follows Recharts conventions and integrates well with theDailyMetricstype.ui/src/components/analytics/Analytics.tsx (1)
137-204: Dashboard layout and data rendering look good.The responsive grid layout, metric cards, and chart composition are well-structured. Loading and empty states are properly handled.
ui/src/components/analytics/DatePickerWithRange.tsx (1)
42-131: Date picker implementation looks solid.The two-popover approach for start/end dates with proper state management and validation is well-implemented. The glass wrapper styling and disabled date logic for future dates work correctly.
engine/src/api/endpoints/analytics.py (1)
35-38: Static analysis B008 warnings are false positives for FastAPI.Using
Query()andDepends()in function parameter defaults is the standard FastAPI pattern for dependency injection. These warnings can be safely ignored.ui/src/types/analytics.ts (1)
3-26: Type definitions align well with backend models.The interfaces correctly mirror the Pydantic models from
engine/src/api/models/conversation.py, with appropriate handling of optional fields (avg_ttft_ms,avg_ttr_ms,approval_acceptance_rateasnumber | null).Note: The issue requirements mentioned
conversation_countinDailyMetrics, but it's absent from both the backend model and this frontend type. Verify if this was intentionally deferred.Also applies to: 29-38
engine/src/api/services/conversation_persistence.py (3)
34-70: LGTM on token/timing recording logic.The accumulation pattern for tokens/cost and overwrite pattern for timing metrics are appropriate. The defensive
max(x or 0, 0)guards against negative values effectively.Note: If multiple TTFT/TTR events occur for the same run, only the last value is retained. Verify this aligns with expected usage patterns.
72-96: LGTM!Snapshot creates an independent copy of buffer state, and clear handles missing keys safely.
114-128: These methods appear to be unused dead code and are never invoked in the application.The
apply_usage_snapshot()andfinalize_usage_snapshot()methods are not called anywhere in the codebase, making the race condition concern in their implementation moot. If these methods are intended to be used in the future, ensure they include proper concurrency controls before activating them.Likely an incorrect or invalid review comment.
KaranJagtiani
left a comment
KaranJagtiani
left a comment
There was a problem hiding this comment.
Commit message should be this: "feat: add token metrics analytics dashboard page"
nitishbhosale7
force-pushed
the
#71/feat/analytics-dashboard-for-token-usage
branch
from
7cdf2f7 to
1da946e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ui/src/components/analytics/TotalResponseTimeAreaChart.tsx`:
- Around line 64-82: The legend shows duplicate entries because both the Line
and Area elements in TotalResponseTimeAreaChart use name="TTR (Total Response
Time)"; update the Area element (which shares dataKey="avg_ttr_ms") to not
appear in the legend by setting legendType="none" on the Area (or alternatively
remove the Area's name prop) while keeping the Line's name prop so the legend
shows a single TTR entry.
♻️ Duplicate comments (5)
ui/src/components/analytics/AnimatedCounter.tsx (1)
14-37: Rules of Hooks violation remains unresolved.This issue was raised in previous reviews but the code still violates React's Rules of Hooks. Hooks at lines 28, 34, and 37 (
useSpring,useEffect,useTransform) are called after early returns at lines 14-16 and 24-26.If
valuechanges from numeric to non-numeric (or vice versa) between renders, React will encounter a different hook call count, causing state corruption or runtime errors like "Rendered fewer hooks than expected."The fix requires moving all hooks to the top level before any conditional returns:
🐛 Proposed fix
export function AnimatedCounter({ value, className }: AnimatedCounterProps) { const match = value.match(/^([^0-9\.-]*)([\d,\.]+)([^0-9\.]*)$/); + const prefix = match?.[1] ?? ""; + const numericPart = match?.[2] ?? ""; + const suffix = match?.[3] ?? ""; + const parsedValue = parseFloat(numericPart.replace(/,/g, "")); + const isValidNumber = match !== null && !isNaN(parsedValue); - if (!match) { - return <span className={className}>{value}</span>; - } - - const prefix = match[1]; - const numericPart = match[2]; - const suffix = match[3]; - - const parsedValue = parseFloat(numericPart.replace(/,/g, "")); - - if (isNaN(parsedValue)) { - return <span className={className}>{value}</span>; - } - const count = useSpring(0, { stiffness: 100, damping: 20, duration: 0.5 }); useEffect(() => { - count.set(parsedValue); - }, [parsedValue, count]); + if (isValidNumber) { + count.set(parsedValue); + } + }, [parsedValue, count, isValidNumber]); + const rounded = useTransform(count, (latest) => { + if (!isValidNumber) return value; const hasDecimal = numericPart.includes("."); const decimalPlaces = hasDecimal ? numericPart.split(".")[1].length : 0; - let formattedNumber = latest.toFixed(decimalPlaces); - const parts = formattedNumber.split("."); parts[0] = parts[0].replace(/\B(?=(\d{3})+(?!\d))/g, ","); formattedNumber = parts.join("."); - return `${prefix}${formattedNumber}${suffix}`; }); + if (!isValidNumber) { + return <span className={className}>{value}</span>; + } + return ( <motion.span className={className}>{rounded}</motion.span> ); }ui/src/components/analytics/CostTrendChart.tsx (3)
51-60: Same timezone risk in Tooltip labelFormatter.The
new Date(label)call has the same UTC parsing issue. Use the date string directly.Proposed fix
- labelFormatter={(label) => new Date(label).toLocaleDateString()} + labelFormatter={(label) => String(label)}
61-70: Invalidlabelprop on Area component.The
label="Cost"prop is invalid—RechartsAreaacceptsboolean,object,ReactElement, orfunctionforlabel, not strings. This prop should be removed. ThestackId="1"is also redundant for a single Area.Proposed fix
<Area type="monotone" dataKey="cost" name="Cost" - label="Cost" stroke="#10b981" - stackId="1" fill="url(`#colorCost`)" animationDuration={1500} />
33-43: Timezone date-shift risk in XAxis tickFormatter.
new Date(value)parses ISO date strings (e.g., "2024-01-15") as UTC midnight, which can shift to the previous day when displayed in negative UTC offset timezones. Parse the date string directly to avoid this issue.Proposed fix
<XAxis dataKey="date" tickFormatter={(value) => { - const date = new Date(value); - return `${date.getMonth() + 1}/${date.getDate()}`; + const [, mm, dd] = String(value).split("-"); + return `${Number(mm)}/${Number(dd)}`; }}engine/src/api/endpoints/analytics.py (1)
194-194: Add bounds validation tocache_hit_rate.If upstream data has
cached_tokens > prompt_tokens(malformed data), the rate could exceed 1.0, confusing users when displayed as a percentage. Add a guard:- cache_hit_rate = (total_cached_tokens / total_prompt_tokens) if total_prompt_tokens > 0 else 0.0 + cache_hit_rate = min((total_cached_tokens / total_prompt_tokens), 1.0) if total_prompt_tokens > 0 else 0.0
🧹 Nitpick comments (5)
ui/src/components/analytics/TotalResponseTimeAreaChart.tsx (1)
23-27: Remove unusedavg_ttft_msnormalization.Line 25 normalizes
avg_ttft_msbut the chart only rendersavg_ttr_ms. This is dead code.Proposed fix
const chartData = data.map((d) => ({ ...d, - avg_ttft_ms: d.avg_ttft_ms || 0, avg_ttr_ms: d.avg_ttr_ms || 0, }));engine/src/api/endpoints/analytics.py (3)
35-42: Consider using non-optional authentication for cleaner code.Using
optional=Truethen manually checking and raising 401 adds boilerplate. If authentication is always required, usecurrent_user(optional=False)which auto-raises 401:- user=Depends(fastapi_users.current_user(optional=True)), + user=Depends(fastapi_users.current_user()), ) -> MetricsAggregation: try: - if not user: - raise HTTPException(status_code=401, detail="Authentication required for metrics")However, if you prefer custom error messages, the current approach is acceptable.
46-50: Add backend validation forstart_date > end_date.While frontend validation exists, backend validation provides defense-in-depth. If clients bypass the frontend (e.g., direct API calls), invalid date ranges will silently return empty results.
if start_date: # use custom range s_date = start_date # if end_date is provided, use it, otherwise valid until today e_date = end_date if end_date else date.today() + + if s_date > e_date: + raise HTTPException( + status_code=400, + detail="start_date cannot be after end_date" + )
59-60: Remove redundant fallback.
last_n_daysalready defaults to 30 viaQuery(default=30, ...), so the fallback is unnecessary.else: # use last_n_days - days = last_n_days if last_n_days else 30 + days = last_n_daysui/package.json (1)
34-34: Consider deduplicating icon libraries.The project already has
react-icons(line 44). Addinglucide-reactintroduces a second icon library. Consider using one consistently to reduce bundle size, or verify both are necessary for distinct icon sets.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
engine/src/api/endpoints/__init__.pyengine/src/api/endpoints/analytics.pyengine/src/api/models/conversation.pyui/design_system.mdui/package.jsonui/src/app/analytics/page.tsxui/src/components/analytics/Analytics.tsxui/src/components/analytics/AnimatedCounter.tsxui/src/components/analytics/CacheEfficiencyGauge.tsxui/src/components/analytics/CostTrendChart.tsxui/src/components/analytics/DatePickerWithRange.tsxui/src/components/analytics/MetricCard.tsxui/src/components/analytics/TimeRangeSelector.tsxui/src/components/analytics/TimetoFirstTokenAreaChart.tsxui/src/components/analytics/TokenUsageChart.tsxui/src/components/analytics/TotalResponseTimeAreaChart.tsxui/src/components/navbar/Navbar.tsxui/src/components/ui/calendar.tsxui/src/components/ui/popover.tsxui/src/lib/api.tsui/src/types/analytics.tsui/tailwind.config.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- ui/src/app/analytics/page.tsx
- ui/src/components/analytics/Analytics.tsx
- engine/src/api/endpoints/init.py
- ui/src/components/analytics/DatePickerWithRange.tsx
- ui/src/types/analytics.ts
- ui/src/components/analytics/MetricCard.tsx
- ui/src/components/ui/popover.tsx
- ui/src/components/analytics/TokenUsageChart.tsx
- ui/src/components/ui/calendar.tsx
- ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx
- ui/tailwind.config.ts
- ui/src/components/analytics/TimeRangeSelector.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-10T15:42:06.735Z
Learnt from: nitishbhosale7
Repo: skyflo-ai/skyflo PR: 73
File: engine/src/api/endpoints/analytics.py:35-57
Timestamp: 2026-01-10T15:42:06.735Z
Learning: In the analytics metrics endpoint (engine/src/api/endpoints/analytics.py), the period_end calculation intentionally differs between modes: last_n_days uses datetime.now() for a rolling real-time window, while custom date ranges use datetime.max.time() for day-aligned boundaries. This is by design to support different use cases.
Applied to files:
engine/src/api/endpoints/analytics.py
🧬 Code graph analysis (5)
ui/src/components/analytics/CostTrendChart.tsx (2)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-138)ui/src/types/analytics.ts (1)
DailyMetrics(24-33)
engine/src/api/models/conversation.py (1)
ui/src/types/analytics.ts (2)
DailyMetrics(24-33)MetricsAggregation(3-21)
engine/src/api/endpoints/analytics.py (2)
engine/src/api/models/conversation.py (3)
Conversation(12-33)MetricsAggregation(140-157)DailyMetrics(130-138)ui/src/types/analytics.ts (2)
MetricsAggregation(3-21)DailyMetrics(24-33)
ui/src/lib/api.ts (2)
engine/src/api/models/conversation.py (1)
MetricsAggregation(140-157)ui/src/types/analytics.ts (1)
MetricsAggregation(3-21)
ui/src/components/analytics/TotalResponseTimeAreaChart.tsx (2)
engine/src/api/models/conversation.py (1)
DailyMetrics(130-138)ui/src/types/analytics.ts (1)
DailyMetrics(24-33)
🪛 Biome (2.1.2)
ui/src/components/analytics/AnimatedCounter.tsx
[error] 28-28: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 34-34: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 37-37: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🪛 Ruff (0.14.11)
engine/src/api/endpoints/analytics.py
36-36: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
37-37: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
38-38: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
38-38: Do not perform function call fastapi_users.current_user in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (16)
ui/src/components/navbar/Navbar.tsx (1)
70-75: LGTM!The Analytics NavIcon follows the established pattern for other navigation items, with consistent icon sizing, tooltip, routing, and active state handling.
ui/src/components/analytics/CacheEfficiencyGauge.tsx (1)
10-30: LGTM!The input validation properly handles edge cases (NaN, undefined, null, out-of-range values) and the tiered color scheme provides clear visual feedback for cache efficiency levels.
ui/src/lib/api.ts (1)
83-97: LGTM with minor note on parameter precedence.The implementation correctly handles both the simple number case and the flexible object case. When both
lastNDaysand date fields are provided in the object, all parameters will be sent to the backend wherestart_datetakes precedence. This is acceptable given the frontend validation, but consider documenting this behavior in a JSDoc comment for future maintainers.engine/src/api/endpoints/analytics.py (4)
16-27: LGTM!The
_empty_daily_statshelper function is well-structured, providing a clean factory for daily statistics dictionaries. This addresses previous feedback about deduplicating the default dictionary structure.
95-159: LGTM!The aggregation loop correctly:
- Handles missing timestamps with early continue
- Filters messages by date range
- Separates tool approval counting from token usage processing
- Safely handles optional latency metrics with null checks
167-185: LGTM!The daily breakdown correctly iterates through all dates in the period, providing zero-filled days for chart continuity. The
dateobject is now passed directly toDailyMetrics.
200-218: LGTM!The
MetricsAggregationresponse is correctly constructed with all required fields matching the Pydantic model definition.engine/src/api/models/conversation.py (5)
5-5: LGTM!The
dateimport is correctly added to support theDailyMetrics.datefield type.
57-67: LGTM!The
TokenUsageMetricsmodel is well-structured with sensible defaults, preventing instantiation errors when fields are missing.
86-86: LGTM!The optional
token_usagefield correctly extendsMessageReadto include token metrics when available.
130-139: LGTM!The
DailyMetricsmodel aligns with the frontendDailyMetricsinterface. Pydantic will serialize thedatefield to an ISO string, matching the frontend'sstringtype expectation.
140-158: LGTM!The
MetricsAggregationmodel correctly defines all fields required by the analytics dashboard, matching the frontendMetricsAggregationinterface. Thedatetimefields will serialize to ISO strings for JSON responses.ui/design_system.md (1)
1-81: Well-structured design system documentation.This is a valuable addition that documents the color palette, typography, spacing, effects, and animation utilities. It will help maintain consistency across the UI components, particularly for the new analytics dashboard feature.
ui/package.json (3)
15-17: Radix UI dependencies for popover and slot primitives.These additions support the new DatePickerWithRange and popover components needed for the analytics time range selector.
40-40: Date picker addition for analytics time range selection.This supports the custom date range feature in the analytics dashboard.
66-66: TypeScript pinned to exact version 5.9.3 for build reproducibility.Exact pinning ensures consistent builds across environments. This version is compatible with Next.js 14.2.11 and React 18.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
nitishbhosale7
force-pushed
the
#71/feat/analytics-dashboard-for-token-usage
branch
from
1da946e to
f4c0dd4
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ui/src/components/analytics/Analytics.tsx`:
- Around line 32-55: When timeRange === "custom" but dateRange is incomplete you
return early leaving previous metrics visible; before setLoading(false) and
return, clear the metrics state so the dashboard shows an empty state (e.g.,
call your metrics state updater such as setData([]) or setResult(null)). Locate
the custom-range branch around timeRange, dateRange, setLoading and
result/getMetrics and add a call to the appropriate state setter to clear the
current metrics prior to returning.
In `@ui/src/components/analytics/TokenUsageChart.tsx`:
- Around line 41-67: The tickFormatter on the XAxis and the labelFormatter on
the Tooltip are creating Date objects from "YYYY-MM-DD" strings which are
treated as UTC and cause off-by-one-day shifts; instead, parse the date string
directly (split by "-" into year, month, day) and use those numeric parts to
build the display strings (e.g., return `${Number(month)}/${Number(day)}` for
XAxis tickFormatter and return a localized-looking string like
`${Number(month)}/${Number(day)}/${year}` or other desired format in Tooltip
labelFormatter) so no UTC parsing occurs; update the XAxis tickFormatter and
Tooltip labelFormatter accordingly.
♻️ Duplicate comments (7)
ui/src/components/analytics/DatePickerWithRange.tsx (3)
69-77: Enforce range invariant: cleartowhenfromis cleared.When the start date is cleared (
valis undefined), the current logic preservesto(line 74), resulting in{ from: undefined, to: <date> }. This invalid range state can leak into query params or filtering logic.🔧 Proposed fix
onSelect={(val) => { + // If start is cleared, clear the whole range to keep invariants. + if (!val) { + setDate(undefined) + setIsStartOpen(false) + return + } // If picking a new start date that is after current end date, clear end date - if (val && date?.to && val > date.to) { + if (date?.to && val > date.to) { setDate({ from: val, to: undefined }) } else { - setDate({ from: val || undefined, to: date?.to }) + setDate({ from: val, to: date?.to }) } setIsStartOpen(false) }}
78-84: Use date-only comparison to prevent time-of-day issues.The comparison
day > new Date()includes the time component. Since calendar days are at midnight, this can cause inconsistent behavior for determining if a date is "in the future."📅 Proposed fix
disabled={(day) => { // Disable if after end date if (date?.to && day > date.to) return true; // Disable if future date - if (day > new Date()) return true; + const today = new Date(); + today.setHours(23, 59, 59, 999); + if (day > today) return true; return false; }}
120-126: Same time-of-day comparison issue in end date picker.Apply the same date normalization fix here for consistent behavior across both date pickers.
📅 Proposed fix
disabled={(day) => { // Disable if before start date if (date?.from && day < date.from) return true; // Disable if future date - if (day > new Date()) return true; + const today = new Date(); + today.setHours(23, 59, 59, 999); + if (day > today) return true; return false; }}ui/src/components/analytics/CostTrendChart.tsx (3)
35-38: Timezone date-shift risk in XAxis formatter.
new Date(value)parses ISO date strings (e.g.,"2025-01-15") as UTC, which can shift the displayed day depending on the user's timezone. Parse the date string directly to avoid this.🔧 Proposed fix
<XAxis dataKey="date" tickFormatter={(value) => { - const date = new Date(value); - return `${date.getMonth() + 1}/${date.getDate()}`; + const [, mm, dd] = String(value).split("-"); + return `${Number(mm)}/${Number(dd)}`; }}
58-58: Same timezone issue in Tooltip labelFormatter.Consider using the raw date string or parsing without UTC interpretation.
🔧 Proposed fix
- labelFormatter={(label) => new Date(label).toLocaleDateString()} + labelFormatter={(label) => String(label)}
61-70: Remove invalidlabelprop and redundantstackId.Thelabel="Cost"prop on the<Area>component is invalid. Recharts Area acceptsnameprop for legend labels — which you already have. ThestackId="1"is also redundant when there's only one Area series.🔧 Proposed fix
<Area type="monotone" dataKey="cost" name="Cost" - label="Cost" stroke="#10b981" - stackId="1" fill="url(`#colorCost`)" animationDuration={1500} />ui/src/components/analytics/AnimatedCounter.tsx (1)
11-52: Fix Rules of Hooks violation by moving hooks above early returns.Hooks are called conditionally when
valueis non-numeric (Line 14–26), which can break rendering.🐛 Proposed fix
export function AnimatedCounter({ value, className }: AnimatedCounterProps) { - const match = value.match(/^([^0-9\.-]*)([\d,\.]+)([^0-9\.]*)$/); - - if (!match) { - return <span className={className}>{value}</span>; - } - - const prefix = match[1]; - const numericPart = match[2]; - const suffix = match[3]; - - const parsedValue = parseFloat(numericPart.replace(/,/g, "")); - - if (isNaN(parsedValue)) { - return <span className={className}>{value}</span>; - } + const match = value.match(/^([^0-9\.-]*)([\d,\.]+)([^0-9\.]*)$/); + const prefix = match?.[1] ?? ""; + const numericPart = match?.[2] ?? ""; + const suffix = match?.[3] ?? ""; + const parsedValue = match ? parseFloat(numericPart.replace(/,/g, "")) : NaN; + const isValidNumber = match != null && !isNaN(parsedValue); const count = useSpring(0, { stiffness: 100, damping: 20, duration: 0.5 }); useEffect(() => { - count.set(parsedValue); - }, [parsedValue, count]); + if (isValidNumber) { + count.set(parsedValue); + } + }, [parsedValue, count, isValidNumber]); + const rounded = useTransform(count, (latest) => { + if (!isValidNumber) return value; const hasDecimal = numericPart.includes("."); const decimalPlaces = hasDecimal ? numericPart.split(".")[1].length : 0; let formattedNumber = latest.toFixed(decimalPlaces); @@ return `${prefix}${formattedNumber}${suffix}`; }); + if (!isValidNumber) { + return <span className={className}>{value}</span>; + } + return ( <motion.span className={className}>{rounded}</motion.span> ); }React Rules of Hooks early return conditional hooks
🧹 Nitpick comments (5)
ui/src/components/analytics/CacheEfficiencyGauge.tsx (1)
54-63: Consider aligning the shadow color with the dynamic gauge color.The label uses a fixed purple shadow (
rgba(139, 92, 246, 0.5)) while the gauge color is computed dynamically based on the percentage. For visual consistency, consider usingcacheEfficiencyColorfor the shadow as well.💡 Optional enhancement
<Label value={`${percentage}%`} position="center" fill="#fff" style={{ fontSize: "32px", fontWeight: "bold", - filter: "drop-shadow(0px 0px 5px rgba(139, 92, 246, 0.5))", + filter: `drop-shadow(0px 0px 5px ${cacheEfficiencyColor}80)`, }} />engine/src/api/endpoints/analytics.py (1)
194-194: Consider clampingcache_hit_rateto [0.0, 1.0].If upstream data reports
cached_tokens > prompt_tokens(invalid but possible), the rate will exceed 1.0, which may confuse users when displayed as a percentage.Suggested fix
- cache_hit_rate = (total_cached_tokens / total_prompt_tokens) if total_prompt_tokens > 0 else 0.0 + cache_hit_rate = min((total_cached_tokens / total_prompt_tokens), 1.0) if total_prompt_tokens > 0 else 0.0ui/src/components/analytics/TotalResponseTimeAreaChart.tsx (3)
4-14: Remove unusedLineimport.
Lineis imported from recharts but never used in this component.Suggested fix
import { AreaChart, - Line, XAxis, YAxis, CartesianGrid, Tooltip, Legend, ResponsiveContainer, Area, } from "recharts";
23-27: Remove unusedavg_ttft_msnormalization.Line 25 normalizes
avg_ttft_msbut the chart only displaysavg_ttr_ms. This is unnecessary processing.Suggested fix
const chartData = data.map((d) => ({ ...d, - avg_ttft_ms: d.avg_ttft_ms || 0, avg_ttr_ms: d.avg_ttr_ms || 0, }));
22-22: Consider renaming file to match export.The file is named
TotalResponseTimeAreaChart.tsxbut exportsTotalResponseTimeLineChart. Consider aligning these for consistency.
nitishbhosale7
force-pushed
the
#71/feat/analytics-dashboard-for-token-usage
branch
2 times, most recently
from
95d1f99 to
3788da9
Compare
|
yay!! no more extra comments by Code Rabbit |
nitishbhosale7
force-pushed
the
#71/feat/analytics-dashboard-for-token-usage
branch
from
3788da9 to
79b7939
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@engine/src/api/endpoints/analytics.py`:
- Around line 68-73: The current Conversation.filter using
updated_at__lte=period_end_dt can drop conversations that contain messages
inside the period but were later updated; instead filter by the messages'
timestamps or derive conversation IDs from Message records in the period.
Replace the Conversation.filter(user=user, updated_at__gte=period_start,
updated_at__lte=period_end_dt) logic with a query that selects conversations
whose related Message.created_at (or equivalent message timestamp field) falls
between period_start and period_end_dt (e.g., via
messages__created_at__gte/___lte or by querying Message for created_at range and
using distinct conversation IDs) so all in-range messages are scanned (refer to
Conversation.filter, updated_at__lte, period_end_dt, period_start, and the
Message.created_at relation).
♻️ Duplicate comments (4)
ui/src/components/analytics/AnimatedCounter.tsx (1)
11-48: Fix Rules of Hooks violation (hooks after early returns).
Hooks on Lines 28–48 run conditionally because of early returns on Lines 14–26; this breaks hook ordering and can crash whenvaluetoggles between numeric/non‑numeric.🐛 Proposed fix (move hooks to top level)
export function AnimatedCounter({ value, className }: AnimatedCounterProps) { - const match = value.match(/^([^0-9\.-]*)([\d,\.]+)([^0-9\.]*)$/); - - if (!match) { - return <span className={className}>{value}</span>; - } - - const prefix = match[1]; - const numericPart = match[2]; - const suffix = match[3]; - - const parsedValue = parseFloat(numericPart.replace(/,/g, "")); - - if (isNaN(parsedValue)) { - return <span className={className}>{value}</span>; - } + const match = value.match(/^([^0-9\.-]*)([\d,\.]+)([^0-9\.]*)$/); + const prefix = match?.[1] ?? ""; + const numericPart = match?.[2] ?? ""; + const suffix = match?.[3] ?? ""; + const parsedValue = match ? parseFloat(numericPart.replace(/,/g, "")) : NaN; + const isValidNumber = match !== null && !isNaN(parsedValue); const count = useSpring(0, { stiffness: 100, damping: 20, duration: 0.5 }); useEffect(() => { - count.set(parsedValue); - }, [parsedValue, count]); + if (isValidNumber) { + count.set(parsedValue); + } + }, [parsedValue, count, isValidNumber]); + const rounded = useTransform(count, (latest) => { + if (!isValidNumber) return value; const hasDecimal = numericPart.includes("."); const decimalPlaces = hasDecimal ? numericPart.split(".")[1].length : 0; @@ return `${prefix}${formattedNumber}${suffix}`; }); + if (!isValidNumber) { + return <span className={className}>{value}</span>; + } + return ( <motion.span className={className}>{rounded}</motion.span> ); }ui/src/components/analytics/DatePickerWithRange.tsx (2)
69-76: Clear the end date when the start date is cleared.
valcan be undefined, but the current logic preservesto, producing an invalid range ({ from: undefined, to: ... }) that can leak into filtering.🛠️ Proposed fix
onSelect={(val) => { + if (!val) { + setDate(undefined) + setIsStartOpen(false) + return + } // If picking a new start date that is after current end date, clear end date - if (val && date?.to && val > date.to) { + if (date?.to && val > date.to) { setDate({ from: val, to: undefined }) } else { - setDate({ from: val || undefined, to: date?.to }) + setDate({ from: val, to: date?.to }) } setIsStartOpen(false) }}
78-84: Normalize date-only comparisons indisabledchecks.
day > new Date()includes time-of-day and can incorrectly disable “today” later in the day. Normalize to midnight for stable behavior.🛠️ Proposed fix
disabled={(day) => { // Disable if after end date if (date?.to && day > date.to) return true; // Disable if future date - if (day > new Date()) return true; + const today = new Date(); + today.setHours(0, 0, 0, 0); + const dayOnly = new Date(day.getFullYear(), day.getMonth(), day.getDate()); + if (dayOnly > today) return true; return false; }}disabled={(day) => { // Disable if before start date if (date?.from && day < date.from) return true; // Disable if future date - if (day > new Date()) return true; + const today = new Date(); + today.setHours(0, 0, 0, 0); + const dayOnly = new Date(day.getFullYear(), day.getMonth(), day.getDate()); + if (dayOnly > today) return true; return false; }}Also applies to: 120-125
engine/src/api/endpoints/analytics.py (1)
223-228: Avoid returning raw exception messages to clients.The response currently includes
str(e), which can leak internal details. Log the full error, but return a generic message.🛡️ Proposed fix
except Exception as e: logger.exception(f"Error getting metrics: {str(e)}") raise HTTPException( status_code=500, - detail=f"Error getting metrics: {str(e)}", + detail="An internal error occurred while fetching metrics", ) from e
nitishbhosale7
force-pushed
the
#71/feat/analytics-dashboard-for-token-usage
branch
from
79b7939 to
4053887
Compare
|
@nitishbhosale7 are you planning on closing this? |
nitishbhosale7
force-pushed
the
#71/feat/analytics-dashboard-for-token-usage
branch
from
4053887 to
80ec4e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/src/components/navbar/Navbar.tsx (1)
114-134: 🧹 Nitpick | 🔵 TrivialConsider lifting
TooltipProviderto the app layout level.Every
NavIconrender mounts its ownTooltipProvider. Radix UI recommends a single provider at the root so tooltip state is coordinated globally. With the current implementation, rapid hover between icons can briefly show two tooltips simultaneously.♻️ Suggested refactor
Move the provider to the layout that wraps the navbar (e.g.,
app/layout.tsx):// In app/layout.tsx or the layout that renders <Navbar /> + import { TooltipProvider } from "@/components/ui/tooltip"; ... + <TooltipProvider> <Navbar /> {children} + </TooltipProvider>Then remove
TooltipProviderfromNavIcon:return ( - <TooltipProvider> <Tooltip> ... </Tooltip> - </TooltipProvider> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/navbar/Navbar.tsx` around lines 114 - 134, The Navbar currently mounts a TooltipProvider inside NavIcon (see TooltipProvider in Navbar.tsx) causing multiple providers and uncoordinated tooltip state; lift a single TooltipProvider to the app layout component that wraps the navbar (add it once at the top-level layout) and remove the TooltipProvider wrapper from NavIcon/Navbar.tsx so NavIcon only renders Tooltip, TooltipTrigger, TooltipContent and the button/icon logic; ensure existing TooltipTrigger/TooltipContent usage (and props like tooltip, icon, isActive, onClick) remains unchanged.engine/src/api/services/conversation_persistence.py (1)
184-243:⚠️ Potential issue | 🔴 CriticalCritical:
messages_jsonis never saved back to the Conversation inappend_text_segment.Both branches of
append_text_segmentmodify the in-memorymessageslist (lines 205, 231-232) but never callconversation.update_from_dict({"messages_json": messages}).save(). Compare withappend_user_message(line 173) andappend_tool_segment(line 290), which both persist the updatedmessages_json.The only incidental save happens inside
_apply_usage_to_latest_assistant(line 143), but that only triggers whenrun_idis provided AND a usage snapshot exists. Without those conditions, assistant text content is lost.Proposed fix — add save at end of function
if run_id: usage_snapshot = self._snapshot_usage(conversation_id, run_id) if usage_snapshot: assistant = messages[-1] assistant["token_usage"] = TokenUsageMetrics(**usage_snapshot).model_dump() await self._message.update(token_usage=assistant["token_usage"]) + await conversation.update_from_dict({"messages_json": messages}).save()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/api/services/conversation_persistence.py` around lines 184 - 243, append_text_segment mutates the local messages list (Conversation.messages_json) but never persists it back to the DB, so appended assistant segments are lost unless _snapshot_usage triggers a save; fix by calling conversation.update_from_dict({"messages_json": messages}).save() after you modify messages in both branches (after messages.append(...) in the new-assistant branch and after assistant["segments"]=segments / assistant["content"]=... in the existing-assistant branch), ensuring you still create/update Message via Message.create or self._message.update as now and preserve the existing run_id token-usage logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/src/api/endpoints/analytics.py`:
- Around line 67-71: The current Message.filter(...) call loads all Message rows
into memory (using Message.filter(conversation__user=user,
created_at__gte=period_start,
created_at__lte=period_end).distinct().prefetch_related('conversation')) and
then aggregates in Python; replace this with database-side aggregation: use
Tortoise annotate()/aggregate() or a GROUP BY query to compute the totals and
per-day counts (grouping by date_trunc on created_at or DATE(created_at)) and
any other metrics needed by the endpoint, remove the
.prefetch_related('conversation') and the in-Python loops, and return only the
summarized rows (e.g., date and count, total_count, distinct_conversation_count)
so the endpoint no longer materializes all Message objects in memory.
In `@engine/src/api/models/conversation.py`:
- Around line 46-47: Add a new DB migration that alters the messages table to
add the token_usage column so the schema matches the Message model: create a
migration file (following the project's migration naming pattern) that executes
ALTER TABLE messages ADD COLUMN token_usage JSONB NULL (and the corresponding
downgrade that drops the column). Ensure the migration references the Message
model change (token_usage = fields.JSONField) so tests/migration runner pick it
up and include any necessary import/registration code consistent with existing
migrations (e.g., similar structure to 0_20250903153104_init.py and
1_20250912203310_update.py).
In `@engine/src/api/services/conversation_persistence.py`:
- Line 235: The concatenation uses text.strip('{}') which removes all
leading/trailing braces rather than a single wrapper pair; change the logic
around the expression in the method that builds content (the line using
self._message.content and variable text) to detect a single surrounding pair of
braces and remove only that pair (e.g., check text.lstrip().startswith('{') and
text.rstrip().endswith('}') then slice to text[1:-1] after trimming outer
whitespace), otherwise leave text unchanged before concatenating with
self._message.content.
- Line 43: Remove the debug print statements that leak to stdout and either
delete them or replace with logger.debug; specifically remove the two prints
containing the variable sequence (the lines with print(f'sequnce {sequence}')
and print(f"next Sequence {str(sequence)}")) and, if keeping a log, change to
logger.debug with a corrected message (e.g., "sequence %s", sequence) to avoid
the "sequnce" typo and format issues; update the function where these prints
appear in conversation_persistence.py to use logger.debug(...) or remove the
calls entirely.
- Around line 234-243: The code is writing segments to a non-existent Message
field `message_json`, so the data is silently dropped; update both calls that
pass message_json (the `await self._message.update(message_json=segments)` in
the content/segments write and the other update near the usage snapshot where
`message_json` is used) to write to the existing JSONField `message_metadata`
instead (i.e., `message_metadata=segments`), and ensure `segments` is
JSON-serializable before updating (e.g., convert to plain dict/list structures
if it’s a custom object) so the segment data is persisted on the Message model.
In `@ui/design_system.md`:
- Line 5: The markdown headings like "### Base Colors (Tailwind Config)" (and
the other four affected headings at lines flagged) are missing a required blank
line after the heading; fix by inserting a single blank line immediately after
each "### ..." subheading (e.g., "### Base Colors (Tailwind Config)" and the
other three-level headings flagged) so there is an empty line between the
heading and the following content, and apply the same change to all five
affected headings to satisfy MD022.
In `@ui/package.json`:
- Line 66: Update the TypeScript pin and verify the codebase compiles: run tsc
--noEmit to surface any new type errors caused by the upgrade from the
package.json TypeScript entry ("typescript": "5.9.3"), then fix any failures—pay
special attention to complex generics in analytics components (Recharts chart
data arrays and Framer Motion transform callback types) and adjust type
annotations or generic constraints to satisfy the stricter inference rules;
additionally consider loosening the pin to "^5.9.3" in package.json if you want
automatic patch upgrades for TS.
In `@ui/src/app/analytics/page.tsx`:
- Line 1: The page is incorrectly marked as a client component by the "use
client" directive which forces the whole page and its imports into the client
bundle; remove the top-level "use client" line from page.tsx so the page remains
a server component and let any client-only pieces (e.g., Navbar, Analytics)
remain/declare themselves as client components via their own "use client"
directives; ensure you don't accidentally move server-only logic into those
client components and run the app to confirm no runtime client-only errors.
In `@ui/src/components/analytics/Analytics.tsx`:
- Around line 26-68: The effect's fetchData can race when timeRange/dateRange
change: modify the useEffect to cancel stale fetches by introducing a local
"cancelled" or "isStale" flag (or use an AbortController) and check it before
calling setData/setError/setLoading; ensure you call setLoading(true) at start
and only setLoading(false) in the finally block, removing the earlier redundant
setLoading(false)/setData(null)/return path inside the custom dateRange branch;
references to update are inside fetchData, getMetrics(...) calls, and the
useEffect cleanup should set the flag (or call controller.abort()) so late
responses from getMetrics do not overwrite state.
- Around line 144-200: The daily metrics lack a per-day conversation_count
field—update the backend DailyMetrics model and the frontend
DailyMetrics/analytics interface to include conversation_count, modify the
analytics endpoint (the function that builds displayData.daily_breakdown) to
compute and populate conversation_count by aggregating conversation records per
day, and then update any components that consume displayData.daily_breakdown
(e.g., TokenUsageChart, CostTrendChart, TimetoFirstTokenAreaChart,
TotalResponseTimeLineChart) to use the new conversation_count value (or
gracefully handle its absence) so per-day conversation volume can be charted or
overlaid. Ensure the symbol names DailyMetrics, displayData.daily_breakdown, and
the analytics endpoint function are changed consistently in both backend and
frontend.
In `@ui/src/components/analytics/AnimatedCounter.tsx`:
- Line 3: The import line in AnimatedCounter.tsx includes an unused symbol
useRef; remove useRef from the import so it only imports what’s used (e.g.,
change `import { useEffect, useRef } from "react";` to import only useEffect).
Update the import at the top of the AnimatedCounter component file
(AnimatedCounter) to eliminate the unused reference.
In `@ui/src/components/analytics/DatePickerWithRange.tsx`:
- Around line 31-39: GlassWrapper is declared inside the component render which
creates a new component identity on every render and forces children like
Calendar to unmount/remount; fix by extracting GlassWrapper as a top-level
component (declare const GlassWrapper = (...) => ... outside the parent
component) or replace its usage with the equivalent plain JSX wrapper divs
inline where Calendar is rendered, ensuring you keep the same classNames
("lg-wrapper", "lg-effect", "lg-tint", "lg-shine", inner "relative z-10 p-2") so
Calendar's component identity and internal state are preserved.
In `@ui/src/components/analytics/TimeRangeSelector.tsx`:
- Around line 38-48: Remove the unnecessary key prop from the Button (the
key={"custom"} on the "Custom" Button is only meaningful for list siblings) and
replace the JSX string expression {"Custom"} with the plain literal Custom;
update the Button element associated with selectedRange/onRangeChange so it no
longer includes the key prop and uses plain text content instead.
- Line 20: The wrapper div in TimeRangeSelector (component
TimeRangeSelector.tsx) uses the undefined Tailwind class bg-dark-card so the
background will be dropped; update the class string on that div to use an
existing token such as bg-dark-secondary (or another valid bg-... token from
ui/tailwind.config.ts) or alternatively add a card entry under the dark color
group in ui/tailwind.config.ts so bg-dark-card becomes valid; ensure you only
change the class list in the JSX (the div with className "flex ...") or the
Tailwind config accordingly to restore the intended dark card background.
- Around line 12-17: The static array "ranges" inside the TimeRangeSelector
component is recreated on every render; move it outside the component body as a
module-level constant (e.g., export const RANGES or const ranges) so it is
initialized once. Keep the same type annotation ({ label: string; value: number
| "custom" }[]) and the same entries ({ label: "7D", value: 7 }, etc.), and
update references inside TimeRangeSelector to use the moved symbol.
In `@ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx`:
- Around line 22-26: Replace the imprecise || null-guard and dead override in
the data mapping: in the data.map that constructs chartData (the mapping inside
TimetoFirstTokenAreaChart, currently setting avg_ttft_ms and avg_ttr_ms), use
the nullish coalescing operator (??) for avg_ttft_ms (e.g., avg_ttft_ms:
d.avg_ttft_ms ?? 0) and remove the avg_ttr_ms override entirely since avg_ttr_ms
is not used as a dataKey and the spread ...d already preserves it.
In `@ui/src/components/analytics/TokenUsageChart.tsx`:
- Line 57: In TokenUsageChart update the YAxis tickFormatter so it always
returns a string (currently it returns a number for values <1000); change the
handler used in the YAxis tickFormatter prop to return a string in both branches
(e.g., use template strings or toString()) and preserve the existing
thousand-format branch (the expression currently `${value / 1000}k`), ensuring
the non-thousand branch returns `${value}` or value.toString(); this fixes the
Recharts-required signature for tickFormatter.
In `@ui/src/components/analytics/TotalResponseTimeAreaChart.tsx`:
- Around line 22-25: Replace the boolean-OR null/zero coercion in the chartData
map with a nullish coalescing operator to only treat null/undefined as absent:
in TotalResponseTimeAreaChart.tsx update the mapping that creates chartData (the
line setting avg_ttr_ms within the data.map callback) to use ?? instead of || so
that falsy but valid values (e.g., 0) are preserved.
- Around line 37-44: The XAxis in TotalResponseTimeAreaChart is currently using
tickFormatter={(value) => String(value)} so dates render as "YYYY-MM-DD"; change
the tickFormatter to format the incoming date strings into "M/D" to match
TokenUsageChart/TimetoFirstTokenAreaChart (e.g., parse the "YYYY-MM-DD" value
into a Date and return `${month}/${day}`), and ensure it gracefully handles
null/undefined inputs; update the XAxis tickFormatter reference in
TotalResponseTimeAreaChart.tsx (and mirror the same formatter used by
TokenUsageChart/TimetoFirstTokenAreaChart if there is a shared helper) so all
charts use consistent "M/D" formatting.
In `@ui/src/lib/api.ts`:
- Around line 83-116: The getMetrics function currently treats options.lastNDays
with a truthy check which drops valid numeric 0 and can produce a trailing "?"
when no params exist; change logic to explicitly check for undefined (e.g.,
options.lastNDays !== undefined) or validate and throw for invalid values, and
replace manual string assembly with URLSearchParams to build query parameters
(include last_n_days, start_date, end_date only when not undefined, formatting
Date with toISOString().split("T")[0]) so the URL is correctly encoded and no
empty query string is produced; update error handling in getMetrics accordingly
if you choose to reject invalid lastNDays values.
In `@ui/src/types/analytics.ts`:
- Around line 24-33: The DailyMetrics TypeScript interface is missing the
conversation_count field—add conversation_count: number to the DailyMetrics
interface (next to date, cost, prompt_tokens, etc.) and update any places that
map or consume DailyMetrics so they read this new property; also ensure the
backend DailyMetrics Pydantic model (the server-side DailyMetrics) includes
conversation_count: int and that the analytics aggregation query populates it so
the frontend receives the value for charting.
- Around line 3-21: The MetricsAggregation TypeScript interface is missing the
required period-over-period fields; add numeric (or nullable number) fields
cost_change_pct and tokens_change_pct to the frontend MetricsAggregation
interface and mirror those same fields on the backend Pydantic model
MetricsAggregation in engine/src/api/models/conversation.py; update the
analytics endpoint handler that builds MetricsAggregation to query the prior
period totals (cost and tokens), compute percentage change = (current - prior) /
prior * 100 with safe null/zero handling (e.g., null if no prior data), and
populate cost_change_pct and tokens_change_pct so MetricCard can read them.
---
Outside diff comments:
In `@engine/src/api/services/conversation_persistence.py`:
- Around line 184-243: append_text_segment mutates the local messages list
(Conversation.messages_json) but never persists it back to the DB, so appended
assistant segments are lost unless _snapshot_usage triggers a save; fix by
calling conversation.update_from_dict({"messages_json": messages}).save() after
you modify messages in both branches (after messages.append(...) in the
new-assistant branch and after assistant["segments"]=segments /
assistant["content"]=... in the existing-assistant branch), ensuring you still
create/update Message via Message.create or self._message.update as now and
preserve the existing run_id token-usage logic.
In `@ui/src/components/navbar/Navbar.tsx`:
- Around line 114-134: The Navbar currently mounts a TooltipProvider inside
NavIcon (see TooltipProvider in Navbar.tsx) causing multiple providers and
uncoordinated tooltip state; lift a single TooltipProvider to the app layout
component that wraps the navbar (add it once at the top-level layout) and remove
the TooltipProvider wrapper from NavIcon/Navbar.tsx so NavIcon only renders
Tooltip, TooltipTrigger, TooltipContent and the button/icon logic; ensure
existing TooltipTrigger/TooltipContent usage (and props like tooltip, icon,
isActive, onClick) remains unchanged.
---
Duplicate comments:
In `@engine/src/api/endpoints/analytics.py`:
- Around line 50-56: Add server-side validation to reject requests where
start_date > end_date: after computing s_date and e_date (the variables in this
block) check if e_date and s_date are both set and if s_date > e_date raise an
HTTPException(400) with a clear message; ensure the analytics endpoint imports
HTTPException from fastapi (or uses the existing error helper) and do this check
before creating period_start/period_end/period_end_dt so invalid ranges return
400 instead of empty results.
- Around line 217-222: The handler currently embeds internal error text into the
HTTPException detail by using str(e) (see the except block around
logger.exception and HTTPException), which can leak sensitive info; change the
raised HTTPException to return a generic client-facing message (e.g., "Internal
server error" or "Error getting metrics") instead of str(e), keep
logger.exception(...) as-is to record the full error, and re-raise the
HTTPException (from e optional) so the internal details are logged but not
returned to clients.
In `@engine/src/api/services/conversation_persistence.py`:
- Around line 122-127: The current bare except around
Conversation.get(id=conversation_id) swallows all errors; change it to catch
Exception as e, log the exception (including conversation_id and run_id) using
the service's logger (or module logger) before returning, and keep the existing
finalize behavior by still calling self._clear_usage_buffer(conversation_id,
run_id) when finalize is true; ensure the log message references
Conversation.get, conversation_id and run_id for easier debugging.
In `@ui/src/components/analytics/Analytics.tsx`:
- Around line 138-175: Add an optional trend prop to MetricCardProps (trend?:
number | null) and update the MetricCard component to render a small directional
badge when trend is non-null: show ▲ <value>% for positive, ▼ <value>% for
negative, formatted to one decimal (include a + sign for positive), and style
the badge (green for positive, red for negative). Then, in Analytics.tsx pass
trend={displayData.cost_change_pct} to the "Total Cost" MetricCard and
trend={displayData.tokens_change_pct} to the "Total Tokens" MetricCard, ensuring
null-safe handling so MetricCard hides the badge when trend is null/undefined.
In `@ui/src/components/analytics/AnimatedCounter.tsx`:
- Around line 11-52: AnimatedCounter violates the Rules of Hooks by
conditionally calling useSpring, useEffect, and useTransform after early
returns; move all hooks to the top-level of the AnimatedCounter function so they
are invoked on every render (declare useSpring, useTransform and the useEffect
that sets the spring before inspecting match/parsedValue), then use the
match/parsedValue logic only to decide what to render (either the animated
motion.span using the transformed value or a plain span with the original
value); reference the AnimatedCounter function and the hook calls useSpring,
useEffect, and useTransform when making the change.
In `@ui/src/components/analytics/CostTrendChart.tsx`:
- Around line 35-38: The chart is still parsing date-only strings with new
Date(...) which yields UTC-midnight and causes date-shifts for negative
timezones; update the parsing in CostTrendChart's tickFormatter and
labelFormatter to split the "YYYY-MM-DD" string into year/month/day and
construct a local Date (e.g., new Date(year, month-1, day)) so the displayed day
is correct for client locale; locate the tickFormatter and labelFormatter
functions in CostTrendChart and replace the direct new Date(dateString) usage
with splitting + constructing a Date from numeric year, month-1, and day.
In `@ui/src/components/analytics/DatePickerWithRange.tsx`:
- Around line 69-75: The onSelect handler in DatePickerWithRange (the callback
that calls setDate and setIsStartOpen) can produce an invalid range when the
start date is cleared; update the logic so when val is falsy (user cleared
start) you clear both from and to (e.g., setDate({ from: undefined, to:
undefined })) instead of preserving date?.to, while keeping the existing branch
that sets from=val and clears to when val > date.to; adjust the branch that
currently does setDate({ from: val || undefined, to: date?.to }) accordingly.
- Around line 77-81: The disabled predicate in DatePickerWithRange compares the
calendar day (variable day) against new Date() which includes a time component,
causing "today" to sometimes be treated as future; update both disabled handlers
(the one using date?.to and the end-date picker counterpart) to compare dates at
the same granularity by normalizing both operands to midnight (e.g., use a
helper like startOfDay or setHours(0,0,0,0) on day and on new Date()) or use a
date-only comparison utility so that only the calendar date is compared, not the
time-of-day.
In `@ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx`:
- Around line 39-46: The XAxis tickFormatter in TimetoFirstTokenAreaChart is
rendering full ISO dates (e.g., "2025-01-15") instead of the short "M/D" used by
TokenUsageChart; update the XAxis tickFormatter (inside
TimetoFirstTokenAreaChart) to format the incoming daily_breakdown date string
into "M/D" consistently with TokenUsageChart by treating the value as an ISO
date string (avoid constructing a Date to prevent timezone shifts) and
extracting month/day parts (e.g., split the ISO string or slice the YYYY-MM-DD
portion) to return "M/D".
2a14f7c to
3e34b4d
Compare
nitishbhosale7
force-pushed
the
#71/feat/analytics-dashboard-for-token-usage
branch
from
3e34b4d to
56c3dbf
Compare
…/feat/analytics-dashboard-for-token-usage
…analytics-dashboard-for-token-usage Conflict/skyflo-ai#71/feat/analytics dashboard for token usage
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/src/api/models/conversation.py (1)
1-158:⚠️ Potential issue | 🟡 MinorCI is blocked: Ruff format check fails for this file.
The pipeline reports
Would reformat: 'src/api/models/conversation.py'. This must be resolved before merge. Runruff format src/api/models/conversation.pyto fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/api/models/conversation.py` around lines 1 - 158, Ruff reports the file is unformatted; run ruff format (or apply equivalent stylistic changes) on the Conversation/Message models file to satisfy the formatter—ensure formatting fixes touch the top-level model classes (Conversation, Message), the Pydantic models (TokenUsageMetrics, MessageRead, ConversationRead, MetricsAggregation), and import block so that ruff no longer reports "Would reformat"; after formatting, re-run ruff check to confirm the CI error is resolved.
♻️ Duplicate comments (20)
ui/design_system.md (1)
5-5:⚠️ Potential issue | 🟡 MinorMD022: headings still missing required blank line after them at lines 5, 27, 64, 73, and 77.
All five
###subheadings are immediately followed by content with no blank line, which violates the markdownlint MD022 rule. This was raised in a prior review and remains unresolved.📝 Proposed fix
### Base Colors (Tailwind Config) + These colors are explicitly defined in your `tailwind.config.ts`...### Semantic Theme Colors (CSS Variables) + These colors use HSL values and adapt based on the theme (Light/Dark).### Liquid Glass Effect (`.lg-*`) + A custom set of utilities to create a "liquid glass" visual style.### Animations + * **Dots**: A typing/loading animation...### Scrollbar + Custom styled webkit scrollbar:Also applies to: 27-27, 64-64, 73-73, 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/design_system.md` at line 5, Several markdown headings (e.g., "### Base Colors (Tailwind Config)" and the other four `###` subheadings) are missing the required blank line after the heading; insert a single blank line immediately after each `###` heading so the heading is followed by an empty line before the next content paragraph or list to satisfy MD022 (apply this change to every `###` subheading in the document).ui/src/components/analytics/DatePickerWithRange.tsx (3)
77-81:⚠️ Potential issue | 🟡 Minor
day > new Date()compares with the current time-of-day, causing inconsistent "today" boundary detection.Since calendar day
Dateobjects are at midnight (00:00:00), comparing againstnew Date()(current time, e.g. 14:35:00) means today's date is still "in the future" during the day — selectable until midnight. Normalizetodayto midnight for consistent behavior.🐛 Proposed fix — normalize to start of day
- if (day > new Date()) return true; + const today = new Date(); today.setHours(0, 0, 0, 0); + if (day > today) return true;Apply the same fix to both the start-date
disabledpredicate (line 79) and the end-datedisabledpredicate (line 118).Also applies to: 116-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/analytics/DatePickerWithRange.tsx` around lines 77 - 81, The disabled predicate is comparing day (midnight-based calendar Date) to new Date() (current time), causing "today" to be treated as future; normalize the current date to midnight and use that for comparison. In DatePickerWithRange.tsx, compute a startOfToday by doing const today = new Date(); today.setHours(0,0,0,0); then replace checks like day > new Date() with day > today in both the start-date disabled predicate (the disabled={(day) => { ... }} used for the start picker) and the end-date disabled predicate (the equivalent disabled predicate used for the end picker) so both use the normalized today boundary.
69-75:⚠️ Potential issue | 🟠 MajorClearing the start date leaves
topopulated, producing an invalid{ from: undefined, to: Date }range.When
valisundefined(user deselects the start date), line 73 callssetDate({ from: undefined, to: date?.to }). Any downstream code that readsdate.towithout a guard assumes a valid range, which could corrupt API query params.🐛 Proposed fix — clear the whole range when start is cleared
onSelect={(val) => { + if (!val) { + setDate(undefined) + setIsStartOpen(false) + return + } if (val && date?.to && val > date.to) { setDate({ from: val, to: undefined }) } else { - setDate({ from: val || undefined, to: date?.to }) + setDate({ from: val, to: date?.to }) } setIsStartOpen(false) }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/analytics/DatePickerWithRange.tsx` around lines 69 - 75, The onSelect handler for the start date currently leaves date.to populated when val is undefined, creating an invalid range; update the onSelect in the start picker so that when val is falsy (user cleared the start), you call setDate({ from: undefined, to: undefined }) instead of preserving date.to, but keep the existing behavior when val is a Date (i.e., if val > date.to then set { from: val, to: undefined } else set { from: val, to: date?.to }); ensure setIsStartOpen(false) remains called. This change targets the onSelect callback that currently references date, setDate, and setIsStartOpen.
31-40:⚠️ Potential issue | 🔴 Critical
GlassWrapperdefined inside the render function causes<Calendar>to unmount/remount on every state change.Every call to
setIsStartOpen/setIsEndOpentriggers a re-render, which creates a newGlassWrappercomponent reference. React treats this as a different component type and unmounts → remounts all children — includingCalendar— destroying its internal navigation state (selected month, focus) on each open/close.🐛 Proposed fix — extract GlassWrapper to module scope
+const GlassWrapper = ({ children }: { children: React.ReactNode }) => ( + <div className="lg-wrapper"> + <div className="lg-effect" /> + <div className="lg-tint" /> + <div className="lg-shine" /> + <div className="relative z-10 p-2"> + {children} + </div> + </div> +) + export function DatePickerWithRange({ ... }) { const [isStartOpen, setIsStartOpen] = React.useState(false) const [isEndOpen, setIsEndOpen] = React.useState(false) - const GlassWrapper = ({ children }: { children: React.ReactNode }) => ( - <div className="lg-wrapper"> - <div className="lg-effect" /> - <div className="lg-tint" /> - <div className="lg-shine" /> - <div className="relative z-10 p-2"> - {children} - </div> - </div> - )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/analytics/DatePickerWithRange.tsx` around lines 31 - 40, The GlassWrapper component is being re-created on every render inside DatePickerWithRange, causing React to unmount/remount its children (including <Calendar>) when setIsStartOpen/setIsEndOpen toggle; move GlassWrapper out of the DatePickerWithRange function to module scope (top-level) so its identity is stable across renders, keep the same props typing ({ children: React.ReactNode }), and update the JSX usage inside DatePickerWithRange to reference the top-level GlassWrapper; no other behavioral changes needed.ui/src/components/analytics/TimeRangeSelector.tsx (3)
12-17: 🧹 Nitpick | 🔵 TrivialMove the static
rangesarray outside the component.
rangesis a constant that never depends on props or state; defining it inside the component recreates the array on every render.♻️ Proposed refactor
+const RANGES: { label: string; value: number | "custom" }[] = [ + { label: "7D", value: 7 }, + { label: "30D", value: 30 }, + { label: "90D", value: 90 }, +]; + export default function TimeRangeSelector({ selectedRange, onRangeChange }: TimeRangeSelectorProps) { - const ranges: { label: string; value: number | "custom" }[] = [ - { label: "7D", value: 7 }, - { label: "30D", value: 30 }, - { label: "90D", value: 90 }, - ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/analytics/TimeRangeSelector.tsx` around lines 12 - 17, Move the static ranges constant out of the TimeRangeSelector component so it isn't recreated on every render: define the const ranges: { label: string; value: number | "custom" }[] above the TimeRangeSelector function (at module scope) and keep the same name and shape so existing references inside TimeRangeSelector continue to work; no behavioral changes are needed beyond relocating the declaration.
38-48: 🧹 Nitpick | 🔵 TrivialRemove the spurious
keyprop and unwrap the string literal on the Custom button.
keyis only meaningful on list-rendered siblings; it has no effect on a standalone element.{"Custom"}is equivalent to the textCustom.♻️ Proposed fix
<Button - key={"custom"} variant={selectedRange === "custom" ? "default" : "ghost"} onClick={() => onRangeChange("custom")} className={`h-8 px-4 text-sm font-medium transition-all duration-200 ${selectedRange === "custom" ? "shadow-lg text-blue-400 bg-blue-400/5" - : " hover:text-blue-400 hover:bg-blue-400/5 duration-300" + : "hover:text-blue-400 hover:bg-blue-400/5 duration-300" }`} > - {"Custom"} + Custom </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/analytics/TimeRangeSelector.tsx` around lines 38 - 48, In the TimeRangeSelector component, remove the unnecessary key={"custom"} prop from the standalone Button element and replace the JSX string expression {"Custom"} with the plain text Custom; update the Button JSX (the one using variant={selectedRange === "custom" ? "default" : "ghost"} and onClick={() => onRangeChange("custom")}) accordingly so it no longer includes the spurious key and uses the unwrapped text node.
20-20:⚠️ Potential issue | 🟠 Major
bg-dark-cardis not defined in the Tailwind config — selector container has no background.This class produces no CSS output and the wrapper renders transparent.
🐛 Proposed fix — use an existing token
- <div className="flex bg-dark-card border max-lg:flex-col items-center border-white/10 rounded-lg p-1 gap-1"> + <div className="flex bg-dark-secondary border max-lg:flex-col items-center border-white/10 rounded-lg p-1 gap-1">Verify available tokens:
#!/bin/bash # List dark-* color tokens in the Tailwind config rg -n "dark" ui/tailwind.config.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/analytics/TimeRangeSelector.tsx` at line 20, The wrapper div in TimeRangeSelector uses the non-existent Tailwind class "bg-dark-card" so the selector has no background; update the className on the div in TimeRangeSelector.tsx (the line containing the string "flex bg-dark-card border ...") to use a valid background token from your Tailwind config (for example replace bg-dark-card with the project's defined dark token such as bg-dark-700 or the equivalent token you find via rg "dark" in ui/tailwind.config.ts), ensuring the rest of the class string remains unchanged.ui/package.json (1)
66-66: Verify TypeScript 5.9.3 patch release exists on npm.TypeScript 5.9 was released on August 1, 2025. The package is pinned to the exact version
5.9.3(no^). If the5.9.3patch does not exist on npm,npm install/yarn installwill fail at build time.#!/bin/bash # Verify typescript 5.9.3 is published on npm curl -sf "https://registry.npmjs.org/typescript/5.9.3" | jq '.version // "NOT FOUND"'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/package.json` at line 66, The package.json pins TypeScript to an exact patch "typescript": "5.9.3" which may not exist on npm; verify that version is published and if it is missing replace the exact pin with a safe specifier (e.g., "^5.9.3" or an existing published patch like "5.9.x") or update to the actual published patch version; locate the dependency entry "typescript": "5.9.3" in ui/package.json, run the registry check (or change to a caret range) and update package.json accordingly, then re-run install to confirm the build succeeds.engine/src/api/services/conversation_persistence.py (3)
206-214:⚠️ Potential issue | 🔴 CriticalStale
self._messagereference can still corrupt data across conversations.The guard
if not self._message:(line 206) does not verify thatself._messagebelongs to the currentconversation_id. While_clear_message()was added tofinalize_usage_snapshotandset_title, any interrupted or errored run that skips finalization leaves a stale_messagereferencing a previous conversation. The next conversation's text will then be appended to the wrongMessagerow.Add a conversation-awareness check:
Proposed fix
- if not self._message: + if not self._message or str(self._message.conversation_id) != str(conversation.id): sequence = await self._get_next_sequence(conversation=conversation) self._message = await Message.create(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/api/services/conversation_persistence.py` around lines 206 - 214, The guard using only "if not self._message" can reuse a stale Message from a previous conversation; change the condition in the block that calls _get_next_sequence and Message.create to also check conversation ownership (e.g., if not self._message or getattr(self._message, 'conversation_id', None) != conversation.id) so a new Message is created whenever the cached self._message does not belong to the current conversation; reference the existing symbols self._message, _get_next_sequence, and Message.create and optionally call _clear_message() before creating a new message to ensure no stale state remains.
232-236:⚠️ Potential issue | 🟠 Major
text.strip('{}')aggressively strips all leading/trailing brace characters.
str.strip('{}')removes every{and}from both ends, not just a single wrapper pair. For text like"{result: success}", this yields"result: success". If the intent is to remove a JSON-like wrapper, use a targeted check-and-slice instead.Proposed fix
if self._message: - content = self._message.content + text.strip('{}') + content = self._message.content + text self._message.content = contentIf stripping a specific wrapper is truly needed, check and slice:
stripped = text.strip() if stripped.startswith('{') and stripped.endswith('}'): stripped = stripped[1:-1] content = self._message.content + stripped🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/api/services/conversation_persistence.py` around lines 232 - 236, The use of text.strip('{}') in the block that updates self._message.content is too aggressive and removes all leading/trailing braces; change it to first trim whitespace, then only remove one wrapping pair if present (e.g., check stripped.startswith('{') and stripped.endswith('}') and slice [1:-1] if true) before appending to self._message.content and updating self._message.message_metadata and await self._message.save() so you preserve inner/legitimate brace characters.
122-127:⚠️ Potential issue | 🟠 MajorBare
except Exceptionsilently swallows errors without logging.This handler catches all exceptions from
Conversation.get()(including unexpected errors like database connection failures) and silently returns. At minimum, log the exception so failures are diagnosable in production.Proposed fix
try: conversation = await Conversation.get(id=conversation_id) - except Exception: + except Exception as e: + logger.warning("Failed to get conversation %s for usage snapshot: %s", conversation_id, e) if finalize: self._clear_usage_buffer(conversation_id, run_id) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/api/services/conversation_persistence.py` around lines 122 - 127, The try/except around Conversation.get(conversation_id) in conversation_persistence.py currently uses a bare except which swallows all exceptions; update it to catch Exception as e (or catch specific DB errors) and log the exception before returning so failures are visible, and keep the existing finalize behavior (call self._clear_usage_buffer(conversation_id, run_id) when finalize is true) followed by the return; ensure the logger used in this module (or self.logger) is invoked with the caught exception details and a clear message referencing Conversation.get and the conversation_id.ui/src/app/analytics/page.tsx (1)
1-1: 🧹 Nitpick | 🔵 Trivial
"use client"is unnecessary on this page component.In Next.js App Router, this page can remain a server component —
NavbarandAnalyticsalready declare"use client"internally. Removing it here avoids pushing the page into the client bundle unnecessarily.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/app/analytics/page.tsx` at line 1, Remove the top-level "use client" directive from the page component so it remains a server component; keep Navbar and Analytics as they are (they already contain "use client"), which prevents this page.tsx from being bundled to the client unnecessarily—open page.tsx, delete the string literal "use client" at the top, and verify Navbar and Analytics imports (Navbar, Analytics) remain unchanged.ui/src/components/analytics/Analytics.tsx (1)
26-68:⚠️ Potential issue | 🟠 MajorRace condition: rapid time-range switches can overwrite data with a stale response.
The
useEffectfiresfetchDataon everytimeRange/dateRangechange but has no cancellation mechanism. If the user switches from 7D → 30D quickly, the slower 7D response can resolve after the 30D response and clobberdatawith stale metrics. This was flagged previously and remains unaddressed.Proposed fix — cancelled-flag pattern
useEffect(() => { + let cancelled = false; const fetchData = async () => { setLoading(true); setError(null); try { let result; if (timeRange === "custom") { if (dateRange?.from && dateRange?.to) { // ... date normalization unchanged ... result = await getMetrics({ startDate: fromDate, endDate: toDate }); } else { - setLoading(false); setData(null); return; } } else { result = await getMetrics(timeRange); } - setData(result); + if (!cancelled) setData(result); } catch (err) { - setError("Failed to load analytics data"); - console.error(err); + if (!cancelled) { + setError("Failed to load analytics data"); + console.error(err); + } } finally { - setLoading(false); + if (!cancelled) setLoading(false); } }; fetchData(); + return () => { cancelled = true; }; }, [timeRange, dateRange]);This also removes the redundant
setLoading(false)on line 51 —finallyalready handles it even on early returns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/analytics/Analytics.tsx` around lines 26 - 68, The fetchData function inside the useEffect (watching timeRange and dateRange) needs a cancellation guard to avoid stale responses overwriting state: introduce an isCancelled flag (or an AbortController) local to the effect, set it when the cleanup runs, and before calling setData/setError/setLoading ensure the response is not cancelled; apply this to both the custom-date and non-custom branches and remove the redundant early setLoading(false) before return since the finally block already handles it. Target identifiers: useEffect -> fetchData, getMetrics, setData, setError, setLoading, dateRange, timeRange.ui/src/components/analytics/TokenUsageChart.tsx (1)
57-57:⚠️ Potential issue | 🟡 Minor
YAxis.tickFormatterreturns anumberfor values below 1000, violating Recharts'stringreturn type.Recharts types
tickFormatteras(value: any, index: number) => string. Whenvalue < 1000, this returnsvaluedirectly (a number), which may cause TypeScript errors and rendering issues.Proposed fix
- tickFormatter={(value) => (value >= 1000 ? `${value / 1000}k` : value)} + tickFormatter={(value) => (value >= 1000 ? `${value / 1000}k` : String(value))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/analytics/TokenUsageChart.tsx` at line 57, The tickFormatter passed to YAxis in TokenUsageChart.tsx returns a number for values < 1000 which violates Recharts' expected string return; update the YAxis.tickFormatter (the arrow function currently written as (value) => (value >= 1000 ? `${value / 1000}k` : value)) to always return a string by converting the numeric branch (e.g., value.toString() or `${value}`) so both branches return strings.ui/src/components/analytics/CostTrendChart.tsx (1)
33-58:⚠️ Potential issue | 🟠 MajorTimezone-shift bug:
new Date("YYYY-MM-DD")parses as UTC, showing wrong day for negative-UTC users.This was identified and fixed in
TokenUsageChart.tsx(string-splitting approach) but the same fix was not applied here. Lines 36 and 58 still usenew Date(value)which interprets"YYYY-MM-DD"as UTC midnight, causing the displayed date to shift back one day for users west of UTC.Proposed fix
<XAxis dataKey="date" tickFormatter={(value) => { - const date = new Date(value); - return `${date.getMonth() + 1}/${date.getDate()}`; + const [, mm, dd] = String(value).split("-"); + return `${Number(mm)}/${Number(dd)}`; }} stroke="#9ca3af" tick={{ fill: "#9ca3af", fontSize: 12 }} tickLine={false} axisLine={false} /> ... <Tooltip ... - labelFormatter={(label) => new Date(label).toLocaleDateString()} + labelFormatter={(label) => String(label)} formatter={(value: number) => [`$${value.toFixed(4)}`, "Cost"]} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/analytics/CostTrendChart.tsx` around lines 33 - 58, The XAxis.tickFormatter and Tooltip.labelFormatter in CostTrendChart.tsx are creating dates with new Date(value), which parses "YYYY-MM-DD" as UTC and causes a timezone-shift; update both places to parse the "YYYY-MM-DD" string by splitting into year, month, day and constructing a local Date via new Date(year, monthIndex, day) (the same string-splitting approach used in TokenUsageChart.tsx) so the displayed tick and tooltip dates are correct for local time.engine/src/api/endpoints/analytics.py (2)
228-233:⚠️ Potential issue | 🟠 MajorExposing
str(e)in the HTTP 500 response detail leaks internal error information.This was flagged in previous review rounds. The
logger.exception()call already captures the full error server-side; the client response should use a generic message to avoid leaking stack traces, file paths, or SQL details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/api/endpoints/analytics.py` around lines 228 - 233, The except block in engine/src/api/endpoints/analytics.py currently includes the internal error text in the HTTP 500 response; keep the existing logger.exception(...) to capture full details server-side but change the HTTPException raised in the except Exception as e handler so its detail is a generic message (e.g., "Internal server error while fetching metrics") instead of using str(e); update the except block that surrounds the metrics retrieval (the except Exception as e handling in this module) to raise HTTPException(status_code=500, detail="<generic message>") while retaining logger.exception for diagnostics.
55-55: 🧹 Nitpick | 🔵 TrivialRedundant
if last_n_days else 30fallback.
last_n_daysis declared withQuery(default=30, ge=1, le=365), so FastAPI guarantees it is always at least1— it can never beNoneor0at this point. The ternary is dead code.- days = last_n_days if last_n_days else 30 + days = last_n_days🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/api/endpoints/analytics.py` at line 55, The ternary fallback is dead code because FastAPI's Query(default=30, ge=1, le=365) guarantees last_n_days is always present; replace the redundant assignment in analytics endpoint by using last_n_days directly (e.g., remove "days = last_n_days if last_n_days else 30" and use last_n_days or assign days = last_n_days) so the function/method handling last_n_days (reference variable last_n_days and the line assigning days) is simplified accordingly.ui/src/lib/api.ts (1)
99-100:⚠️ Potential issue | 🟡 MinorTrailing
?in URL when no params are set.When
optionsis{}(all fieldsundefined),params.toString()returns"", producing…/analytics/metrics?. While harmless for most HTTP clients, it is unnecessary noise.- `${process.env.API_URL}/analytics/metrics?${queryParams}`, + `${process.env.API_URL}/analytics/metrics${queryParams ? `?${queryParams}` : ""}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/lib/api.ts` around lines 99 - 100, The request URL construction appends a trailing `?` when queryParams is empty; update the fetch call in ui/src/lib/api.ts to only include the `?${queryParams}` fragment when queryParams (the result of params.toString()) is non-empty—i.e., compute the URL with `${process.env.API_URL}/analytics/metrics${queryParams ? `?${queryParams}` : ''}` or equivalent and use that in the `fetch` call so no trailing `?` is emitted when params.toString() returns "".ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx (1)
22-26:⚠️ Potential issue | 🟡 Minor
||instead of??for null guard;avg_ttr_msoverride is dead code.
d.avg_ttft_ms || 0coerces0(valid zero-latency) the same asnull/undefined. Use??to guard only againstnull/undefined. Additionally,avg_ttr_msat line 25 overrides the spread value butavg_ttr_msis never rendered as adataKeyin this chart — the override is dead code that...dalready carries.const chartData = data.map((d) => ({ ...d, - avg_ttft_ms: d.avg_ttft_ms || 0, - avg_ttr_ms: d.avg_ttr_ms || 0, + avg_ttft_ms: d.avg_ttft_ms ?? 0, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx` around lines 22 - 26, The chartData mapping currently uses the wrong null-guard and contains a dead override: in the data.map that builds chartData change the fallback from using || to the nullish coalescing operator (use avg_ttft_ms: d.avg_ttft_ms ?? 0) so legitimate zero values aren’t clobbered, and remove the redundant avg_ttr_ms: d.avg_ttr_ms override (the spread ...d already preserves it and avg_ttr_ms is not used as a dataKey in this chart); update the mapping in TimetoFirstTokenAreaChart.tsx where chartData is created.ui/src/components/analytics/TotalResponseTimeAreaChart.tsx (1)
24-24:⚠️ Potential issue | 🟡 Minor
||instead of??for null guard.
d.avg_ttr_ms || 0coerces0(a valid zero-latency measurement) the same asnull/undefined. Use??to guard onlynull/undefined.- avg_ttr_ms: d.avg_ttr_ms || 0, + avg_ttr_ms: d.avg_ttr_ms ?? 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/components/analytics/TotalResponseTimeAreaChart.tsx` at line 24, The null-guard for avg_ttr_ms uses || which treats 0 as falsy; update the mapping that sets avg_ttr_ms (where you currently have d.avg_ttr_ms || 0) to use the nullish coalescing operator instead (d.avg_ttr_ms ?? 0) so that legitimate zero values are preserved; locate this change in the data-mapping for TotalResponseTimeAreaChart (the avg_ttr_ms assignment) and replace the operator accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/src/api/endpoints/analytics.py`:
- Around line 28-30: There is only one blank line between the top-level helper
_empty_daily_stats and the route decorator `@router.get` causing Ruff/PEP8
failure; add one additional blank line so there are two blank lines separating
the top-level definition of _empty_daily_stats and the `@router.get-decorated`
function to satisfy the formatter.
- Line 45: Remove the dead initial assignment period_end_dt = datetime.now() in
analytics.py (the variable is always reassigned inside the subsequent if
start_date branches), so delete that line and rely on the assignments within the
start_date conditional (ensure period_end_dt is still set in both the start_date
True and False branches such as the assignments at the two locations that
currently overwrite it).
- Around line 62-66: The Message.query fetching code currently calls
Message.filter(...).distinct().prefetch_related('conversation') which is
unnecessary; remove both .distinct() and .prefetch_related('conversation') from
the Message.filter call so you simply await
Message.filter(conversation__user=user, created_at__gte=period_start,
created_at__lte=period_end) and rely on msg.conversation_id (used later) rather
than loading the Conversation relation.
In `@engine/src/api/models/conversation.py`:
- Around line 131-139: DailyMetrics declares avg_ttft_ms and avg_ttr_ms as
Optional[float] but doesn't provide default values, forcing callers to always
pass them; update the DailyMetrics datamodel (class DailyMetrics) to set
avg_ttft_ms = None and avg_ttr_ms = None so they are optional at construction
(matching TokenUsageMetrics pattern) and ensure any validators/serialization
still handle None values.
- Around line 141-158: Update the MetricsAggregation Pydantic model so its
Optional fields have explicit defaults: set avg_ttft_ms = None, avg_ttr_ms =
None, and approval_acceptance_rate = None on the MetricsAggregation class; leave
all other fields unchanged and do not add cost_change_pct or tokens_change_pct
as they are not required. This ensures Pydantic treats those Optional[float]
fields as optional at runtime and matches frontend expectations.
In `@engine/src/api/services/conversation_persistence.py`:
- Around line 238-244: When run_id is present the code assumes self._message
exists but append_tool_segment may have left it None, causing AttributeError at
self._message.token_usage; update the logic in append_text_segment (or the block
that handles usage snapshot) to guard for or create a Message before assigning
token_usage: check if self._message is None and either create/initialize the
Message row (matching how messages are created elsewhere) or skip assigning
token usage, then apply TokenUsageMetrics(**usage_snapshot).model_dump() and
await self._message.save() only when self._message is non-None; use the existing
symbols _snapshot_usage, TokenUsageMetrics, self._message, append_tool_segment,
and save to locate and implement the fix.
In `@ui/design_system.md`:
- Line 15: Standardize the hex casing for the dark.active color entry by
changing `#1B1B1C` to lowercase `#1b1b1c` so it matches the rest of the table;
locate the table row that defines `dark.active` and update the hex value to
`#1b1b1c`.
In `@ui/package.json`:
- Line 40: Update the prop name passed to the Calendar component in the
DatePickerWithRange component: replace any use of the deprecated initialFocus
prop with the v9-compatible autoFocus prop (e.g., in DatePickerWithRange.tsx
where Calendar is rendered around the Calendar component calls at the two call
sites currently using initialFocus). Ensure the prop value and type remain the
same when switching names so Calendar receives autoFocus instead of
initialFocus.
In `@ui/src/components/analytics/Analytics.tsx`:
- Around line 111-134: The outer and inner divs in the Analytics component both
apply the same bg-gradient-to-br from-blue-600/10 to-transparent, causing
redundant overlapping gradients; refactor by merging these two wrappers into a
single container (keep the backdrop-blur-md and shadow-lg visual effects plus
rounded-xl and p-8 on that single element) and remove the redundant gradient
class from the other element or eliminate the extra div entirely so children
like DatePickerWithRange and TimeRangeSelector remain unchanged; update the JSX
where the two nested divs are defined in the Analytics component to use one
consolidated wrapper.
In `@ui/src/components/analytics/AnimatedCounter.tsx`:
- Around line 43-47: The effect is restarting because match (from
value.match(...)) is a new array each render; change the dependency to a stable
primitive instead of the array. Create a stable key (e.g., const matchKey =
match ? match[0] : null or const hasMatch = !!match) near where match is
computed, then update the useEffect dependency array to use matchKey (or
hasMatch) along with parsedValue and count; keep the effect body the same (if
(match && !isNaN(parsedValue)) { count.set(parsedValue); }) but reference the
newly created primitive in the dependency list so the spring animation no longer
restarts on parent re-renders.
In `@ui/src/components/analytics/DatePickerWithRange.tsx`:
- Around line 65-67: The Calendar component is using the removed prop
initialFocus (in DatePickerWithRange.tsx) which causes TypeScript/build errors
or broken focus behavior with react-day-picker v9; remove initialFocus and
replace it with the v9-supported autoFocus prop on the Calendar (or
alternatively pin react-day-picker to v8 until full migration), ensure the
change is applied to all Calendar usages in this file (including the occurrences
around lines noted) so keyboard focus on open continues to work.
In `@ui/src/components/analytics/MetricCard.tsx`:
- Around line 16-21: Extract the inline variants object used in the MetricCard
motion.div into a module-level constant (e.g., METRIC_CARD_VARIANTS) and update
the motion.div in the MetricCard component to reference that constant instead of
creating the object inline; also add explicit initial="hidden" and
animate="visible" props to the motion.div (in MetricCard) so the card is
self-contained and reusable without relying on parent orchestration. Ensure the
constant is declared at top-level of the module and referenced by name in the
motion.div within the MetricCard component.
In `@ui/src/components/analytics/TotalResponseTimeAreaChart.tsx`:
- Line 21: The exported component name is inconsistent: the file is
TotalResponseTimeAreaChart.tsx and the function is exported as
TotalResponseTimeLineChart while it renders an <AreaChart>; rename the exported
function to TotalResponseTimeAreaChart (and update any references/imports) so
the component name matches the file and chart type, ensuring the function
declaration and default export use the new name.
---
Outside diff comments:
In `@engine/src/api/models/conversation.py`:
- Around line 1-158: Ruff reports the file is unformatted; run ruff format (or
apply equivalent stylistic changes) on the Conversation/Message models file to
satisfy the formatter—ensure formatting fixes touch the top-level model classes
(Conversation, Message), the Pydantic models (TokenUsageMetrics, MessageRead,
ConversationRead, MetricsAggregation), and import block so that ruff no longer
reports "Would reformat"; after formatting, re-run ruff check to confirm the CI
error is resolved.
---
Duplicate comments:
In `@engine/src/api/endpoints/analytics.py`:
- Around line 228-233: The except block in engine/src/api/endpoints/analytics.py
currently includes the internal error text in the HTTP 500 response; keep the
existing logger.exception(...) to capture full details server-side but change
the HTTPException raised in the except Exception as e handler so its detail is a
generic message (e.g., "Internal server error while fetching metrics") instead
of using str(e); update the except block that surrounds the metrics retrieval
(the except Exception as e handling in this module) to raise
HTTPException(status_code=500, detail="<generic message>") while retaining
logger.exception for diagnostics.
- Line 55: The ternary fallback is dead code because FastAPI's Query(default=30,
ge=1, le=365) guarantees last_n_days is always present; replace the redundant
assignment in analytics endpoint by using last_n_days directly (e.g., remove
"days = last_n_days if last_n_days else 30" and use last_n_days or assign days =
last_n_days) so the function/method handling last_n_days (reference variable
last_n_days and the line assigning days) is simplified accordingly.
In `@engine/src/api/services/conversation_persistence.py`:
- Around line 206-214: The guard using only "if not self._message" can reuse a
stale Message from a previous conversation; change the condition in the block
that calls _get_next_sequence and Message.create to also check conversation
ownership (e.g., if not self._message or getattr(self._message,
'conversation_id', None) != conversation.id) so a new Message is created
whenever the cached self._message does not belong to the current conversation;
reference the existing symbols self._message, _get_next_sequence, and
Message.create and optionally call _clear_message() before creating a new
message to ensure no stale state remains.
- Around line 232-236: The use of text.strip('{}') in the block that updates
self._message.content is too aggressive and removes all leading/trailing braces;
change it to first trim whitespace, then only remove one wrapping pair if
present (e.g., check stripped.startswith('{') and stripped.endswith('}') and
slice [1:-1] if true) before appending to self._message.content and updating
self._message.message_metadata and await self._message.save() so you preserve
inner/legitimate brace characters.
- Around line 122-127: The try/except around Conversation.get(conversation_id)
in conversation_persistence.py currently uses a bare except which swallows all
exceptions; update it to catch Exception as e (or catch specific DB errors) and
log the exception before returning so failures are visible, and keep the
existing finalize behavior (call self._clear_usage_buffer(conversation_id,
run_id) when finalize is true) followed by the return; ensure the logger used in
this module (or self.logger) is invoked with the caught exception details and a
clear message referencing Conversation.get and the conversation_id.
In `@ui/design_system.md`:
- Line 5: Several markdown headings (e.g., "### Base Colors (Tailwind Config)"
and the other four `###` subheadings) are missing the required blank line after
the heading; insert a single blank line immediately after each `###` heading so
the heading is followed by an empty line before the next content paragraph or
list to satisfy MD022 (apply this change to every `###` subheading in the
document).
In `@ui/package.json`:
- Line 66: The package.json pins TypeScript to an exact patch "typescript":
"5.9.3" which may not exist on npm; verify that version is published and if it
is missing replace the exact pin with a safe specifier (e.g., "^5.9.3" or an
existing published patch like "5.9.x") or update to the actual published patch
version; locate the dependency entry "typescript": "5.9.3" in ui/package.json,
run the registry check (or change to a caret range) and update package.json
accordingly, then re-run install to confirm the build succeeds.
In `@ui/src/app/analytics/page.tsx`:
- Line 1: Remove the top-level "use client" directive from the page component so
it remains a server component; keep Navbar and Analytics as they are (they
already contain "use client"), which prevents this page.tsx from being bundled
to the client unnecessarily—open page.tsx, delete the string literal "use
client" at the top, and verify Navbar and Analytics imports (Navbar, Analytics)
remain unchanged.
In `@ui/src/components/analytics/Analytics.tsx`:
- Around line 26-68: The fetchData function inside the useEffect (watching
timeRange and dateRange) needs a cancellation guard to avoid stale responses
overwriting state: introduce an isCancelled flag (or an AbortController) local
to the effect, set it when the cleanup runs, and before calling
setData/setError/setLoading ensure the response is not cancelled; apply this to
both the custom-date and non-custom branches and remove the redundant early
setLoading(false) before return since the finally block already handles it.
Target identifiers: useEffect -> fetchData, getMetrics, setData, setError,
setLoading, dateRange, timeRange.
In `@ui/src/components/analytics/CostTrendChart.tsx`:
- Around line 33-58: The XAxis.tickFormatter and Tooltip.labelFormatter in
CostTrendChart.tsx are creating dates with new Date(value), which parses
"YYYY-MM-DD" as UTC and causes a timezone-shift; update both places to parse the
"YYYY-MM-DD" string by splitting into year, month, day and constructing a local
Date via new Date(year, monthIndex, day) (the same string-splitting approach
used in TokenUsageChart.tsx) so the displayed tick and tooltip dates are correct
for local time.
In `@ui/src/components/analytics/DatePickerWithRange.tsx`:
- Around line 77-81: The disabled predicate is comparing day (midnight-based
calendar Date) to new Date() (current time), causing "today" to be treated as
future; normalize the current date to midnight and use that for comparison. In
DatePickerWithRange.tsx, compute a startOfToday by doing const today = new
Date(); today.setHours(0,0,0,0); then replace checks like day > new Date() with
day > today in both the start-date disabled predicate (the disabled={(day) => {
... }} used for the start picker) and the end-date disabled predicate (the
equivalent disabled predicate used for the end picker) so both use the
normalized today boundary.
- Around line 69-75: The onSelect handler for the start date currently leaves
date.to populated when val is undefined, creating an invalid range; update the
onSelect in the start picker so that when val is falsy (user cleared the start),
you call setDate({ from: undefined, to: undefined }) instead of preserving
date.to, but keep the existing behavior when val is a Date (i.e., if val >
date.to then set { from: val, to: undefined } else set { from: val, to: date?.to
}); ensure setIsStartOpen(false) remains called. This change targets the
onSelect callback that currently references date, setDate, and setIsStartOpen.
- Around line 31-40: The GlassWrapper component is being re-created on every
render inside DatePickerWithRange, causing React to unmount/remount its children
(including <Calendar>) when setIsStartOpen/setIsEndOpen toggle; move
GlassWrapper out of the DatePickerWithRange function to module scope (top-level)
so its identity is stable across renders, keep the same props typing ({
children: React.ReactNode }), and update the JSX usage inside
DatePickerWithRange to reference the top-level GlassWrapper; no other behavioral
changes needed.
In `@ui/src/components/analytics/TimeRangeSelector.tsx`:
- Around line 12-17: Move the static ranges constant out of the
TimeRangeSelector component so it isn't recreated on every render: define the
const ranges: { label: string; value: number | "custom" }[] above the
TimeRangeSelector function (at module scope) and keep the same name and shape so
existing references inside TimeRangeSelector continue to work; no behavioral
changes are needed beyond relocating the declaration.
- Around line 38-48: In the TimeRangeSelector component, remove the unnecessary
key={"custom"} prop from the standalone Button element and replace the JSX
string expression {"Custom"} with the plain text Custom; update the Button JSX
(the one using variant={selectedRange === "custom" ? "default" : "ghost"} and
onClick={() => onRangeChange("custom")}) accordingly so it no longer includes
the spurious key and uses the unwrapped text node.
- Line 20: The wrapper div in TimeRangeSelector uses the non-existent Tailwind
class "bg-dark-card" so the selector has no background; update the className on
the div in TimeRangeSelector.tsx (the line containing the string "flex
bg-dark-card border ...") to use a valid background token from your Tailwind
config (for example replace bg-dark-card with the project's defined dark token
such as bg-dark-700 or the equivalent token you find via rg "dark" in
ui/tailwind.config.ts), ensuring the rest of the class string remains unchanged.
In `@ui/src/components/analytics/TimetoFirstTokenAreaChart.tsx`:
- Around line 22-26: The chartData mapping currently uses the wrong null-guard
and contains a dead override: in the data.map that builds chartData change the
fallback from using || to the nullish coalescing operator (use avg_ttft_ms:
d.avg_ttft_ms ?? 0) so legitimate zero values aren’t clobbered, and remove the
redundant avg_ttr_ms: d.avg_ttr_ms override (the spread ...d already preserves
it and avg_ttr_ms is not used as a dataKey in this chart); update the mapping in
TimetoFirstTokenAreaChart.tsx where chartData is created.
In `@ui/src/components/analytics/TokenUsageChart.tsx`:
- Line 57: The tickFormatter passed to YAxis in TokenUsageChart.tsx returns a
number for values < 1000 which violates Recharts' expected string return; update
the YAxis.tickFormatter (the arrow function currently written as (value) =>
(value >= 1000 ? `${value / 1000}k` : value)) to always return a string by
converting the numeric branch (e.g., value.toString() or `${value}`) so both
branches return strings.
In `@ui/src/components/analytics/TotalResponseTimeAreaChart.tsx`:
- Line 24: The null-guard for avg_ttr_ms uses || which treats 0 as falsy; update
the mapping that sets avg_ttr_ms (where you currently have d.avg_ttr_ms || 0) to
use the nullish coalescing operator instead (d.avg_ttr_ms ?? 0) so that
legitimate zero values are preserved; locate this change in the data-mapping for
TotalResponseTimeAreaChart (the avg_ttr_ms assignment) and replace the operator
accordingly.
In `@ui/src/lib/api.ts`:
- Around line 99-100: The request URL construction appends a trailing `?` when
queryParams is empty; update the fetch call in ui/src/lib/api.ts to only include
the `?${queryParams}` fragment when queryParams (the result of
params.toString()) is non-empty—i.e., compute the URL with
`${process.env.API_URL}/analytics/metrics${queryParams ? `?${queryParams}` :
''}` or equivalent and use that in the `fetch` call so no trailing `?` is
emitted when params.toString() returns "".
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
ui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
engine/src/api/endpoints/__init__.pyengine/src/api/endpoints/analytics.pyengine/src/api/models/conversation.pyengine/src/api/services/conversation_persistence.pyui/design_system.mdui/package.jsonui/src/app/analytics/page.tsxui/src/components/analytics/Analytics.tsxui/src/components/analytics/AnimatedCounter.tsxui/src/components/analytics/CacheEfficiencyGauge.tsxui/src/components/analytics/CostTrendChart.tsxui/src/components/analytics/DatePickerWithRange.tsxui/src/components/analytics/MetricCard.tsxui/src/components/analytics/TimeRangeSelector.tsxui/src/components/analytics/TimetoFirstTokenAreaChart.tsxui/src/components/analytics/TokenUsageChart.tsxui/src/components/analytics/TotalResponseTimeAreaChart.tsxui/src/components/navbar/Navbar.tsxui/src/components/ui/calendar.tsxui/src/components/ui/popover.tsxui/src/lib/api.tsui/src/types/analytics.ts
| } | ||
|
|
||
| @router.get( |
There was a problem hiding this comment.
CI is blocked: missing blank line causes Ruff format failure.
PEP 8 (and Ruff's formatter) requires two blank lines between top-level definitions. There is only one blank line between _empty_daily_stats and the @router.get decorator.
🔧 Proposed fix
}
+
`@router.get`(📝 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.
| } | |
| @router.get( | |
| } | |
| `@router.get`( |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/src/api/endpoints/analytics.py` around lines 28 - 30, There is only
one blank line between the top-level helper _empty_daily_stats and the route
decorator `@router.get` causing Ruff/PEP8 failure; add one additional blank line
so there are two blank lines separating the top-level definition of
_empty_daily_stats and the `@router.get-decorated` function to satisfy the
formatter.
| if not user: | ||
| raise HTTPException(status_code=401, detail="Authentication required for metrics") | ||
|
|
||
| period_end_dt = datetime.now() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Dead initial assignment of period_end_dt.
period_end_dt = datetime.now() at line 45 is always overwritten on both branches of the subsequent if start_date block (lines 53 and 60). The assignment is unreachable as a final value and can be removed.
🧹 Proposed cleanup
- period_end_dt = datetime.now()
-
if start_date:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/src/api/endpoints/analytics.py` at line 45, Remove the dead initial
assignment period_end_dt = datetime.now() in analytics.py (the variable is
always reassigned inside the subsequent if start_date branches), so delete that
line and rely on the assignments within the start_date conditional (ensure
period_end_dt is still set in both the start_date True and False branches such
as the assignments at the two locations that currently overwrite it).
| messages = await Message.filter( | ||
| conversation__user=user, | ||
| created_at__gte=period_start, | ||
| created_at__lte=period_end_dt | ||
| ).distinct().prefetch_related('conversation') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Step 1: Find and inspect the Message model definition
echo "=== Message model definition ==="
rg -n "class Message" --type py -A 40 engine/src
echo ""
echo "=== Usages of msg.conversation or message.conversation in analytics.py ==="
rg -n "msg\.conversation\|message\.conversation" engine/src/api/endpoints/analytics.py
echo ""
echo "=== Full context around lines 62-75 in analytics.py ==="
sed -n '60,80p' engine/src/api/endpoints/analytics.pyRepository: skyflo-ai/skyflo
Length of output: 6544
Remove prefetch_related('conversation') and .distinct() — both are unnecessary and waste resources.
msg.conversation_id (line 68) is a raw FK field available without prefetching; Tortoise ORM automatically exposes it without loading the related Conversation object. Since Message has a 1-N relationship with Conversation, each message row appears exactly once in the result set — no fan-out occurs, so .distinct() is also redundant.
🔧 Proposed fix
messages = await Message.filter(
conversation__user=user,
created_at__gte=period_start,
created_at__lte=period_end_dt
- ).distinct().prefetch_related('conversation')
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/src/api/endpoints/analytics.py` around lines 62 - 66, The
Message.query fetching code currently calls
Message.filter(...).distinct().prefetch_related('conversation') which is
unnecessary; remove both .distinct() and .prefetch_related('conversation') from
the Message.filter call so you simply await
Message.filter(conversation__user=user, created_at__gte=period_start,
created_at__lte=period_end) and rely on msg.conversation_id (used later) rather
than loading the Conversation relation.
| class DailyMetrics(BaseModel): | ||
| date: date | ||
| cost: float | ||
| prompt_tokens: int | ||
| completion_tokens: int | ||
| cached_tokens: int | ||
| total_tokens: int | ||
| avg_ttft_ms: Optional[float] | ||
| avg_ttr_ms: Optional[float] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Provide defaults for Optional fields in DailyMetrics.
avg_ttft_ms and avg_ttr_ms are typed Optional[float] but have no default value, so callers must always explicitly pass them (even as None). Adding = None matches the pattern used in TokenUsageMetrics (lines 66-67) and makes constructing DailyMetrics less error-prone when latency data is absent for a given day.
Proposed fix
class DailyMetrics(BaseModel):
date: date
cost: float
prompt_tokens: int
completion_tokens: int
cached_tokens: int
total_tokens: int
- avg_ttft_ms: Optional[float]
- avg_ttr_ms: Optional[float]
+ avg_ttft_ms: Optional[float] = None
+ avg_ttr_ms: Optional[float] = None📝 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.
| class DailyMetrics(BaseModel): | |
| date: date | |
| cost: float | |
| prompt_tokens: int | |
| completion_tokens: int | |
| cached_tokens: int | |
| total_tokens: int | |
| avg_ttft_ms: Optional[float] | |
| avg_ttr_ms: Optional[float] | |
| class DailyMetrics(BaseModel): | |
| date: date | |
| cost: float | |
| prompt_tokens: int | |
| completion_tokens: int | |
| cached_tokens: int | |
| total_tokens: int | |
| avg_ttft_ms: Optional[float] = None | |
| avg_ttr_ms: Optional[float] = None |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/src/api/models/conversation.py` around lines 131 - 139, DailyMetrics
declares avg_ttft_ms and avg_ttr_ms as Optional[float] but doesn't provide
default values, forcing callers to always pass them; update the DailyMetrics
datamodel (class DailyMetrics) to set avg_ttft_ms = None and avg_ttr_ms = None
so they are optional at construction (matching TokenUsageMetrics pattern) and
ensure any validators/serialization still handle None values.
| class MetricsAggregation(BaseModel): | ||
| period_start: datetime | ||
| period_end: datetime | ||
| total_cost: float | ||
| total_tokens: int | ||
| total_prompt_tokens: int | ||
| total_completion_tokens: int | ||
| total_cached_tokens: int | ||
| total_conversations: int | ||
| avg_ttft_ms: Optional[float] | ||
| avg_ttr_ms: Optional[float] | ||
| avg_cost_per_conversation: float | ||
| avg_tokens_per_conversation: float | ||
| cache_hit_rate: float | ||
| daily_breakdown: List[DailyMetrics] | ||
| total_approvals: int | ||
| total_rejections: int | ||
| approval_acceptance_rate: Optional[float] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the analytics endpoint constructs MetricsAggregation with cost_change_pct/tokens_change_pct
echo "=== Check analytics endpoint for cost_change_pct and tokens_change_pct ==="
rg -n "cost_change_pct\|tokens_change_pct" --type py engine/src/
echo ""
echo "=== Check frontend type definition ==="
rg -n "cost_change_pct\|tokens_change_pct" --type ts ui/src/Repository: skyflo-ai/skyflo
Length of output: 174
🏁 Script executed:
cat -n engine/src/api/models/conversation.py | sed -n '141,158p'Repository: skyflo-ai/skyflo
Length of output: 742
🏁 Script executed:
rg -n "class DailyMetrics" --type py engine/src/ -A 15Repository: skyflo-ai/skyflo
Length of output: 1116
🏁 Script executed:
# Look for PR or requirements context
find . -name "*.md" -type f | xargs rg -l "period-over-period\|cost_change\|tokens_change" 2>/dev/null | head -10
# Check for analytics endpoint
rg -n "MetricsAggregation" --type py engine/src/ -B 5 -A 10Repository: skyflo-ai/skyflo
Length of output: 5178
🏁 Script executed:
cat -n engine/src/api/endpoints/analytics.py | sed -n '206,230p'Repository: skyflo-ai/skyflo
Length of output: 1254
🏁 Script executed:
# Look for PR or commit context
find . -name "*.md" -o -name "PULL_REQUEST*" -o -name ".github" -type d | head -20
# Check for any references to period-over-period or metrics changes in recent context
rg -i "period.over.period\|change.pct\|trend" --type md --type py engine/src/api/endpoints/analytics.pyRepository: skyflo-ai/skyflo
Length of output: 341
🏁 Script executed:
cat .github/PULL_REQUEST_TEMPLATE.md
echo ""
echo "=== Check recent commits to analytics/metrics files ==="
git log --oneline -n 20 -- engine/src/api/endpoints/analytics.py engine/src/api/models/conversation.py 2>/dev/null || echo "Git history not available"Repository: skyflo-ai/skyflo
Length of output: 1291
🏁 Script executed:
# Search for MetricsAggregation in frontend
rg -n "MetricsAggregation\|interface.*Metrics" --type ts ui/src/ -A 20 | head -100
# Also check for any analytics-related types
rg -n "cost_change\|tokens_change\|period.*over.*period" --type ts ui/src/Repository: skyflo-ai/skyflo
Length of output: 42
🏁 Script executed:
# Check directory structure of ui/src
find ui/src -type f -name "*.ts" -o -name "*.tsx" | grep -i metric | head -20
# Broader search for API types
rg -n "interface\|type.*Metric" --type ts ui/src/ | head -30
# Check if there's an API client or types folder
ls -la ui/src/api/ 2>/dev/null || ls -la ui/src/types/ 2>/dev/null || echo "No api/types folder found"Repository: skyflo-ai/skyflo
Length of output: 449
🏁 Script executed:
cat ui/src/api/analytics.tsRepository: skyflo-ai/skyflo
Length of output: 115
🏁 Script executed:
ls -la ui/src/api/
# Search for where api calls are made for analytics
rg -n "metrics\|analytics" --type ts ui/src/ | head -30Repository: skyflo-ai/skyflo
Length of output: 118
🏁 Script executed:
# Check ui directory structure
find ui/src -type f -name "*.ts" -o -name "*.tsx" | head -30
# Search for analytics or metrics references in frontend
rg -n "analytics\|metrics" --type ts ui/src/ -l | head -20Repository: skyflo-ai/skyflo
Length of output: 1132
🏁 Script executed:
# Check the analytics page
cat ui/src/app/analytics/page.tsx
# Also check the cost trend chart
cat ui/src/components/analytics/CostTrendChart.tsxRepository: skyflo-ai/skyflo
Length of output: 3226
🏁 Script executed:
# Find the types/analytics file
find ui/src -path "*types*analytics*" -o -name "analytics.ts" | grep -v node_modules
# More broadly search for types directory
find ui/src -type d -name "types" -o -type d -name "@types"
# Look for where DailyMetrics is defined
rg -n "DailyMetrics\|MetricsAggregation" --type ts ui/src/ -B 2 -A 10 | head -100Repository: skyflo-ai/skyflo
Length of output: 98
🏁 Script executed:
cat ui/src/types/analytics.tsRepository: skyflo-ai/skyflo
Length of output: 874
Remove the claim about missing cost_change_pct and tokens_change_pct fields—they are not part of the current requirements.
The frontend type definition in ui/src/types/analytics.ts does not include cost_change_pct or tokens_change_pct either, and there is no evidence in the codebase that these fields are part of the PR objectives or acceptance criteria. The backend and frontend models are consistent.
However, the Optional field defaults issue is valid. Add = None to the three Optional fields (avg_ttft_ms, avg_ttr_ms, approval_acceptance_rate) for consistency with Pydantic best practices:
Proposed fix
class MetricsAggregation(BaseModel):
period_start: datetime
period_end: datetime
total_cost: float
total_tokens: int
total_prompt_tokens: int
total_completion_tokens: int
total_cached_tokens: int
total_conversations: int
- avg_ttft_ms: Optional[float]
- avg_ttr_ms: Optional[float]
+ avg_ttft_ms: Optional[float] = None
+ avg_ttr_ms: Optional[float] = None
avg_cost_per_conversation: float
avg_tokens_per_conversation: float
cache_hit_rate: float
daily_breakdown: List[DailyMetrics]
total_approvals: int
total_rejections: int
- approval_acceptance_rate: Optional[float]
+ approval_acceptance_rate: Optional[float] = None📝 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.
| class MetricsAggregation(BaseModel): | |
| period_start: datetime | |
| period_end: datetime | |
| total_cost: float | |
| total_tokens: int | |
| total_prompt_tokens: int | |
| total_completion_tokens: int | |
| total_cached_tokens: int | |
| total_conversations: int | |
| avg_ttft_ms: Optional[float] | |
| avg_ttr_ms: Optional[float] | |
| avg_cost_per_conversation: float | |
| avg_tokens_per_conversation: float | |
| cache_hit_rate: float | |
| daily_breakdown: List[DailyMetrics] | |
| total_approvals: int | |
| total_rejections: int | |
| approval_acceptance_rate: Optional[float] | |
| class MetricsAggregation(BaseModel): | |
| period_start: datetime | |
| period_end: datetime | |
| total_cost: float | |
| total_tokens: int | |
| total_prompt_tokens: int | |
| total_completion_tokens: int | |
| total_cached_tokens: int | |
| total_conversations: int | |
| avg_ttft_ms: Optional[float] = None | |
| avg_ttr_ms: Optional[float] = None | |
| avg_cost_per_conversation: float | |
| avg_tokens_per_conversation: float | |
| cache_hit_rate: float | |
| daily_breakdown: List[DailyMetrics] | |
| total_approvals: int | |
| total_rejections: int | |
| approval_acceptance_rate: Optional[float] = None |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/src/api/models/conversation.py` around lines 141 - 158, Update the
MetricsAggregation Pydantic model so its Optional fields have explicit defaults:
set avg_ttft_ms = None, avg_ttr_ms = None, and approval_acceptance_rate = None
on the MetricsAggregation class; leave all other fields unchanged and do not add
cost_change_pct or tokens_change_pct as they are not required. This ensures
Pydantic treats those Optional[float] fields as optional at runtime and matches
frontend expectations.
| return ( | ||
| <div className="flex flex-col gap-5 h-full overflow-auto px-2 py-2 bg-dark"> | ||
|
|
||
| <div className="bg-gradient-to-br from-blue-600/10 to-transparent rounded-xl backdrop-blur-md shadow-lg shadow-blue-900/10 mb-8"> | ||
| <div className="flex max-sm:flex-col max-sm:gap-10 justify-between bg-gradient-to-br from-blue-600/10 to-transparent rounded-xl p-8"> | ||
| <div className="max-sm:text-center"> | ||
| <h1 className="text-3xl max-sm:inline-block font-bold bg-gradient-to-r from-sky-400 via-blue-500 to-indigo-400 bg-clip-text text-transparent tracking-tight flex items-center"> | ||
| Analytics | ||
| </h1> | ||
|
|
||
| <p className="text-slate-400 mt-2 sm:text-left"> | ||
| Overview of your AI usage and performance | ||
| </p> | ||
| </div> | ||
|
|
||
| <div className="flex flex-row max-lg:flex-col-reverse items-center gap-4"> | ||
| {timeRange === "custom" && ( | ||
| <DatePickerWithRange date={dateRange} setDate={setDateRange} /> | ||
| )} | ||
| <TimeRangeSelector selectedRange={timeRange} onRangeChange={setTimeRange} /> | ||
| </div> | ||
|
|
||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nested gradient wrapper creates visual redundancy.
Lines 114-115 apply overlapping bg-gradient-to-br from-blue-600/10 to-transparent classes on both the outer <div> and the inner <div>. The inner div also adds rounded-xl p-8, making the outer wrapper serve only as a backdrop-blur-md shadow-lg container. Consider merging into a single element to reduce DOM nesting and avoid the double-gradient rendering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/components/analytics/Analytics.tsx` around lines 111 - 134, The outer
and inner divs in the Analytics component both apply the same bg-gradient-to-br
from-blue-600/10 to-transparent, causing redundant overlapping gradients;
refactor by merging these two wrappers into a single container (keep the
backdrop-blur-md and shadow-lg visual effects plus rounded-xl and p-8 on that
single element) and remove the redundant gradient class from the other element
or eliminate the extra div entirely so children like DatePickerWithRange and
TimeRangeSelector remain unchanged; update the JSX where the two nested divs are
defined in the Analytics component to use one consolidated wrapper.
| useEffect(() => { | ||
| if (match && !isNaN(parsedValue)) { | ||
| count.set(parsedValue); | ||
| } | ||
| }, [parsedValue, count, match]); |
There was a problem hiding this comment.
match in the useEffect dependency array causes the spring animation to restart on every parent re-render.
value.match(...) returns a new array object on every call, so Object.is(prevMatch, currMatch) is always false when the value is numeric. React re-runs the effect on every render, calling count.set(parsedValue) even when parsedValue hasn't changed. During an in-progress animation this restarts the spring from its intermediate position, producing a visible stutter on any parent re-render.
Replace match in the dependency array with a stable primitive derived from it:
🐛 Proposed fix
+ const isValidNumber = match !== null && !isNaN(parsedValue);
+
const rounded = useTransform(count, (latest) => {
- if (!match || isNaN(parsedValue)) {
+ if (!isValidNumber) {
return value;
}
...
});
useEffect(() => {
- if (match && !isNaN(parsedValue)) {
+ if (isValidNumber) {
count.set(parsedValue);
}
- }, [parsedValue, count, match]);
+ }, [parsedValue, count, isValidNumber]);
// Handle conditional rendering
- if (!match || isNaN(parsedValue)) {
+ if (!isValidNumber) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/components/analytics/AnimatedCounter.tsx` around lines 43 - 47, The
effect is restarting because match (from value.match(...)) is a new array each
render; change the dependency to a stable primitive instead of the array. Create
a stable key (e.g., const matchKey = match ? match[0] : null or const hasMatch =
!!match) near where match is computed, then update the useEffect dependency
array to use matchKey (or hasMatch) along with parsedValue and count; keep the
effect body the same (if (match && !isNaN(parsedValue)) {
count.set(parsedValue); }) but reference the newly created primitive in the
dependency list so the spring animation no longer restarts on parent re-renders.
| <Calendar | ||
| initialFocus | ||
| mode="single" |
There was a problem hiding this comment.
initialFocus was removed in react-day-picker v9 — build error or silent breakage.
A GitHub issue explicitly reports that the initialFocus prop is missing from react-day-picker v9. With react-day-picker: ^9.13.0 in package.json, passing initialFocus to <Calendar> will either cause a TypeScript compile error (unknown prop) in strict mode, or silently do nothing — breaking keyboard-focus-on-open UX inside both Popovers.
Remove initialFocus and replace with the v9 autoFocus approach, or pin react-day-picker to ^8 until the Calendar component is migrated to the v9 API (see the separate package.json comment).
🐛 Immediate mitigation — remove the prop
<Calendar
- initialFocus
mode="single"
selected={date?.from} <Calendar
- initialFocus
mode="single"
selected={date?.to}Also applies to: 108-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/components/analytics/DatePickerWithRange.tsx` around lines 65 - 67,
The Calendar component is using the removed prop initialFocus (in
DatePickerWithRange.tsx) which causes TypeScript/build errors or broken focus
behavior with react-day-picker v9; remove initialFocus and replace it with the
v9-supported autoFocus prop on the Calendar (or alternatively pin
react-day-picker to v8 until full migration), ensure the change is applied to
all Calendar usages in this file (including the occurrences around lines noted)
so keyboard focus on open continues to work.
| <motion.div | ||
| variants={{ | ||
| hidden: { y: 20, opacity: 0 }, | ||
| visible: { y: 0, opacity: 1 }, | ||
| }} | ||
| className="bg-navbar rounded-lg border border-slate-700/60 p-8 inline-block transition-colors group" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
fd -t f "MetricCard.tsx" -x cat -n {}Repository: skyflo-ai/skyflo
Length of output: 1545
🏁 Script executed:
fd -t f "Analytics.tsx" -x cat -n {}Repository: skyflo-ai/skyflo
Length of output: 11101
🏁 Script executed:
rg -n "staggerChildren|initial=.*hidden|animate=.*visible" --type=tsxRepository: skyflo-ai/skyflo
Length of output: 87
Extract variants to module-level constant for better reusability and maintainability.
The motion.div currently relies on parent orchestration from Analytics.tsx (which correctly propagates initial="hidden" and animate="visible" via staggerChildren). However, while this works, adding explicit initial and animate props directly to MetricCard would make it self-contained and reusable without requiring parent motion wrapper context.
Additionally, defining variants inline creates a new object reference on each render. Extracting it to a module-level constant is a minor best practice improvement.
♻️ Suggested improvement
+"use client";
+
import React from "react";
import { motion } from "framer-motion";
import { AnimatedCounter } from "./AnimatedCounter";
+
+const cardVariants = {
+ hidden: { y: 20, opacity: 0 },
+ visible: { y: 0, opacity: 1 },
+};
export default function MetricCard({ title, value, icon, color }: MetricCardProps) {
return (
<motion.div
- variants={{
- hidden: { y: 20, opacity: 0 },
- visible: { y: 0, opacity: 1 },
- }}
+ variants={cardVariants}
+ initial="hidden"
+ animate="visible"
className="bg-navbar rounded-lg border border-slate-700/60 p-8 inline-block transition-colors group"
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/components/analytics/MetricCard.tsx` around lines 16 - 21, Extract the
inline variants object used in the MetricCard motion.div into a module-level
constant (e.g., METRIC_CARD_VARIANTS) and update the motion.div in the
MetricCard component to reference that constant instead of creating the object
inline; also add explicit initial="hidden" and animate="visible" props to the
motion.div (in MetricCard) so the card is self-contained and reusable without
relying on parent orchestration. Ensure the constant is declared at top-level of
the module and referenced by name in the motion.div within the MetricCard
component.
| data: DailyMetrics[]; | ||
| } | ||
|
|
||
| export default function TotalResponseTimeLineChart({ data }: TotalResponseTimeLineChartProps) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Component name doesn't match the file name.
The exported function is named TotalResponseTimeLineChart while the file is TotalResponseTimeAreaChart.tsx. The chart also uses <AreaChart>, not a <LineChart>. Align the export name with the file and chart type to avoid confusion.
-export default function TotalResponseTimeLineChart({ data }: TotalResponseTimeLineChartProps) {
+export default function TotalResponseTimeAreaChart({ data }: TotalResponseTimeLineChartProps) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/components/analytics/TotalResponseTimeAreaChart.tsx` at line 21, The
exported component name is inconsistent: the file is
TotalResponseTimeAreaChart.tsx and the function is exported as
TotalResponseTimeLineChart while it renders an <AreaChart>; rename the exported
function to TotalResponseTimeAreaChart (and update any references/imports) so
the component name matches the file and chart type, ensuring the function
declaration and default export use the new name.
|
@nitishbhosale7 run the linter for engine and resolve all coderabbit comments |
2 participants
Description
Please include a summary of the changes and which issue is fixed. Explain the motivation for the changes.
Related Issue(s)
Fixes #71
Type of Change
Testing
Please describe the tests you've added/performed to verify your changes.
Checklist
Screenshots (if applicable)
Additional Notes