Conversation
WalkthroughA new "Job Revenue" metric and associated percentage change were introduced to the dashboard, with supporting backend aggregation logic, a dedicated API endpoint, and frontend display components. The dashboard now fetches metrics from a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant MetricsService
participant Database
User->>Frontend: Loads dashboard
Frontend->>Backend: GET /metrics/dashboard?start=...&end=...
Backend->>MetricsService: aggregateMetric('dashboard', start, end)
MetricsService->>Database: Query job revenue, bank balance, previous period data
Database-->>MetricsService: Returns queried data
MetricsService-->>Backend: Aggregated dashboard metrics (jobRevenue, cashAvailable, changes)
Backend-->>Frontend: JSON metrics response
Frontend->>Frontend: Update state with jobRevenue, jobRevenueChange
Frontend->>User: Render JobRevenueWidget with new data
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/backend/.eslintrc.json (2)
14-17: Consider enabling TypeScript-specific linting rules.While disabling
no-unused-varsandno-emptymay be intentional for development, consider using TypeScript-specific alternatives for better type safety:"rules": { - "no-unused-vars": "off", + "@typescript-eslint/no-unused-vars": ["error", { "argsIgnorePattern": "^_" }], "no-empty": "off" }This allows unused variables with underscore prefix while maintaining type checking.
18-20: Document the D1Database global usage.The
D1Databaseglobal declaration suggests this is a Cloudflare Workers D1 database type. Consider adding a comment to clarify its purpose for future maintainers."globals": { + // Cloudflare Workers D1 Database global type "D1Database": "readonly" }packages/frontend/components/ui/formatters.tsx (2)
3-5: Improve type safety for the Currency component.The
childrenprop should have better type constraints to ensure only numbers are passed, and consider handling edge cases.-export function Currency({ children }: { children: number }) { - return <span>{formatCurrency(children)}</span>; +export function Currency({ children }: { children: number }) { + if (typeof children !== 'number' || !isFinite(children)) { + return <span>--</span>; + } + return <span>{formatCurrency(children)}</span>; }
7-10: Consider consistent prop naming and add accessibility attributes.The
Percentagecomponent usesvalueprop whileCurrencyuseschildren. Consider consistency and add ARIA attributes for better accessibility.-export function Percentage({ value }: { value: number }) { +export function Percentage({ value }: { value: number }) { + if (typeof value !== 'number' || !isFinite(value)) { + return <span className="text-gray-500">--</span>; + } const color = value > 0 ? 'text-green-500' : value < 0 ? 'text-red-500' : 'text-gray-500'; - return <span className={color}>{formatPercentage(value)}</span>; + return ( + <span + className={color} + aria-label={`${value > 0 ? 'Increase' : value < 0 ? 'Decrease' : 'No change'} of ${formatPercentage(value)}`} + > + {formatPercentage(value)} + </span> + ); }packages/frontend/components/widgets/JobRevenueWidget.tsx (2)
4-12: Enhance type safety and add prop validation.The component looks well-structured but could benefit from better TypeScript types and error handling for edge cases.
+interface JobRevenueWidgetProps { + value: number; + change: number; +} + -export default function JobRevenueWidget({ value, change }: { value: number; change: number }) { +export default function JobRevenueWidget({ value, change }: JobRevenueWidgetProps) { + // Handle edge cases for invalid data + const displayValue = typeof value === 'number' && isFinite(value) ? value : 0; + const displayChange = typeof change === 'number' && isFinite(change) ? change : 0; + return ( <Card className="p-4"> <h3 className="text-sm font-medium text-gray-400 mb-2">Job Revenue (Invoiced)</h3> - <h2 className="text-3xl font-semibold"><Currency>{value}</Currency></h2> - <Percentage value={change} /> + <h2 className="text-3xl font-semibold"><Currency>{displayValue}</Currency></h2> + <Percentage value={displayChange} /> </Card> ); }
7-7: Consider semantic HTML improvements.The heading structure could be improved for better accessibility and semantic meaning.
- <h3 className="text-sm font-medium text-gray-400 mb-2">Job Revenue (Invoiced)</h3> + <div className="text-sm font-medium text-gray-400 mb-2" role="heading" aria-level="3"> + Job Revenue (Invoiced) + </div>Or alternatively, ensure the heading level is appropriate for the page structure where this widget is used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/descriptions.csvis excluded by!**/*.csv
📒 Files selected for processing (8)
packages/backend/.eslintrc.json(1 hunks)packages/backend/src/routes/metrics.ts(1 hunks)packages/backend/src/services/metricsAggregation.ts(4 hunks)packages/frontend/components/today-overview.tsx(6 hunks)packages/frontend/components/ui/formatters.tsx(1 hunks)packages/frontend/components/widgets/JobRevenueWidget.tsx(1 hunks)packages/frontend/lib/api.ts(2 hunks)packages/frontend/lib/today-widgets.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/frontend/components/today-overview.tsx (2)
packages/frontend/components/today-widgets/WidgetContainer.tsx (1)
WidgetContainer(17-40)packages/frontend/components/widgets/JobRevenueWidget.tsx (1)
JobRevenueWidget(4-12)
🔇 Additional comments (10)
packages/frontend/lib/today-widgets.ts (1)
3-3: Consider widget ID naming consistency.The new widget ID
'jobrevenue'follows camelCase while some existing widgets use different patterns. The implementation looks correct and follows the established structure.The widget registration is properly implemented and maintains consistency with the existing pattern. The label "Job Revenue (Invoiced)" clearly describes the metric being displayed.
packages/frontend/lib/api.ts (2)
19-20: LGTM! Well-structured schema additions.The new fields
jobRevenueandjobRevenueChangeare properly typed and integrate well with the existing schema structure. The nullable nature ofjobRevenueChangemakes sense as it might not be available when there's insufficient historical data.
178-178: Endpoint change aligns with backend implementation.The updated endpoint path correctly matches the new
/metrics/dashboardroute added in the backend, maintaining consistency across the stack.packages/backend/src/services/metricsAggregation.ts (4)
42-46: Well-structured previous period calculations.The date range calculations for previous periods are implemented correctly and positioned at the top for reuse throughout the dashboard logic. This is a good architectural decision.
190-203: Job revenue logic is correctly implemented.The job revenue calculation properly:
- Sums invoice totals for the current period
- Calculates percentage change vs. previous period
- Handles division by zero scenarios appropriately
205-222: Improved cash available calculation using bank balances.This is a significant improvement over the previous approach. Using the latest bank balance metric (
xero_bank_total) provides a more accurate representation of cash available compared to summing invoices, as it reflects actual bank account balances.
319-320: Proper integration of new metrics into result structure.The new metrics are correctly added to both the result object and the expectedKeys array, ensuring consistent data structure and proper fallback values.
Also applies to: 337-337
packages/frontend/components/today-overview.tsx (3)
19-19: Complete integration of JobRevenueWidget infrastructure.The widget integration is thorough and follows established patterns:
- Proper import and component mapping
- Consistent interface and state structure
- Type-safe implementation
Also applies to: 68-69, 108-108, 133-134
204-205: Robust data handling with fallback values.The data fetching correctly uses nullish coalescing (
??) to provide fallback values, ensuring the component remains stable even if the backend doesn't return these new metrics.
414-421: Well-positioned widget layout adjustment.The new JobRevenueWidget is properly positioned at
x:1, y:0and the burnrate widget is appropriately repositioned tox:2, y:0to maintain a clean grid layout. The widget receives the correct props from the metrics state.Also applies to: 426-426
| api.get('/metrics/dashboard', async (c: Context<{ Bindings: Bindings; Variables: Variables }>) => { | ||
| try { | ||
| const db = c.get('db'); | ||
| const startStr = c.req.query('start'); | ||
| const endStr = c.req.query('end'); | ||
| if (!startStr || !endStr) { | ||
| return c.json({ error: 'Missing start or end' }, 400); | ||
| } | ||
| const start = Date.parse(startStr); | ||
| const end = Date.parse(endStr) + 24 * 60 * 60 * 1000 - 1; | ||
| const res = await aggregateMetric({ db, metric: 'dashboard', start, end }); | ||
| return c.json(res); | ||
| } catch (error) { | ||
| console.error('Error in /metrics/dashboard:', error); | ||
| return c.json({ error: 'Failed to fetch dashboard metrics' }, 500); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding retry logic and date validation for consistency.
The new dashboard route looks well-structured but is missing two features present in the main metrics route:
-
Missing retry logic: The main
/metricsroute usesfetchWithIngestionRetryto handle data freshness and trigger N8N workflows. Consider using the same pattern for consistency. -
Date validation: The route doesn't validate date formats before parsing, which could cause runtime errors.
Apply this diff to add date validation:
api.get('/metrics/dashboard', async (c: Context<{ Bindings: Bindings; Variables: Variables }>) => {
try {
const db = c.get('db');
const startStr = c.req.query('start');
const endStr = c.req.query('end');
if (!startStr || !endStr) {
return c.json({ error: 'Missing start or end' }, 400);
}
+ const start = Date.parse(startStr);
+ const end = Date.parse(endStr);
+ if (isNaN(start) || isNaN(end)) {
+ return c.json({ error: 'Invalid date format' }, 400);
+ }
- const start = Date.parse(startStr);
- const end = Date.parse(endStr) + 24 * 60 * 60 * 1000 - 1;
+ const endInclusive = end + 24 * 60 * 60 * 1000 - 1;
- const res = await aggregateMetric({ db, metric: 'dashboard', start, end });
+ const res = await aggregateMetric({ db, metric: 'dashboard', start, end: endInclusive });
return c.json(res);
} catch (error) {
console.error('Error in /metrics/dashboard:', error);
return c.json({ error: 'Failed to fetch dashboard metrics' }, 500);
}
});Consider also using fetchWithIngestionRetry like the main metrics route for data freshness consistency.
📝 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.
| api.get('/metrics/dashboard', async (c: Context<{ Bindings: Bindings; Variables: Variables }>) => { | |
| try { | |
| const db = c.get('db'); | |
| const startStr = c.req.query('start'); | |
| const endStr = c.req.query('end'); | |
| if (!startStr || !endStr) { | |
| return c.json({ error: 'Missing start or end' }, 400); | |
| } | |
| const start = Date.parse(startStr); | |
| const end = Date.parse(endStr) + 24 * 60 * 60 * 1000 - 1; | |
| const res = await aggregateMetric({ db, metric: 'dashboard', start, end }); | |
| return c.json(res); | |
| } catch (error) { | |
| console.error('Error in /metrics/dashboard:', error); | |
| return c.json({ error: 'Failed to fetch dashboard metrics' }, 500); | |
| } | |
| }); | |
| api.get('/metrics/dashboard', async (c: Context<{ Bindings: Bindings; Variables: Variables }>) => { | |
| try { | |
| const db = c.get('db'); | |
| const startStr = c.req.query('start'); | |
| const endStr = c.req.query('end'); | |
| if (!startStr || !endStr) { | |
| return c.json({ error: 'Missing start or end' }, 400); | |
| } | |
| const start = Date.parse(startStr); | |
| const end = Date.parse(endStr); | |
| if (isNaN(start) || isNaN(end)) { | |
| return c.json({ error: 'Invalid date format' }, 400); | |
| } | |
| const endInclusive = end + 24 * 60 * 60 * 1000 - 1; | |
| const res = await aggregateMetric({ db, metric: 'dashboard', start, end: endInclusive }); | |
| return c.json(res); | |
| } catch (error) { | |
| console.error('Error in /metrics/dashboard:', error); | |
| return c.json({ error: 'Failed to fetch dashboard metrics' }, 500); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/backend/src/routes/metrics.ts around lines 236 to 252, the
/metrics/dashboard route lacks date format validation and retry logic present in
the main /metrics route. To fix this, add validation to ensure the 'start' and
'end' query parameters are valid date strings before parsing to prevent runtime
errors. Also, replace the direct call to aggregateMetric with
fetchWithIngestionRetry to maintain data freshness and trigger ingestion
workflows consistently, mirroring the main metrics route's behavior.
1 participant
Summary
Testing
pnpm lintpnpm typecheckpnpm build(fails: Failed to fetchInterfrom Google Fonts)pnpm devSummary by CodeRabbit
New Features
Improvements
Bug Fixes