feat: lead time breakdown #71
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughAdds period metadata (currentPeriod/previousPeriod) to DORA metrics, introduces a LeadTimeBreakdown type and service with five stages, updates backend query/transformer/resolver shapes and tests, and extends frontend GraphQL types and UI to surface breakdowns and prior-period comparisons. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/metrics-and-insights/page.tsx (1)
143-159: Inconsistent default values for duration metrics.Lead time displays
"0"when there's no data, but MTTR (lines 176-181) displays"0 hours". For consistency in the UI, consider using the same format for both duration-based metrics.Suggested fix
<CardDoraMetric name="Lead time" amount={ metrics.leadTime?.currentAmount ? humanizeDuration(metrics.leadTime?.currentAmount) - : "0" + : "0 hours" } previousAmount={ metrics.leadTime?.previousAmount ? humanizeDuration(metrics.leadTime?.previousAmount) - : "0" + : "0 hours" }
🤖 Fix all issues with AI agents
In `@apps/api/src/app/metrics/services/dora-metrics.types.ts`:
- Around line 36-43: FailureRateResult currently inherits MetricResult's data as
bigint[] which is wrong for percentage/fractional values; update the
FailureRateResult interface to explicitly override the inherited data type to
number[] (e.g., in the FailureRateResult declaration add data: number[]) so
currentAmount/previousAmount remain number and the chart/consumers receive
number[] instead of bigint[]; ensure the module still imports or references
MetricResult and adjust any callers that assumed bigint[] to use number[].
In `@apps/api/src/lib/date.ts`:
- Around line 108-116: In getPreviousPeriod, validate inputs before computing
the previous range: parse from and to into Date objects and check both are valid
(use isNaN(date.getTime())), and ensure toDate.getTime() > fromDate.getTime()
(duration > 0); if validation fails, fast‑fail by throwing a clear Error (e.g.,
"Invalid date range: 'to' must be after 'from'") so invalid or zero‑length
ranges are not propagated. Update the function getPreviousPeriod to perform
these checks and throw the error early.
In
`@apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx`:
- Around line 1-9: The CI formatting warnings come from the import list and the
DateTimeRange/formatLocaleDate usage; run the project's formatter (e.g.,
prettier) or manually reflow the imports so each import or grouped specifier
follows the project's line-break rules and adjust the
DateTimeRange/formatLocaleDate block (lines referencing DateTimeRange and
formatLocaleDate) to match the same line-wrapping style; specifically update the
import group containing Group, HoverCard, Paper, Stack, Text and the
Icon/IconProps imports and reformat the DateTimeRange/formatLocaleDate lines to
satisfy the project's formatter/linter so the warnings disappear.
In
`@apps/web/src/app/metrics-and-insights/lead-time/components/lead-time-breakdown/step.tsx`:
- Around line 115-117: The code passes stage.previousAmount directly into
getAbbreviatedDuration, but currentAmount is guarded with a ternary fallback;
update the JSX that renders previousAmount (the Text showing
getAbbreviatedDuration(stage.previousAmount)) to use the same null/zero-safe
guard as currentAmount (e.g., pass stage.previousAmount ?
getAbbreviatedDuration(stage.previousAmount) : "0s" or equivalent) so
getAbbreviatedDuration always receives a valid value; locate the usage in
step.tsx and mirror the currentAmount handling to prevent null/0 display issues.
In `@apps/web/src/app/metrics-and-insights/lead-time/page.tsx`:
- Around line 33-36: The JSX prop formatting for the LeadTimeBreakdown component
violates the project's formatter rules; split the props onto separate lines and
align them with the component opening tag — update the JSX inside the Box so
<LeadTimeBreakdown breakdown={metrics.leadTime.breakdown}
previousPeriod={metrics.leadTime.previousPeriod} /> is formatted with each prop
on its own line (e.g., <LeadTimeBreakdown\n
breakdown={metrics.leadTime.breakdown}\n
previousPeriod={metrics.leadTime.previousPeriod}\n/>), preserving the Box
wrapper and existing indentation.
In `@apps/web/src/providers/date.provider.ts`:
- Around line 70-77: The abbreviations map in date.provider.ts currently
collides because both "months" and "minutes" use "m"; update the abbreviations
Record so each unit is distinct (e.g., set "months" => "mo" and "minutes" =>
"min") and ensure any code that formats/parses durations using this
abbreviations object (the abbreviations constant) is updated to expect the new
tokens.
🧹 Nitpick comments (3)
apps/api/src/app/metrics/resolvers/queries/dora-metrics.query.ts (1)
59-70: Consider extracting filter construction to a shared helper.The filter object construction is duplicated across all four metric resolvers (
leadTime,changeFailureRate,deploymentFrequency,meanTimeToRecover). A helper function could reduce this repetition.♻️ Example helper
const buildDoraFilters = ( workspaceId: number, input: DoraMetricsInput ): DoraMetricsFilters => ({ workspaceId, dateRange: { from: input.dateRange.from ?? thirtyDaysAgo().toISOString(), to: input.dateRange.to ?? new Date().toISOString(), }, period: input.period, teamIds: input.teamIds ?? undefined, applicationIds: input.applicationIds ?? undefined, environmentIds: input.environmentIds ?? undefined, repositoryIds: input.repositoryIds ?? undefined, });apps/api/src/app/metrics/services/lead-time-breakdown.service.ts (1)
77-111: Consider avoiding mutation of returned arrays.The
conditionsarray returned frombuildDeploymentQueryFiltersis mutated at lines 91-95. While functional, this side-effect pattern can be error-prone if the function is reused elsewhere. Consider either returning a new array or adding the date/archive conditions insidebuildDeploymentQueryFilters.Alternative approach
const buildBreakdownAggregateQuery = ({ args, from, to, }: BreakdownAggregateQueryArgs): Prisma.Sql => { - const { joins, conditions } = buildDeploymentQueryFilters(args, "d"); + const { joins, conditions: baseConditions } = buildDeploymentQueryFilters(args, "d"); + + const conditions = [ + ...baseConditions, + Prisma.sql`d."deployedAt" >= ${new Date(from)}`, + Prisma.sql`d."deployedAt" <= ${new Date(to)}`, + Prisma.sql`d."archivedAt" IS NULL`, + ]; const allJoins = [ ...joins, Prisma.sql`INNER JOIN "DeploymentPullRequest" dpr ON d."id" = dpr."deploymentId"`, Prisma.sql`INNER JOIN "PullRequest" pr ON dpr."pullRequestId" = pr."id"`, Prisma.sql`LEFT JOIN "PullRequestTracking" prt ON pr."id" = prt."pullRequestId"`, ]; - conditions.push( - Prisma.sql`d."deployedAt" >= ${new Date(from)}`, - Prisma.sql`d."deployedAt" <= ${new Date(to)}`, - Prisma.sql`d."archivedAt" IS NULL` - ); - const whereClause = Prisma.join(conditions, " AND ");apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx (1)
115-128: Consider guarding the date-range display when bounds are missing.
IfpreviousPeriod.fromor.tois null, the UI currently renders a dangling hyphen; a conditional render or fallback label would avoid that edge-case display.
apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx
Outdated
Show resolved
Hide resolved
apps/web/src/app/metrics-and-insights/lead-time/components/lead-time-breakdown/step.tsx
Show resolved
Hide resolved
![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 25 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/web/src/providers/date.provider.ts">
<violation number="1" location="apps/web/src/providers/date.provider.ts:72">
P1: Abbreviation collision: both `months` and `minutes` are mapped to `"m"`. This will cause ambiguous output (e.g., "2m" could mean 2 months or 2 minutes). Use a distinct abbreviation for months, such as `"mo"`.</violation>
</file>
<file name="apps/api/src/app/metrics/services/lead-time-breakdown.integration.test.ts">
<violation number="1" location="apps/api/src/app/metrics/services/lead-time-breakdown.integration.test.ts:36">
P2: Misleading comment: the stated durations don't match the actual values being tested. The comment says `review lag: 2h, review: 3h` but the actual durations (computed by the assertions via subtraction) are 1h each. Consider updating the comment to clarify these are cumulative times or list the actual durations: `coding: 1h, review lag: 1h, approval: 1h, merge: 4h, deploy: 5h`.</violation>
</file>
<file name="apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx">
<violation number="1" location="apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx:116">
P3: The hovercard always renders the " - " separator even when `previousPeriod.from` or `previousPeriod.to` is missing (both are optional/null), which can show an empty or partial date range. Consider only rendering the range when both bounds exist or showing a placeholder instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/api/src/app/metrics/services/lead-time-breakdown.integration.test.ts
Outdated
Show resolved
Hide resolved
apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx`:
- Around line 83-133: The displayed "UTC period" uses new Date(...) which
renders in the user's local timezone; replace those usages by constructing
UTCDate(...) (from `@date-fns/utc`) for previousPeriod.from and previousPeriod.to
before passing to formatLocaleDate so the formatted range preserves UTC; add the
import for UTCDate from "@date-fns/utc" and update the references in the
component (look for previousPeriod?.from / previousPeriod?.to and calls to
formatLocaleDate) to use UTCDate instead of new Date.
🧹 Nitpick comments (1)
apps/api/src/app/metrics/services/lead-time-breakdown.integration.test.ts (1)
57-79: Stabilize time-dependent assertions.Using
Date.now()/new Date()can introduce flakiness around midnight/DST or time zone changes. Consider a fixed base time for deployedAt and dateRange, then reuse it across the test (and optionally the file).♻️ Proposed adjustment
- const deployment = await seedDeployment( + const baseTime = new Date("2026-01-15T12:00:00Z"); + const dayMs = 24 * 60 * 60 * 1000; + const deployment = await seedDeployment( ctx, application.applicationId, environment.environmentId, { - deployedAt: new Date(), + deployedAt: baseTime, authorId: gitProfile.gitProfileId, } ); @@ - dateRange: { - from: new Date(Date.now() - 86400000).toISOString(), // 1 day ago - to: new Date(Date.now() + 86400000).toISOString(), // 1 day future - }, + dateRange: { + from: new Date(baseTime.getTime() - dayMs).toISOString(), + to: new Date(baseTime.getTime() + dayMs).toISOString(), + },
apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx`:
- Around line 84-143: The HoverCard trigger (HoverCard.Target) currently wraps a
non-focusable Stack, preventing keyboard users from opening the dropdown; update
the trigger to be keyboard-accessible by making the trigger element focusable
(e.g., add tabIndex={0} to the Stack or wrap contents in a focusable element),
add appropriate ARIA attributes (role="button" and aria-expanded tied to
HoverCard state or a descriptive aria-label), and handle key events (onKeyDown
to open on Enter/Space) so that the previous-period details rendered in
HoverCard.Dropdown (see HoverCard.Target and Stack) are reachable by keyboard
users.
apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.