WIP: response time tracking #939
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new Next.js GET API route at Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as GET /response-time
participant Provider as EmailProvider
participant Logger as Logger
participant Aggregator as Aggregation Logic
Client->>Handler: GET /api/user/stats/response-time?fromDate=&toDate=
Handler->>Handler: validate query params
Handler->>Provider: fetchSentMessages(fromDate,toDate)
rect rgb(220,240,230)
Note over Handler,Provider: iterate sent threads
Handler->>Provider: getThreadMessages(threadId)
alt thread loaded
Handler->>Handler: sort messages by internalDate
Handler->>Handler: detect RECEIVED -> SENT pairs
Handler->>Aggregator: emit ResponseTimeEntry (minutes)
else error loading thread
Provider-->>Logger: log error and continue
end
end
rect rgb(240,240,255)
Handler->>Aggregator: calculateSummaryStats(entries)
Handler->>Aggregator: calculateDistribution(entries)
Handler->>Aggregator: calculateTrend(entries)
end
rect rgb(255,240,240)
Note over Handler: if fromDate/toDate present
Handler->>Handler: compute previous period stats (recursive/fetch)
end
Handler->>Client: JSON { summary, distribution, trend }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 1 file
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="apps/web/app/api/user/stats/response-time/route.ts">
<violation number="1" location="apps/web/app/api/user/stats/response-time/route.ts:106">
The received filter compares against the recipient address, so the customer's reply is excluded and your own outbound message is treated as inbound; switch to the sender address so we keep actual customer responses.</violation>
<violation number="2" location="apps/web/app/api/user/stats/response-time/route.ts:118">
This filter also checks against the recipient address, so inbound replies are treated as your own outbound messages; use the sender address to capture actual user-sent mail.</violation>
<violation number="3" location="apps/web/app/api/user/stats/response-time/route.ts:255">
Guard the previous-period recursion when `differenceInDays` returns 0 (same-day range); otherwise this calls itself with identical parameters and recurses forever.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| const previousFromDate = subDays(new Date(fromDate), currentPeriodDays); | ||
| const previousToDate = new Date(fromDate); | ||
|
|
||
| const previousPeriodStats = await getResponseTimeStats({ |
There was a problem hiding this comment.
Guard the previous-period recursion when differenceInDays returns 0 (same-day range); otherwise this calls itself with identical parameters and recurses forever.
Prompt for AI agents
Address the following comment on apps/web/app/api/user/stats/response-time/route.ts at line 255:
<comment>Guard the previous-period recursion when `differenceInDays` returns 0 (same-day range); otherwise this calls itself with identical parameters and recurses forever.</comment>
<file context>
@@ -0,0 +1,309 @@
+ const previousFromDate = subDays(new Date(fromDate), currentPeriodDays);
+ const previousToDate = new Date(fromDate);
+
+ const previousPeriodStats = await getResponseTimeStats({
+ fromDate: previousFromDate.getTime(),
+ toDate: previousToDate.getTime(),
</file context>
| .filter( | ||
| (m) => | ||
| m.internalDate && | ||
| m.headers.from?.includes(sentMsg.headers.to || ""), |
There was a problem hiding this comment.
This filter also checks against the recipient address, so inbound replies are treated as your own outbound messages; use the sender address to capture actual user-sent mail.
Prompt for AI agents
Address the following comment on apps/web/app/api/user/stats/response-time/route.ts at line 118:
<comment>This filter also checks against the recipient address, so inbound replies are treated as your own outbound messages; use the sender address to capture actual user-sent mail.</comment>
<file context>
@@ -0,0 +1,309 @@
+ .filter(
+ (m) =>
+ m.internalDate &&
+ m.headers.from?.includes(sentMsg.headers.to || ""),
+ )
+ .sort(
</file context>
✅ Addressed in 6b48d85
| .filter( | ||
| (m) => | ||
| m.internalDate && | ||
| !m.headers.from?.includes(sentMsg.headers.to || ""), |
There was a problem hiding this comment.
The received filter compares against the recipient address, so the customer's reply is excluded and your own outbound message is treated as inbound; switch to the sender address so we keep actual customer responses.
Prompt for AI agents
Address the following comment on apps/web/app/api/user/stats/response-time/route.ts at line 106:
<comment>The received filter compares against the recipient address, so the customer's reply is excluded and your own outbound message is treated as inbound; switch to the sender address so we keep actual customer responses.</comment>
<file context>
@@ -0,0 +1,309 @@
+ .filter(
+ (m) =>
+ m.internalDate &&
+ !m.headers.from?.includes(sentMsg.headers.to || ""),
+ )
+ .sort(
</file context>
✅ Addressed in 6b48d85
Add response time tracking and expose GET endpoint in
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| }) | ||
| .sort((a, b) => a.periodDate.getTime() - b.periodDate.getTime()); | ||
|
|
||
| // Calculate previous period comparison |
There was a problem hiding this comment.
Unbounded recursion when computing the previous period. getResponseTimeStats calls itself with another prior window, which never terminates (and loops immediately when differenceInDays is 0). Consider adding a guard (e.g., skip when currentPeriodDays <= 0) and/or a depth cap/flag to disable the previous-period calculation in recursive calls.
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| // Fetch sent messages in the date range | ||
| const sentMessages = await emailProvider.getMessagesByFields({ | ||
| type: "sent", | ||
| ...(fromDate ? { after: new Date(fromDate) } : {}), |
There was a problem hiding this comment.
Using truthy checks on fromDate/toDate drops valid 0 (Unix epoch). Consider checking for null/undefined instead (e.g., fromDate != null, toDate != null) in the spread filters and the previous-period if so 0 is respected.
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
![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
♻️ Duplicate comments (2)
apps/web/app/api/user/stats/response-time/route.ts (2)
101-125: Inbound vs outbound filters are inverted (using recipient instead of sender).
receivedMessagesandsentMessagesInThreadboth compareheaders.fromagainstsentMsg.headers.to, which is the recipient address of your original outbound email. This:
- Treats your own outbound messages as “received”.
- Excludes actual customer replies from
receivedMessages.You almost certainly want to distinguish based on the sender (your address) instead:
- “Sent” = messages whose
frommatches the original sender (sentMsg.headers.from).- “Received” = messages whose
fromdoes not match that sender.A minimal fix:
- const receivedMessages = threadMessages - .filter( - (m) => - m.internalDate && - !m.headers.from?.includes(sentMsg.headers.to || ""), - ) + const receivedMessages = threadMessages + .filter( + (m) => + m.internalDate && + !m.headers.from?.includes(sentMsg.headers.from || ""), + ) @@ - const sentMessagesInThread = threadMessages - .filter( - (m) => - m.internalDate && - m.headers.from?.includes(sentMsg.headers.to || ""), - ) + const sentMessagesInThread = threadMessages + .filter( + (m) => + m.internalDate && + m.headers.from?.includes(sentMsg.headers.from || ""), + )This aligns with the previous Cubic comment and ensures response times are computed between actual customer messages and your replies.
245-273: Guard previous‑period recursion to avoid infinite loop for zero‑length ranges.If
differenceInDaysreturns 0 (same‑day or otherwise zero‑day range), the computed previous window is identical (fromDate→fromDate), andgetResponseTimeStatsrecursively calls itself with the same parameters indefinitely.You should skip the previous‑period computation when the day span is ≤ 0:
// Calculate previous period comparison let previousPeriodComparison = null; if (fromDate && toDate) { const currentPeriodDays = differenceInDays( new Date(toDate), new Date(fromDate), ); - const previousFromDate = subDays(new Date(fromDate), currentPeriodDays); - const previousToDate = new Date(fromDate); - - const previousPeriodStats = await getResponseTimeStats({ - fromDate: previousFromDate.getTime(), - toDate: previousToDate.getTime(), - emailProvider, - logger, - }); - - if (previousPeriodStats.summary.medianResponseTime > 0) { - const percentChange = - ((medianResponseTime - previousPeriodStats.summary.medianResponseTime) / - previousPeriodStats.summary.medianResponseTime) * - 100; - - previousPeriodComparison = { - medianResponseTime: previousPeriodStats.summary.medianResponseTime, - percentChange: Math.round(percentChange), - }; - } + if (currentPeriodDays > 0) { + const previousFromDate = subDays(new Date(fromDate), currentPeriodDays); + const previousToDate = new Date(fromDate); + + const previousPeriodStats = await getResponseTimeStats({ + fromDate: previousFromDate.getTime(), + toDate: previousToDate.getTime(), + emailProvider, + logger, + }); + + if (previousPeriodStats.summary.medianResponseTime > 0) { + const percentChange = + ((medianResponseTime - + previousPeriodStats.summary.medianResponseTime) / + previousPeriodStats.summary.medianResponseTime) * + 100; + + previousPeriodComparison = { + medianResponseTime: previousPeriodStats.summary.medianResponseTime, + percentChange: Math.round(percentChange), + }; + } + } }In date-fns v4.1.0, what does `differenceInDays` return when both arguments are the same date, and can it be zero for valid ranges?
🧹 Nitpick comments (3)
apps/web/app/api/user/stats/response-time/route.ts (3)
44-49:periodDateis typed asDatebut serialized to JSON as a string.The
trendtype currently exposesperiodDate: Date, andResponseTimeResponsere‑exports that return type, butNextResponse.jsonwill serialize theDateto an ISO string:trend: Array<{ period: string; periodDate: Date; ... }> ... return { period: format(date, "LLL dd, y"), periodDate: date, // becomes string over the wire ... };On the client, anything using
ResponseTimeResponsewill thinkperiodDateis aDateinstance but actually receive a string.Consider normalizing this to a string (or number) in both type and implementation, e.g.:
- trend: Array<{ - period: string; - periodDate: Date; - medianResponseTime: number; - count: number; - }>; + trend: Array<{ + period: string; + periodDate: string; // ISO string + medianResponseTime: number; + count: number; + }>;and:
- return { - period: format(date, "LLL dd, y"), - periodDate: date, - medianResponseTime: Math.round(median), - count: values.length, - }; + return { + period: format(date, "LLL dd, y"), + periodDate: date.toISOString(), + medianResponseTime: Math.round(median), + count: values.length, + };This keeps the API contract honest and avoids subtle client bugs.
Also applies to: 236-241, 278-288, 291-293
91-153: SequentialgetThreadMessagescalls may be slow for larger datasets.The
for...ofloop awaitsemailProvider.getThreadMessagesone thread at a time. With up to 500 sent messages, this can mean hundreds of sequential network calls and a very slow endpoint.Consider batching with a bounded concurrency pattern, for example:
- Build an array of async tasks per unique
threadId.- Use
Promise.allwith a simple pool/limit (e.g.p-limitor a hand‑rolled queue) to run, say, 5–10 in parallel while keeping rate limits in mind.This should materially improve response times without changing the external behavior.
11-14: Handle query param validation errors with 400 instead of 500.The current code at line 295 uses
responseTimeParams.parse()without error handling. When invalid query params are passed (e.g.,?fromDate=abc),z.coerce.number()will throw a ZodError that bubbles up as a 500 error instead of the expected 400 client error.Switch to
safeParse()and return a 400 JSON error on validation failure:const result = responseTimeParams.safeParse({ fromDate: searchParams.get("fromDate"), toDate: searchParams.get("toDate"), }); if (!result.success) { return NextResponse.json( { error: "Invalid query parameters", details: result.error.flatten() }, { status: 400 } ); } const params = result.data;Also applies to: 295-309
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/api/user/stats/response-time/route.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/app/api/user/stats/response-time/route.ts (2)
apps/web/utils/logger.ts (1)
Logger(5-5)apps/web/utils/middleware.ts (1)
withEmailProvider(401-414)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Jit Security
- GitHub Check: test
- GitHub Check: Review for correctness
- GitHub Check: Analyze (javascript-typescript)
| // Calculate response rate (threads with reply vs total sent) | ||
| const responseRate = 100; // All threads in our dataset have replies by definition | ||
|
|
||
| return { | ||
| summary: { | ||
| medianResponseTime: Math.round(medianResponseTime), | ||
| averageResponseTime: Math.round(averageResponseTime), | ||
| responseRate: Math.round(responseRate), | ||
| within1Hour: Math.round(within1Hour), |
There was a problem hiding this comment.
responseRate is hardcoded to 100%, which makes the metric misleading.
Right now:
// Calculate response rate (threads with reply vs total sent)
const responseRate = 100; // All threads in our dataset have replies by definitionBut you only push to responseTimes when a valid “received → sent” pair exists; threads without a qualifying reply are effectively invisible. Exposing responseRate: 100 will always claim perfect performance, regardless of reality.
Options:
- Compute a real rate, e.g.
responseTimes.length / processedThreads.size * 100, or - Drop/rename this field until you track unanswered threads explicitly.
I’d recommend at least removing the hardcoded 100% to avoid shipping a clearly incorrect metric.
🤖 Prompt for AI Agents
In apps/web/app/api/user/stats/response-time/route.ts around lines 275-283 the
responseRate is hardcoded to 100 which is misleading; compute an actual
percentage instead by dividing the number of threads with valid response pairs
(responseTimes.length) by the total number of processed threads
(processedThreads.size or equivalent), multiply by 100 and round, and guard
against division by zero (return 0 if no processed threads); alternatively
remove/rename the field if you cannot accurately track unanswered threads yet.
There was a problem hiding this comment.
3 issues found across 1 file
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="apps/web/app/api/user/stats/response-time/route.ts">
<violation number="1" location="apps/web/app/api/user/stats/response-time/route.ts:106">
Received/sent classification compares sender headers to the outgoing recipients, so the buckets are inverted and the reported response times track customer replies instead of the user’s responses.</violation>
<violation number="2" location="apps/web/app/api/user/stats/response-time/route.ts:248">
`differenceInDays` can return 0 for same‑day ranges, so the recursive previous‑period call runs with the exact same dates and never terminates, leading to infinite recursion for short queries.</violation>
<violation number="3" location="apps/web/app/api/user/stats/response-time/route.ts:276">
Hardcoding the responseRate to 100% always reports perfect performance even if some processed threads never receive a qualifying reply; derive the percentage from the ratio of threads with measured response times to total processed threads.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| // Calculate previous period comparison | ||
| let previousPeriodComparison = null; | ||
| if (fromDate && toDate) { | ||
| const currentPeriodDays = differenceInDays( |
There was a problem hiding this comment.
differenceInDays can return 0 for same‑day ranges, so the recursive previous‑period call runs with the exact same dates and never terminates, leading to infinite recursion for short queries.
Prompt for AI agents
Address the following comment on apps/web/app/api/user/stats/response-time/route.ts at line 248:
<comment>`differenceInDays` can return 0 for same‑day ranges, so the recursive previous‑period call runs with the exact same dates and never terminates, leading to infinite recursion for short queries.</comment>
<file context>
@@ -0,0 +1,309 @@
+ // Calculate previous period comparison
+ let previousPeriodComparison = null;
+ if (fromDate && toDate) {
+ const currentPeriodDays = differenceInDays(
+ new Date(toDate),
+ new Date(fromDate),
</file context>
✅ Addressed in 6b48d85
| .filter( | ||
| (m) => | ||
| m.internalDate && | ||
| !m.headers.from?.includes(sentMsg.headers.to || ""), |
There was a problem hiding this comment.
Received/sent classification compares sender headers to the outgoing recipients, so the buckets are inverted and the reported response times track customer replies instead of the user’s responses.
Prompt for AI agents
Address the following comment on apps/web/app/api/user/stats/response-time/route.ts at line 106:
<comment>Received/sent classification compares sender headers to the outgoing recipients, so the buckets are inverted and the reported response times track customer replies instead of the user’s responses.</comment>
<file context>
@@ -0,0 +1,309 @@
+ .filter(
+ (m) =>
+ m.internalDate &&
+ !m.headers.from?.includes(sentMsg.headers.to || ""),
+ )
+ .sort(
</file context>
✅ Addressed in 6b48d85
| } | ||
|
|
||
| // Calculate response rate (threads with reply vs total sent) | ||
| const responseRate = 100; // All threads in our dataset have replies by definition |
There was a problem hiding this comment.
Hardcoding the responseRate to 100% always reports perfect performance even if some processed threads never receive a qualifying reply; derive the percentage from the ratio of threads with measured response times to total processed threads.
Prompt for AI agents
Address the following comment on apps/web/app/api/user/stats/response-time/route.ts at line 276:
<comment>Hardcoding the responseRate to 100% always reports perfect performance even if some processed threads never receive a qualifying reply; derive the percentage from the ratio of threads with measured response times to total processed threads.</comment>
<file context>
@@ -0,0 +1,309 @@
+ }
+
+ // Calculate response rate (threads with reply vs total sent)
+ const responseRate = 100; // All threads in our dataset have replies by definition
+
+ return {
</file context>
| const responseRate = 100; // All threads in our dataset have replies by definition | |
| const responseRate = processedThreads.size === 0 ? 0 : (responseTimes.length / processedThreads.size) * 100; |
| const responseTimes: ResponseTimeEntry[] = []; | ||
| const processedThreads = new Set<string>(); | ||
|
|
||
| const sentLabelId = "SENT"; |
There was a problem hiding this comment.
Sent/received detection is provider-specific. Hardcoding Gmail "SENT" and falling back to the first 100 sentMessages can misclassify messages (e.g., Microsoft), skewing response times. Consider using emailProvider.isSentMessage(message) for reliable, provider-aware detection.
- const sentLabelId = "SENT";
@@
- // Determine if message is sent or received
- let isSent = false;
- if (message.labelIds?.includes(sentLabelId)) {
- isSent = true;
- }
-
- // If we still haven't matched, fallback to checking if this specific message is in our known sent list
- // (Only efficient if sentMessages is small, but we capped it at 100)
- if (!isSent) {
- // Optimize: check id match
- if (sentMessages.some((sm: any) => sm.id === message.id)) {
- isSent = true;
- }
- }
+ // Determine if message is sent or received
+ const isSent = emailProvider.isSentMessage(message);
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/web/app/api/user/stats/response-time/route.ts (4)
81-82: Guard against dropping valid epoch timestamp (0).The truthy checks
fromDate ? ...andtoDate ? ...will omit the filter when the timestamp is0(Unix epoch). Use explicit nullish checks instead.Apply this diff:
- ...(fromDate ? { after: new Date(fromDate) } : {}), - ...(toDate ? { before: new Date(toDate) } : {}), + ...(fromDate != null ? { after: new Date(fromDate) } : {}), + ...(toDate != null ? { before: new Date(toDate) } : {}),
216-245: Prevent infinite recursion for same-day ranges.When
differenceInDaysreturns 0, the recursive call togetResponseTimeStatsreceives identical date parameters, causing infinite recursion.Apply this diff to guard the recursion:
async function calculatePreviousPeriodComparison( fromDate: number | null | undefined, toDate: number | null | undefined, currentMedian: number, emailProvider: EmailProvider, logger: Logger, ): Promise<SummaryStats["previousPeriodComparison"]> { if (!fromDate || !toDate) return null; const currentDays = differenceInDays(new Date(toDate), new Date(fromDate)); + // Guard against infinite recursion for same-day or invalid ranges + if (currentDays <= 0) return null; + const prevFrom = subDays(new Date(fromDate), currentDays); const prevTo = new Date(fromDate);
271-271: Compute actual response rate instead of hardcoding 100%.Hardcoding
responseRate: 100is misleading. Threads without qualifying received→sent pairs are excluded fromresponseTimes, making the reported rate inaccurate.The actual rate should reflect the ratio of threads with measured responses to total processed threads. However,
processedThreadsis local tocalculateResponseTimesand not accessible here. Consider one of these approaches:Option 1: Return
processedThreads.sizefromcalculateResponseTimesand pass it tocalculateSummaryStats:export async function calculateResponseTimes( sentMessages: any[], emailProvider: EmailProvider, logger: Logger, -): Promise<ResponseTimeEntry[]> { +): Promise<{ responseTimes: ResponseTimeEntry[]; totalThreads: number }> { const responseTimes: ResponseTimeEntry[] = []; const processedThreads = new Set<string>(); // ... existing logic ... - return responseTimes; + return { responseTimes, totalThreads: processedThreads.size }; }Then update
calculateSummaryStats:export async function calculateSummaryStats( responseTimes: ResponseTimeEntry[], + totalThreads: number, fromDate: number | null | undefined, toDate: number | null | undefined, emailProvider: EmailProvider, logger: Logger, ): Promise<SummaryStats> { const values = responseTimes.map((r) => r.responseTimeMinutes); // ... return { medianResponseTime: Math.round(medianResponseTime), averageResponseTime: Math.round(averageResponseTime), - responseRate: 100, + responseRate: totalThreads === 0 ? 0 : Math.round((responseTimes.length / totalThreads) * 100), within1Hour: Math.round(within1Hour), previousPeriodComparison, }; }Option 2: Remove or rename the field until you can accurately track unanswered threads.
223-223: Use nullish check for consistency.Similar to lines 81-82, the truthy check here will reject valid
0timestamps.Apply this diff:
- if (!fromDate || !toDate) return null; + if (fromDate == null || toDate == null) return null;
🧹 Nitpick comments (3)
apps/web/app/api/user/stats/response-time/route.test.ts (3)
11-11: Remove unused import.The import on line 11 is shadowed by line 20 and never used.
Apply this diff:
-import { getMockMessage } from "@/utils/test/helpers"; import type { EmailProvider } from "@/utils/email/types";
197-219: Consider adding test coverage for edge cases.The current test doesn't verify:
- The
responseRatefield (currently hardcoded to 100%)- The
previousPeriodComparisonlogic (requires non-nullfromDate/toDate)- Edge cases like empty arrays or single-value arrays
Consider adding tests like:
it("should handle empty response times", async () => { const result = await calculateSummaryStats( [], null, null, mockProvider, mockLogger, ); expect(result.averageResponseTime).toBe(0); expect(result.medianResponseTime).toBe(0); expect(result.within1Hour).toBe(0); }); it("should calculate previous period comparison when dates provided", async () => { // Test with fromDate/toDate to verify previousPeriodComparison // Note: This would require mocking getResponseTimeStats });
221-238: Consider testing bucket boundaries.The current test verifies basic bucketing but doesn't test boundary values that might be off-by-one errors.
Consider adding a test for exact boundaries:
it("should handle bucket boundaries correctly", () => { const responseTimes = [ { responseTimeMinutes: 59 }, // < 1h (edge) { responseTimeMinutes: 60 }, // 1-4h (boundary) { responseTimeMinutes: 239 }, // 1-4h (edge) { responseTimeMinutes: 240 }, // 4-24h (boundary) { responseTimeMinutes: 1440 }, // 1-3d (boundary) ] as any[]; const result = calculateDistribution(responseTimes); expect(result.lessThan1Hour).toBe(1); expect(result.oneToFourHours).toBe(2); expect(result.fourTo24Hours).toBe(1); expect(result.oneToThreeDays).toBe(1); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/api/user/stats/response-time/route.test.ts(1 hunks)apps/web/app/api/user/stats/response-time/route.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/app/api/user/stats/response-time/route.test.ts (2)
apps/web/utils/logger.ts (1)
Logger(5-5)apps/web/app/api/user/stats/response-time/route.ts (3)
calculateResponseTimes(119-193)calculateSummaryStats(247-275)calculateDistribution(277-289)
apps/web/app/api/user/stats/response-time/route.ts (2)
apps/web/utils/middleware.ts (1)
withEmailProvider(401-414)apps/web/utils/logger.ts (1)
Logger(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Static Code Analysis Js
- GitHub Check: Jit Security
- GitHub Check: cubic · AI code reviewer
- GitHub Check: test
- GitHub Check: Review for correctness
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
apps/web/app/api/user/stats/response-time/route.ts (5)
122-131: Sent/received detection is Gmail-specific; prefer provider-aware helperClassification currently relies on the hardcoded
"SENT"label and a fallback membership in the initialsentMessageslist. That’s fine for Gmail but will misclassify messages for other providers (e.g. Outlook) that don’t share the same label semantics.If your
EmailProviderexposes something likeemailProvider.isSentMessage(message), prefer delegating the classification to that helper so each provider can encode its own rules. This also centralizes any future tweaks in one place.Also applies to: 155-165
12-16: Invalid timestamps becomeNaNand can bubble into Date logic / 500s; tighten schema or treat as bad request
z.coerce.number().nullish()will happily coerce invalid query strings (e.g.?fromDate=abcor empty) toNaN, which then flows intonew Date(fromDate)/differenceInDaysand can cause unpredictable behavior or 500s rather than a clean 400. Consider refining to finite numbers and/or usingsafeParseto return a 400 for bad params, e.g.:const timestamp = z .preprocess((value) => { if (value == null) return undefined; const n = Number(value); return Number.isFinite(n) ? n : undefined; }, z.number().optional()); const responseTimeSchema = z.object({ fromDate: timestamp, toDate: timestamp, });and handle
safeParsefailure in the handler as a 400 instead of throwing.Also applies to: 52-57
83-86: Truthiness checks onfromDate/toDatedrop valid0(Unix epoch) and conflate “absent” with “0”Both the
getMessagesByFieldsfilters andcalculatePreviousPeriodComparisonuse truthy checks (fromDate ? …/if (!fromDate || !toDate)) so a valid0timestamp is treated as missing, andnull/undefined/0are all folded together. If0is meant to be valid, prefer explicit nullish checks:// Fetch ...(fromDate != null ? { after: new Date(fromDate) } : {}), ...(toDate != null ? { before: new Date(toDate) } : {}), // Previous-period guard if (fromDate == null || toDate == null) return null;This also pairs better with a refined schema that rejects
NaN.Also applies to: 217-225
68-87: Previous-period calculation causes unbounded recursion (and same-range loops when day-diff ≤ 0)
calculateSummaryStatsalways callscalculatePreviousPeriodComparison, which callsgetResponseTimeStatsagain with another window; that inner call again computes a previous period, and so on without a base case. WhendifferenceInDaysreturns0(same-day range) you even recurse with identical dates. This will eventually stack overflow for any request withfromDate/toDate.You need both (a) a guard on the day span and (b) a way to disable previous-period recursion for the recursive call, e.g.:
type GetResponseTimeStatsOptions = { includePreviousPeriod?: boolean }; async function getResponseTimeStats( { fromDate, toDate, emailProvider, logger }: ResponseTimeParams & { emailProvider: EmailProvider; logger: Logger }, { includePreviousPeriod = true }: GetResponseTimeStatsOptions = {}, ) { // ... const summary = await calculateSummaryStats( responseTimes, includePreviousPeriod ? fromDate : null, includePreviousPeriod ? toDate : null, emailProvider, logger, ); } async function calculatePreviousPeriodComparison(/* … */) { if (fromDate == null || toDate == null) return null; const currentDays = differenceInDays(new Date(toDate), new Date(fromDate)); if (!Number.isFinite(currentDays) || currentDays <= 0) return null; const prevStats = await getResponseTimeStats( { fromDate: subDays(new Date(fromDate), currentDays).getTime(), toDate: new Date(fromDate).getTime(), emailProvider, logger, }, { includePreviousPeriod: false }, ); // ... }Without something along these lines, this endpoint is not safe to deploy.
Also applies to: 217-246, 261-267
248-276:responseRatehardcoded to 100% makes the metric incorrect / misleading
calculateSummaryStatsalways returnsresponseRate: 100, whilegetEmptyStatsuses0. SinceresponseTimesonly includes threads with a valid received→sent pair, unanswered threads are invisible and this 100% figure does not reflect reality.Either compute an actual rate (e.g. thread-level responses / total processed threads, possibly by returning
processedThreads.sizefromcalculateResponseTimes) or drop/rename the field until you have enough data to compute it meaningfully. At minimum, avoid returning a hardcoded 100%.Also applies to: 333-341
🧹 Nitpick comments (3)
apps/web/app/api/user/stats/response-time/route.ts (3)
211-215: Align “within 1 hour” metric with< 1 hourbucket semantics
calculateWithin1Hourtreats<= 60minutes as “within 1 hour”, whilecalculateDistributionuses< 60for thelessThan1Hourbucket. A 60‑minute response is counted as “within 1 hour” but ends up in theoneToFourHoursbucket, which may be confusing when comparing metrics.Either change the metric to
< 60to match the bucket, or rename the bucket to something likeupTo1Hourand adjust the threshold, so names and thresholds stay consistent.Also applies to: 278-299
81-87: Sampling only the first 100 sent messages can skew stats for heavy users
getMessagesByFieldsis capped atmaxResults: 100, andcalculateResponseTimesprocesses only threads corresponding to those messages. For users with high volume, this may bias summary/distribution/trend toward the most recent or arbitrary slice of activity.If feasible, consider:
- Paging through more messages for the selected window, or
- Making the cap configurable / clearly documenting that this is a sampled view.
This is not a correctness bug but worth being explicit about.
Also applies to: 122-191
18-23: API response serializesDatefields to strings; ensure consumer types reflect that
ResponseTimeEntry.receivedDate/sentDateandTrendEntry.periodDateare typed asDate, butNextResponse.jsonwill serialize them as ISO strings over the wire. If these interfaces (or a derivedResponseTimeResponsetype) are reused on the client, the runtime shape will actually be{ periodDate: string, receivedDate: string, … }.Consider defining explicit API response types that use
stringfor date fields (and keepDatefor internal/server-only structs), or consistently parse them back toDateon the client while reflecting that in types.Also applies to: 45-50, 304-331, 333-352
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/api/user/stats/response-time/route.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/app/api/user/stats/response-time/route.ts (3)
apps/web/utils/middleware.ts (1)
withEmailProvider(401-414)apps/web/utils/logger.ts (1)
Logger(5-5)apps/web/utils/types.ts (1)
ParsedMessage(51-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
There was a problem hiding this comment.
2 issues found across 2 files (reviewed changes from recent commits).
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="apps/web/app/api/user/stats/response-time/route.ts">
<violation number="1" location="apps/web/app/api/user/stats/response-time/route.ts:167">
Response-time metrics ignore the requested date range: once a thread is selected, every historical reply in that thread is counted, so a 7‑day query still includes months-old responses.</violation>
<violation number="2" location="apps/web/app/api/user/stats/response-time/route.ts:272">
Response rate is always reported as 100%, because it is hard-coded instead of being derived from the share of sent conversations that received a reply.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| return { | ||
| medianResponseTime: Math.round(medianResponseTime), | ||
| averageResponseTime: Math.round(averageResponseTime), | ||
| responseRate: 100, |
There was a problem hiding this comment.
Response rate is always reported as 100%, because it is hard-coded instead of being derived from the share of sent conversations that received a reply.
Prompt for AI agents
Address the following comment on apps/web/app/api/user/stats/response-time/route.ts at line 272:
<comment>Response rate is always reported as 100%, because it is hard-coded instead of being derived from the share of sent conversations that received a reply.</comment>
<file context>
@@ -23,215 +74,251 @@ async function getResponseTimeStats({
+ return {
+ medianResponseTime: Math.round(medianResponseTime),
+ averageResponseTime: Math.round(averageResponseTime),
+ responseRate: 100,
+ within1Hour: Math.round(within1Hour),
+ previousPeriodComparison,
</file context>
| isSent = true; | ||
| } | ||
|
|
||
| if (isSent) { |
There was a problem hiding this comment.
Response-time metrics ignore the requested date range: once a thread is selected, every historical reply in that thread is counted, so a 7‑day query still includes months-old responses.
Prompt for AI agents
Address the following comment on apps/web/app/api/user/stats/response-time/route.ts at line 167:
<comment>Response-time metrics ignore the requested date range: once a thread is selected, every historical reply in that thread is counted, so a 7‑day query still includes months-old responses.</comment>
<file context>
@@ -23,215 +74,251 @@ async function getResponseTimeStats({
+ isSent = true;
+ }
+
+ if (isSent) {
+ // Message is SENT
+ if (lastReceivedDate) {
</file context>
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.