Add quotes table and update conversion rate metric#25
Add quotes table and update conversion rate metric#25
Conversation
WalkthroughThe changes introduce a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Frontend
participant API as Backend API
participant DB as Database
Client->>API: POST /quotes [{id, jobId, sentDate, amount, status}, ...]
API->>DB: Upsert quotes (by id)
API-->>Client: Success response
Client->>API: GET /metrics?range=...
API->>DB: Query quotes where sentDate in range and amount > 200
API->>DB: Count quotesSent
API->>DB: Count quotesAccepted (status='accepted')
API-->>Client: {quotesSent, quotesAccepted, conversionRate, ...}
Client->>Client: Render ConversionRateWidget(sent, accepted)
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: 3
🧹 Nitpick comments (6)
packages/frontend/__tests__/ConversionRateWidget.test.tsx (1)
6-16: Expand test coverage for better component validation.The test correctly verifies basic label rendering, but consider adding tests for:
- Percentage calculation with various sent/accepted values
- Bar height rendering logic
- Tooltip content and interactions
- Edge cases (zero sent/accepted values)
- Hover state changes
Consider adding these additional test cases:
+ it('calculates percentage correctly', () => { + render( + <TooltipProvider> + <ConversionRateWidget sent={10} accepted={3} hovered={false} onHover={() => {}} getDescription={() => ''} /> + </TooltipProvider> + ); + expect(screen.getByText('30%')).toBeTruthy(); + }); + + it('handles zero sent quotes', () => { + render( + <TooltipProvider> + <ConversionRateWidget sent={0} accepted={0} hovered={false} onHover={() => {}} getDescription={() => ''} /> + </TooltipProvider> + ); + expect(screen.getByText('0%')).toBeTruthy(); + });packages/backend/src/services/metricsAggregation.conversionRate.test.ts (1)
20-36: Test logic is correct but could be more comprehensive.The conversion rate calculation is accurate (1 accepted out of 2 quotes over $200 = 0.5), and the test properly filters out the sub-$200 quote.
Consider improving:
- Fix the type assertion at line 32
- Add edge case tests
- const result = await aggregateMetric({ db: db as any, metric: 'conversionRate', start, end }); + const result = await aggregateMetric({ db, metric: 'conversionRate', start, end });Consider adding these test cases:
it('returns 0 when no quotes exist', async () => { const db = setupDb(); const result = await aggregateMetric({ db, metric: 'conversionRate', start: Date.now(), end: Date.now() }); expect(result).toEqual({ value: 0 }); }); it('ignores quotes under $200', async () => { const db = setupDb(); await db.insert(quotes).values([ { id: 'q1', jobId: null, sentDate: '2024-06-05', amount: 100, status: 'accepted' }, { id: 'q2', jobId: null, sentDate: '2024-06-06', amount: 150, status: 'sent' }, ]); const result = await aggregateMetric({ db, metric: 'conversionRate', start: Date.parse('2024-06-01'), end: Date.parse('2024-06-30') }); expect(result).toEqual({ value: 0 }); });packages/backend/src/db/schema/quotes.ts (2)
9-9: Add database-level constraint for status values.While the comment indicates valid status values, there's no database-level constraint to enforce them. Consider adding a check constraint to ensure data integrity.
- status: text('status').notNull(), // 'sent', 'accepted', 'rejected' + status: text('status', { enum: ['sent', 'accepted', 'rejected'] }).notNull(),Note: The exact syntax may vary depending on your Drizzle ORM version. You may need to use a separate constraint definition.
7-7: Consider adding format specification for date field.The
sentDatefield stores dates as text without format specification. Consider documenting the expected date format or using a more structured approach.- sentDate: text('sent_date').notNull(), + sentDate: text('sent_date').notNull(), // Expected format: YYYY-MM-DD or ISO 8601packages/backend/src/routes/quotes.ts (1)
28-44: Optimize batch processing performance.Processing quotes sequentially could be slow for large batches. Consider using batch insert operations or parallel processing.
- for (const q of parsed.quotes) { - await db.insert(quotes).values({ - id: q.id, - jobId: q.jobId ?? null, - sentDate: q.sentDate, - amount: q.amount, - status: q.status, - }).onConflictDoUpdate({ - target: [quotes.id], - set: { - jobId: q.jobId ?? null, - sentDate: q.sentDate, - amount: q.amount, - status: q.status, - }, - }); - } + const values = parsed.quotes.map(q => ({ + id: q.id, + jobId: q.jobId ?? null, + sentDate: q.sentDate, + amount: q.amount, + status: q.status, + })); + + // Use batch insert if supported by your ORM version + await db.insert(quotes).values(values).onConflictDoUpdate({ + target: [quotes.id], + set: { + jobId: excluded.jobId, + sentDate: excluded.sentDate, + amount: excluded.amount, + status: excluded.status, + }, + });packages/frontend/components/today-widgets/ConversionRateWidget.tsx (1)
21-21: Add validation for conversion rate logic.The component could display conversion rates over 100% if
accepted > sent, which would be logically impossible. Consider adding validation and handling this edge case.- const percentage = sent > 0 ? Math.round((accepted / sent) * 100) : 0; + const percentage = sent > 0 ? Math.min(100, Math.round((accepted / sent) * 100)) : 0; + + // Optional: Add warning for invalid data + if (accepted > sent && sent > 0) { + console.warn('Invalid conversion rate data: accepted quotes exceed sent quotes'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/data-model.md(2 hunks)docs/roadmap.md(2 hunks)packages/backend/src/db/index.ts(2 hunks)packages/backend/src/db/schema/quotes.ts(1 hunks)packages/backend/src/routes/api.ts(2 hunks)packages/backend/src/routes/quotes.ts(1 hunks)packages/backend/src/services/metricsAggregation.conversionRate.test.ts(1 hunks)packages/backend/src/services/metricsAggregation.ts(5 hunks)packages/frontend/__tests__/ConversionRateWidget.test.tsx(1 hunks)packages/frontend/components/metrics-explorer.tsx(1 hunks)packages/frontend/components/today-overview.tsx(6 hunks)packages/frontend/components/today-widgets/ConversionRateWidget.tsx(1 hunks)packages/frontend/components/today-widgets/QuotesAcceptedWidget.tsx(0 hunks)packages/frontend/lib/api.ts(1 hunks)packages/frontend/lib/today-widgets.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/frontend/components/today-widgets/QuotesAcceptedWidget.tsx
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/backend/src/db/index.ts (1)
packages/backend/src/db/schema/quotes.ts (1)
quotes(4-10)
packages/backend/src/routes/api.ts (1)
packages/backend/src/routes/quotes.ts (1)
registerQuotesRoutes(16-48)
packages/frontend/__tests__/ConversionRateWidget.test.tsx (1)
packages/frontend/components/today-widgets/ConversionRateWidget.tsx (1)
ConversionRateWidget(14-71)
packages/backend/src/routes/quotes.ts (2)
packages/backend/src/db/index.ts (1)
DB(31-31)packages/backend/src/db/schema/quotes.ts (1)
quotes(4-10)
packages/frontend/components/today-overview.tsx (1)
packages/frontend/components/today-widgets/ConversionRateWidget.tsx (1)
ConversionRateWidget(14-71)
🔇 Additional comments (24)
packages/frontend/lib/api.ts (1)
12-12: LGTM! Schema extension aligns with backend changes.The addition of
quotesSentfield to the MetricsResponse schema properly supports the new conversion rate metric calculation. The optional number typing is consistent with other metrics and appropriate for this use case.packages/frontend/lib/today-widgets.ts (1)
11-11: LGTM! Label update reflects the new metric accurately.The label change from "Quotes Accepted" to "Conversion Rate" appropriately reflects the updated metric calculation while maintaining the same widget ID for consistency.
packages/frontend/components/metrics-explorer.tsx (1)
33-33: LGTM! Metric mapping updated consistently.The mapping key change from 'quotes accepted' to 'conversion rate' maintains consistency with the updated metric naming while preserving the widget ID association.
packages/backend/src/routes/api.ts (2)
16-16: LGTM! Import follows established patterns.The import statement properly brings in the quotes route registration function consistent with other route imports in the file.
497-497: LGTM! Route registration properly integrated.The quotes routes are registered following the same pattern as other route registrations, enabling the new
/quotesPOST endpoint for quote data management.packages/backend/src/db/index.ts (1)
10-10: LGTM! Proper schema integration.The quotes schema is correctly imported and added to the schema aggregation object, following the established pattern for other database tables.
Also applies to: 22-22
docs/data-model.md (2)
78-78: Documentation accurately reflects the quotes table schema.The quotes table fields align with the database schema implementation and provide clear documentation for the new functionality.
103-103: Conversion rate metric description is precise and helpful.The updated description clearly specifies the "$200" threshold filter, which matches the implementation logic in the backend services.
packages/backend/src/services/metricsAggregation.conversionRate.test.ts (1)
8-18: Database setup correctly mirrors the quotes schema.The table creation SQL matches the expected schema structure, and the Drizzle setup is appropriate for unit testing.
packages/frontend/components/today-widgets/ConversionRateWidget.tsx (2)
1-71: Component implementation looks solid overall.The ConversionRateWidget is well-implemented with good accessibility features, proper TypeScript typing, and clean styling. The visualization clearly shows the relationship between sent and accepted quotes.
52-52:Details
✅ Verification successful
Verify $200 threshold implementation.
The tooltip mentions "quotes over $200" but this component doesn't validate or enforce this threshold. Ensure that the filtering happens in the backend metrics aggregation.
🏁 Script executed:
#!/bin/bash # Check if the $200 filtering is implemented in the metrics aggregation service rg -A 10 -B 5 "200|amount.*>" --type ts packages/backend/src/services/Length of output: 12433
Filtering confirmed in backend metrics aggregation
The
gt(quotes.amount, 200)filter is already applied inpackages/backend/src/services/metricsAggregation.tsfor both the aggregate and individual‐metric (quotesSent,quotesAccepted,conversionRate) queries. No further changes are needed in the frontend tooltip.docs/roadmap.md (2)
49-49: Documentation accurately reflects the implementation changes.The widget name change from
QuotesAcceptedWidgettoConversionRateWidgetand the updated notes correctly document the new quotes-based approach.
36-37:Details
✅ Verification successful
Verify $200 threshold filtering implementation.
The documentation specifies that quotes over $200 are counted, but I don't see this filtering logic in the provided database schema or API routes. Please ensure this filtering is implemented in the metrics aggregation service.
🏁 Script executed:
#!/bin/bash # Search for the $200 amount filtering in the metrics aggregation logic rg -A 15 -B 5 "200|amount.*>|WHERE.*amount" --type ts packages/backend/src/services/Length of output: 14862
$200 Threshold Filtering Confirmed
The metrics aggregation service applies the
$200amount filter in both sent and accepted quote counts, and the conversionRate tests cover this logic.Key locations:
- packages/backend/src/services/metricsAggregation.ts
- Quotes sent:
.where(and(gte(quotes.sentDate, isoStart), lte(quotes.sentDate, isoEnd), gt(quotes.amount, 200)))- Quotes accepted: adds
eq(quotes.status, 'accepted')alongsidegt(quotes.amount, 200)- packages/backend/src/services/metricsAggregation.conversionRate.test.ts
- Verifies accepted vs. sent quotes over
$200No further changes needed.
packages/backend/src/services/metricsAggregation.ts (5)
5-5: LGTM: Import statements are correctly updated.The addition of
gtoperator andquotesschema import aligns with the new quotes table functionality.Also applies to: 8-8
232-241: LGTM: Quotes logic properly updated to use dedicated quotes table.The new implementation correctly:
- Filters quotes by
sentDatewithin the specified period- Applies the
gt(quotes.amount, 200)filter for quotes over $200- Counts both sent and accepted quotes consistently
- Uses proper status filtering for accepted quotes
This represents a significant improvement over the previous job-based logic.
243-243: LGTM: Simplified and more accurate conversion rate calculation.The new calculation
quotesAccepted / quotesSentis much cleaner and more intuitive than the previous complex logic. The implicit division by zero protection (quotesSent > 0 check) works correctly since the result would be 0 when quotesSent is 0.
347-350: LGTM: Individual metric queries properly updated.Both
quotesAcceptedandconversionRateindividual metric queries have been correctly updated to:
- Use the quotes table instead of jobs table
- Apply consistent filtering (amount > $200, sentDate range)
- Maintain the same query structure and error handling
The logic is consistent with the dashboard aggregation queries.
Also applies to: 354-366
369-376:Details
✅ Verification successful
Verify consistency: costPerLead still uses jobs table.
While the quotes-related metrics have been updated to use the quotes table, the
costPerLeadmetric still uses the jobs table with job status filtering. This may be intentional, but please verify this is the correct business logic.
🏁 Script executed:
#!/bin/bash # Description: Check if costPerLead should also use quotes table for consistency # Expected: Find documentation or requirements about costPerLead calculation # Search for costPerLead references and documentation rg -A 5 -B 5 "costPerLead|cost.per.lead" # Check if there are any TODO comments or documentation about this metric fd -e md -e txt -e rst | xargs rg -i "cost.*lead|marketing.*spend"Length of output: 20938
🏁 Script executed:
#!/bin/bash # Display lines around quotesSent and quotesAccepted definitions in metricsAggregation.ts rg -n "quotesSent|quotesAccepted" -A 5 -B 5 packages/backend/src/services/metricsAggregation.tsLength of output: 2851
costPerLead now correctly uses the quotes table
The implementation immediately follows the conversionRate logic and uses
quotesAccepted(from thequotestable) to calculate cost per lead, so it’s aligned with the other quote-based metrics. No further changes needed here.packages/frontend/components/today-overview.tsx (6)
22-22: LGTM: Import statement correctly updated.The import change from
QuotesAcceptedWidgettoConversionRateWidgetaligns with the widget refactoring described in the PR objectives.
68-70: LGTM: MetricsState interface properly extended.The addition of
quotesSentandconversionRateproperties maintains type safety and follows the existing pattern of numeric metric properties.
110-110: LGTM: Component mapping correctly updated.The COMPONENT_MAP update to use
ConversionRateWidgetis consistent with the import change and maintains the component registration pattern.
132-134: LGTM: State initialization includes new metrics.The initialization values of 0 for both
quotesSentandconversionRateare appropriate defaults and consistent with other numeric metrics in the state.
203-205: LGTM: Data extraction properly handles new backend metrics.The extraction logic correctly:
- Uses null coalescing operator (
??) to provide default values- Maintains consistency with existing metric extraction patterns
- Handles the new
quotesSentandconversionRatefields from the backend response
535-541: LGTM: Widget integration correctly implemented.The
ConversionRateWidgetintegration properly:
- Passes
sentprop frommetrics.quotesSent- Passes
acceptedprop frommetrics.quotesAccepted- Maintains the existing hover and description handler patterns
- Uses the same container structure as other widgets
This matches the component API shown in the relevant code snippets.
| id: text('id').primaryKey(), | ||
| jobId: text('job_id').references(() => jobs.id), | ||
| sentDate: text('sent_date').notNull(), | ||
| amount: real('amount').notNull(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using integer for money amounts to avoid precision issues.
Using real for monetary values can lead to floating point precision errors. Consider storing amounts as integers in the smallest currency unit (e.g., cents) to maintain precision.
- amount: real('amount').notNull(),
+ amount: integer('amount').notNull(), // Store as cents to avoid floating point issuesAlternatively, if you need decimal precision, consider using a decimal/numeric type or a string field with validation.
📝 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.
| amount: real('amount').notNull(), | |
| // Instead of REAL (floating point), store amounts in the smallest unit (e.g. cents) as an integer | |
| amount: integer('amount').notNull(), // Store as cents to avoid floating point issues |
🤖 Prompt for AI Agents
In packages/backend/src/db/schema/quotes.ts at line 8, the amount field is
defined as a real type which can cause floating point precision errors for
monetary values. Change the type to an integer representing the smallest
currency unit (e.g., cents) to avoid precision issues. Alternatively, if decimal
precision is required, use a decimal/numeric type or a string with proper
validation instead of real.
| for (const q of parsed.quotes) { | ||
| await db.insert(quotes).values({ | ||
| id: q.id, | ||
| jobId: q.jobId ?? null, | ||
| sentDate: q.sentDate, | ||
| amount: q.amount, | ||
| status: q.status, | ||
| }).onConflictDoUpdate({ | ||
| target: [quotes.id], | ||
| set: { | ||
| jobId: q.jobId ?? null, | ||
| sentDate: q.sentDate, | ||
| amount: q.amount, | ||
| status: q.status, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap batch operations in a database transaction.
The current implementation processes quotes sequentially without transaction boundaries. If an error occurs partway through, some quotes will be inserted while others won't, leading to inconsistent state.
+ await db.transaction(async (tx) => {
for (const q of parsed.quotes) {
- await db.insert(quotes).values({
+ await tx.insert(quotes).values({
id: q.id,
jobId: q.jobId ?? null,
sentDate: q.sentDate,
amount: q.amount,
status: q.status,
}).onConflictDoUpdate({
target: [quotes.id],
set: {
jobId: q.jobId ?? null,
sentDate: q.sentDate,
amount: q.amount,
status: q.status,
},
});
}
+ });📝 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.
| for (const q of parsed.quotes) { | |
| await db.insert(quotes).values({ | |
| id: q.id, | |
| jobId: q.jobId ?? null, | |
| sentDate: q.sentDate, | |
| amount: q.amount, | |
| status: q.status, | |
| }).onConflictDoUpdate({ | |
| target: [quotes.id], | |
| set: { | |
| jobId: q.jobId ?? null, | |
| sentDate: q.sentDate, | |
| amount: q.amount, | |
| status: q.status, | |
| }, | |
| }); | |
| } | |
| await db.transaction(async (tx) => { | |
| for (const q of parsed.quotes) { | |
| await tx.insert(quotes).values({ | |
| id: q.id, | |
| jobId: q.jobId ?? null, | |
| sentDate: q.sentDate, | |
| amount: q.amount, | |
| status: q.status, | |
| }).onConflictDoUpdate({ | |
| target: [quotes.id], | |
| set: { | |
| jobId: q.jobId ?? null, | |
| sentDate: q.sentDate, | |
| amount: q.amount, | |
| status: q.status, | |
| }, | |
| }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In packages/backend/src/routes/quotes.ts around lines 28 to 44, the batch
insertion of quotes is done sequentially without a transaction, risking partial
updates on errors. Wrap the entire loop that inserts or updates quotes inside a
single database transaction to ensure atomicity, so either all quotes are
processed successfully or none are applied, maintaining consistent state.
| export function registerQuotesRoutes(api: Hono<any>) { | ||
| api.post('/quotes', async (c) => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding authentication and rate limiting.
The endpoint doesn't appear to have authentication or rate limiting. This could allow unauthorized access or abuse of the API.
Could you verify if authentication middleware is applied at a higher level? If not, consider adding proper authentication and rate limiting for this endpoint.
🏁 Script executed:
#!/bin/bash
# Check for authentication middleware in the API routes
rg -A 10 -B 5 "registerQuotesRoutes|app\.use.*auth|middleware.*auth" --type tsLength of output: 4376
Add authentication and rate limiting to /quotes
The /quotes endpoint in packages/backend/src/routes/quotes.ts (lines 16–17) is exposed without any Hono-level protection:
- It isn’t mounted under the
/protectedpath inpackages/backend/src/routes/api.ts - No authentication or rate-limiting middleware is applied (the Next.js
middleware.tsat the repo root doesn’t cover Hono routes)
Please address the following:
• Apply authentication middleware (e.g. api.use(authMiddleware)) before registering the /quotes route or move it under api.route('/protected', protectedRoutes).
• Introduce rate limiting (e.g. Hono’s RateLimit middleware) to throttle requests and prevent abuse.
🤖 Prompt for AI Agents
In packages/backend/src/routes/quotes.ts around lines 16 to 17, the /quotes
endpoint lacks authentication and rate limiting, exposing it to unauthorized
access and abuse. Fix this by applying authentication middleware (e.g.,
api.use(authMiddleware)) before defining the /quotes route or by moving the
route under a protected path such as api.route('/protected', protectedRoutes).
Additionally, add rate limiting middleware (e.g., Hono's RateLimit) to throttle
incoming requests and prevent abuse.
1 participant
Summary
quotestableTesting
npm run lint(fails: ESLint config missing)npm run typechecknpm run testnpm run build(fails: connect EHOSTUNREACH for fonts)Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Refactor
Chores