feat: pr deployed tracking + many misc changes#76
Merged
waltergalvao merged 16 commits intomainfrom Feb 19, 2026
Merged
Conversation
sweetr-dev
Bot
added
the
large
WalkthroughAdds deployment-aware pull request tracking: new types and API to update PR tracking on deployment, call-site integration when creating deployments from merged PRs, merge-commit propagation changes, new timing metrics (timeToCode, timeToDeploy, firstDeployedAt) surfaced through API and UI. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR adds deployment tracking for pull requests (
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| apps/api/src/app/deployment/services/deployment-pr-linking.service.ts | Adds updatePullRequestDeploymentTracking function and renames findPullRequestsByCommitHashes to findMergedPullRequestsByCommitHashes with merged-only filter. Contains a typo in error message ("Depoloyed"). |
| apps/api/src/app/deployment/workers/deployment-triggered-by-pull-request-merge.worker.ts | Refactored to pass pullRequestId instead of full PullRequest object, fetching fresh data with tracking included. |
| apps/api/src/app/github/services/github-pull-request-tracking.service.ts | Adds getTimeToDeploy function and updates getTimeToMerge to accept firstReadyAt as a fallback date. |
| apps/api/src/app/github/services/github-pull-request.service.ts | Adds fetchPullRequestMergeCommitSha REST API fallback for when webhook doesn't provide merge commit SHA. Passes firstReadyAt to getTimeToMerge. |
| apps/api/src/lib/logger.ts | Adds log sanitization for workspace and pullRequest fields, but mutates the caller's object and uses incorrect field names for PullRequest. |
| apps/api/src/app/pull-requests/services/pull-request.service.ts | Refactors findPullRequestById to accept an options object with optional include parameter for Prisma relations. |
| apps/web/src/components/card-pull-request/timeline-pull-request.tsx | Adds "Coding" timeline step with timeToCode metric and info tooltip to the PR timeline visualization. |
Sequence Diagram
sequenceDiagram
participant Webhook as GitHub Webhook
participant SyncWorker as SyncPR Worker
participant PRService as PR Service
participant GitHub as GitHub API
participant DeployWorker as Deploy Worker
participant DeployService as Deploy Service
participant DB as Database
Webhook->>SyncWorker: PR merged event
SyncWorker->>PRService: syncPullRequest(webhookMergeCommitSha)
alt webhookMergeCommitSha missing
PRService->>GitHub: REST API: pulls.get()
GitHub-->>PRService: merge_commit_sha
end
PRService->>DB: upsert PullRequest
PRService->>DB: upsert PullRequestTracking (timeToCode, timeToMerge)
PRService-->>SyncWorker: pullRequest
SyncWorker->>DeployWorker: enqueue(pullRequestId)
DeployWorker->>DB: findPullRequestById(include: tracking)
DeployWorker->>DeployService: createDeploymentFromPullRequestMerge
DeployService->>DB: upsert Deployment
DeployService->>DB: link PR to Deployment
DeployService->>DB: update PullRequestTracking(firstDeployedAt, timeToDeploy)
Last reviewed commit: f94e713
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
apps/api/src/lib/logger.ts (1)
37-37: Redundant|| {}—cleanObjis already guaranteed non-nullish
cleanObjis assignedobj || {}on line 29, so it is never falsy. The|| {}guard on line 37 is dead code.🧹 Nit
- pinoLogger.info(cleanObj || {}, msg); + pinoLogger.info(cleanObj, msg);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/logger.ts` at line 37, The call to pinoLogger.info uses a redundant fallback (pinoLogger.info(cleanObj || {}, msg)) even though cleanObj is already initialized as obj || {} earlier; remove the dead guard and pass cleanObj directly (i.e., change to pinoLogger.info(cleanObj, msg)) so the log call relies on the guaranteed non-nullish cleanObj variable in logger.ts.apps/api/src/app/github/services/github-pull-request-tracking.service.ts (1)
99-104: Consider adding a guard fordeployedAtbeforemergedAt.If
deployedAtis earlier thanmergedAt(e.g., clock skew, data inconsistency),differenceInBusinessMillisecondswould return a negative value. A simple guard orMath.max(0, ...)would make this more defensive.🛡️ Optional defensive guard
export const getTimeToDeploy = (mergedAt: Date, deployedAt: Date) => { + if (deployedAt < mergedAt) return 0; return differenceInBusinessMilliseconds(mergedAt, deployedAt); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/github/services/github-pull-request-tracking.service.ts` around lines 99 - 104, The getTimeToDeploy function can return negative values if deployedAt is earlier than mergedAt; update getTimeToDeploy (which calls differenceInBusinessMilliseconds) to guard against negative results by ensuring you compute max(0, differenceInBusinessMilliseconds(mergedAt, deployedAt)) or by swapping/validating the dates before calling differenceInBusinessMilliseconds so the function always returns zero or positive milliseconds.apps/api/src/app/pull-requests/services/pull-request.service.ts (1)
123-137: Return type doesn't reflect included relations.Since
findPullRequestByIdalways returnsPullRequest | nullregardless of theincludeparameter, callers that passinclude: { tracking: true }won't get type-safe access totrackingon the result without a cast. Consider using a generic signature to propagate the include shape into the return type.This isn't blocking since callers can cast or assert, but it reduces the benefit of the typed
includeparameter.♻️ Example with generic return type
-export const findPullRequestById = async ({ - workspaceId, - pullRequestId, - include = {}, -}: FindPullRequestByIdArgs) => { +export const findPullRequestById = async < + T extends Prisma.PullRequestInclude = {}, +>({ + workspaceId, + pullRequestId, + include = {} as T, +}: Omit<FindPullRequestByIdArgs, 'include'> & { include?: T }) => { return getPrisma(workspaceId).pullRequest.findUnique({ where: { id: pullRequestId, workspaceId, }, include: { ...include, }, - }); + }) as Promise<Prisma.PullRequestGetPayload<{ include: T }> | null>; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/pull-requests/services/pull-request.service.ts` around lines 123 - 137, The function findPullRequestById always types its result as PullRequest | null which ignores the shape of the include parameter; change it to a generic signature so the include shape is propagated to the return type (use Prisma's helper types). For example, make findPullRequestById generic (e.g. <T extends Prisma.PullRequestInclude | null = null>), accept include?: T in FindPullRequestByIdArgs, and set the return type to Promise<Prisma.PullRequestGetPayload<{ include: T }> | null>; keep the existing implementation body (getPrisma(...).pullRequest.findUnique(...)) but ensure the include variable is typed as T so callers receive type-safe included relations like tracking.apps/api/src/app/deployment/services/deployment-create-from-merge.service.ts (1)
56-80: No transaction wraps deployment creation, linking, and tracking update.If
updatePullRequestDeploymentTrackingfails (Line 76), the deployment and PR link are already persisted. SinceupsertDeploymentis idempotent, retries should be safe, but the caller will see a failure despite a partially successful operation. Consider whether this is acceptable or if a transaction would be warranted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/deployment/services/deployment-create-from-merge.service.ts` around lines 56 - 80, The current sequence calls upsertDeployment, linkPullRequestsToDeployment, and updatePullRequestDeploymentTracking separately which can leave persisted state if the final call fails; wrap these three operations in a single database transaction (or use the repository/ORM transaction helper) so that upsertDeployment, linkPullRequestsToDeployment, and updatePullRequestDeploymentTracking are executed atomically and rolled back on error, or alternatively implement a compensating rollback when updatePullRequestDeploymentTracking fails; locate these calls (upsertDeployment, linkPullRequestsToDeployment, updatePullRequestDeploymentTracking) in deployment-create-from-merge.service.ts and move them into a transactional block that commits only after all succeed.packages/graphql-types/frontend/graphql.ts (1)
1797-1797:timeToCodeincluded inPullRequestsQuerybut not in other PR-fetching queries.The
timeToCodefield is now part of thePullRequestsQuerytracking selection. Note that other queries fetching PR tracking data (DeploymentQueryon Line 1577,TeamPullRequestsInProgressQueryon Line 1862,TeamWorkLogQueryon Line 1900) do not includetimeToCode. If this is intentional (e.g., only needed in specific views for now), this is fine — just flagging for awareness in case it should be consistent across all PR tracking selections.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/graphql-types/frontend/graphql.ts` at line 1797, PullRequestsQuery includes the tracking.timeToCode field while other PR-tracking queries (DeploymentQuery, TeamPullRequestsInProgressQuery, TeamWorkLogQuery) do not; decide whether timeToCode should be consistently returned and, if so, add timeToCode to the tracking selection in those queries (or if intentional, leave PullRequestsQuery as-is and add a comment near PullRequestsQuery explaining why timeToCode is only selected there). Update the GraphQL query definitions for DeploymentQuery, TeamPullRequestsInProgressQuery, and TeamWorkLogQuery (or add the explanatory comment in PullRequestsQuery) to reflect the chosen consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/deployment/services/deployment-pr-linking.service.ts`:
- Around line 167-213: The DataIntegrityException message in
updatePullRequestDeploymentTracking contains a typo ("Depoloyed"); update the
thrown DataIntegrityException call to correct the text to "Deployed Pull Request
is not merged" (keep the surrounding context and the extra: { pullRequest }
payload untouched) so the error message reads correctly when
DataIntegrityException is raised.
- Around line 200-212: The update call to
getPrisma(...).pullRequestTracking.update assumes a tracking row exists and can
raise Prisma P2025 when pullRequest.tracking is null; change this to either (a)
guard before calling update and create the tracking row if missing, or (b)
replace the update with pullRequestTracking.upsert using pullRequest.id and
workspaceId as the unique key so it will create or update atomically; ensure
this prevents unhandled errors inside the Promise.all batch that processes
deployments.
In `@apps/api/src/app/deployment/services/deployment-pr-linking.types.ts`:
- Around line 38-43: The UpdatePullRequestDeploymentTrackingArgs type allows
pullRequest.tracking to be null, but updatePullRequestDeploymentTracking
currently calls prisma.pullRequestTracking.update() unguarded; change the
service to either (a) perform an early null check/return when
pullRequest.tracking is null before calling prisma.pullRequestTracking.update(),
or (b) replace the update call with prisma.pullRequestTracking.upsert(...) using
pullRequest.tracking?.id (or pullRequest.id) as the key and providing both
create and update payloads so a missing tracking record is created; update the
service code around prisma.pullRequestTracking.update() accordingly to ensure no
runtime throw when tracking is null.
In
`@apps/api/src/app/deployment/workers/deployment-triggered-by-pull-request-merge.worker.ts`:
- Around line 34-46: The pullRequest returned from findPullRequestById is
inferred with an optional include so TypeScript can't guarantee the tracking
field for the downstream call to createDeploymentFromPullRequestMerge; add a
local type assertion when passing pullRequest (assert as PullRequest & {
tracking: PullRequestTracking | null }) so the callsite satisfies the expected
shape, or alternatively change findPullRequestById to a generic overload that
narrows its return type based on the include parameter; update the call that
supplies pullRequest to createDeploymentFromPullRequestMerge to use the
asserted/narrowed type and ensure imports for PullRequest and
PullRequestTracking are present.
In `@apps/api/src/app/github/services/github-pull-request.service.ts`:
- Around line 195-211: fetchPullRequestMergeCommitSha currently calls
octokit.rest.pulls.get without error handling and can throw; wrap the REST call
in a try/catch inside fetchPullRequestMergeCommitSha, returning undefined on any
failure (matching getFirstCommit's behavior) and optionally log the error (e.g.,
console.error or the module logger) with context including installationId and
gitPrData.number; ensure the function still returns
response.data.merge_commit_sha || undefined on success.
In `@apps/api/src/lib/logger.ts`:
- Around line 29-35: The code currently mutates the caller's object because
const cleanObj = obj || {} only copies the reference; change cleanObj to be a
shallow copy (e.g., const cleanObj = obj ? { ...obj } : {}) so subsequent
cleanObj[key] = pick(...) does not modify obj, and ensure the sanitized object
(cleanObj) is passed to Sentry.addBreadcrumb (instead of obj); update usages
around cleanObj, loggableFields and pick to operate on the new shallow copy so
the original obj remains unchanged.
- Line 24: The loggable fields array for pullRequest is wrong — replace the
current array ["id", "name", "description", "createdAt", "updatedAt"] with the
correct PullRequest fields such as ["id", "title", "createdAt", "updatedAt"] (or
add other valid fields like "number" or "state" if desired) so the logger picks
up the PR title and valid attributes; update the pullRequest array definition in
logger.ts accordingly (refer to the pullRequest variable/constant where the
array is declared).
- Around line 31-34: The TypeScript error comes from indexing cleanObj (typed as
object) and loggableFields with a general string; update the usage in the loop
inside apps/api/src/lib/logger.ts (symbols: cleanObj, loggableFields, pick) so
cleanObj is treated as Record<string, unknown> (either change the parameter type
obj?: object to obj?: Record<string, unknown> or cast cleanObj to that type
before indexing) and guard the access to loggableFields with a proper key check
(use `if (key in loggableFields)` and index as `loggableFields[key as keyof
typeof loggableFields]`) before calling pick, ensuring the types align for
pick's arguments.
In `@apps/web/src/components/card-pull-request/timeline-pull-request.tsx`:
- Around line 132-134: The Text node rendering the coding duration uses
{timeToCode && <>{humanizeDuration(timeToCode)}</>} which omits the "in " prefix
and skips rendering when timeToCode is 0; update this to render consistently
with other entries by checking for a defined numeric value (e.g., timeToCode !==
null && timeToCode !== undefined or typeof timeToCode === "number") and render
the prefix string "in " followed by humanizeDuration(timeToCode) (referencing
the timeToCode variable and humanizeDuration function in the Text component) so
zero durations still display "in 0s" while maintaining consistency with other
timeline entries.
---
Nitpick comments:
In
`@apps/api/src/app/deployment/services/deployment-create-from-merge.service.ts`:
- Around line 56-80: The current sequence calls upsertDeployment,
linkPullRequestsToDeployment, and updatePullRequestDeploymentTracking separately
which can leave persisted state if the final call fails; wrap these three
operations in a single database transaction (or use the repository/ORM
transaction helper) so that upsertDeployment, linkPullRequestsToDeployment, and
updatePullRequestDeploymentTracking are executed atomically and rolled back on
error, or alternatively implement a compensating rollback when
updatePullRequestDeploymentTracking fails; locate these calls (upsertDeployment,
linkPullRequestsToDeployment, updatePullRequestDeploymentTracking) in
deployment-create-from-merge.service.ts and move them into a transactional block
that commits only after all succeed.
In `@apps/api/src/app/github/services/github-pull-request-tracking.service.ts`:
- Around line 99-104: The getTimeToDeploy function can return negative values if
deployedAt is earlier than mergedAt; update getTimeToDeploy (which calls
differenceInBusinessMilliseconds) to guard against negative results by ensuring
you compute max(0, differenceInBusinessMilliseconds(mergedAt, deployedAt)) or by
swapping/validating the dates before calling differenceInBusinessMilliseconds so
the function always returns zero or positive milliseconds.
In `@apps/api/src/app/pull-requests/services/pull-request.service.ts`:
- Around line 123-137: The function findPullRequestById always types its result
as PullRequest | null which ignores the shape of the include parameter; change
it to a generic signature so the include shape is propagated to the return type
(use Prisma's helper types). For example, make findPullRequestById generic (e.g.
<T extends Prisma.PullRequestInclude | null = null>), accept include?: T in
FindPullRequestByIdArgs, and set the return type to
Promise<Prisma.PullRequestGetPayload<{ include: T }> | null>; keep the existing
implementation body (getPrisma(...).pullRequest.findUnique(...)) but ensure the
include variable is typed as T so callers receive type-safe included relations
like tracking.
In `@apps/api/src/lib/logger.ts`:
- Line 37: The call to pinoLogger.info uses a redundant fallback
(pinoLogger.info(cleanObj || {}, msg)) even though cleanObj is already
initialized as obj || {} earlier; remove the dead guard and pass cleanObj
directly (i.e., change to pinoLogger.info(cleanObj, msg)) so the log call relies
on the guaranteed non-nullish cleanObj variable in logger.ts.
In `@packages/graphql-types/frontend/graphql.ts`:
- Line 1797: PullRequestsQuery includes the tracking.timeToCode field while
other PR-tracking queries (DeploymentQuery, TeamPullRequestsInProgressQuery,
TeamWorkLogQuery) do not; decide whether timeToCode should be consistently
returned and, if so, add timeToCode to the tracking selection in those queries
(or if intentional, leave PullRequestsQuery as-is and add a comment near
PullRequestsQuery explaining why timeToCode is only selected there). Update the
GraphQL query definitions for DeploymentQuery, TeamPullRequestsInProgressQuery,
and TeamWorkLogQuery (or add the explanatory comment in PullRequestsQuery) to
reflect the chosen consistency.
Comment on lines
+31
to
+34
| for (const key of Object.keys(cleanObj)) { | ||
| if (loggableFields[key]) { | ||
| cleanObj[key] = pick(cleanObj[key], loggableFields[key]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check tsconfig for strict/noImplicitAny settings
fd 'tsconfig.*\.json' apps/api --exec cat {}Repository: sweetr-dev/sweetr.dev
Length of output: 958
🏁 Script executed:
# Read the logger.ts file to see the complete context
cat -n apps/api/src/lib/logger.ts | head -60Repository: sweetr-dev/sweetr.dev
Length of output: 1911
TypeScript type errors: object cannot be indexed by string
cleanObj is typed as object (from obj?: object), which does not allow bracket-notation access with a string key. Additionally, loggableFields is typed as { workspace: string[]; pullRequest: string[] } — accessing it with a bare string key is a type error since the key is not restricted to the literal union "workspace" | "pullRequest".
The proposed fix (casting cleanObj to Record<string, unknown> and using key in loggableFields + key as keyof typeof loggableFields) resolves both issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/lib/logger.ts` around lines 31 - 34, The TypeScript error comes
from indexing cleanObj (typed as object) and loggableFields with a general
string; update the usage in the loop inside apps/api/src/lib/logger.ts (symbols:
cleanObj, loggableFields, pick) so cleanObj is treated as Record<string,
unknown> (either change the parameter type obj?: object to obj?: Record<string,
unknown> or cast cleanObj to that type before indexing) and guard the access to
loggableFields with a proper key check (use `if (key in loggableFields)` and
index as `loggableFields[key as keyof typeof loggableFields]`) before calling
pick, ensuring the types align for pick's arguments.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 23 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/lib/logger.ts">
<violation number="1" location="apps/api/src/lib/logger.ts:24">
P2: Incorrect field names for `pullRequest` in loggable fields. The PullRequest model uses `title` (not `name`) and doesn't have a `description` field. This will silently drop the PR title from every info log since these fields are undefined on the model.</violation>
<violation number="2" location="apps/api/src/lib/logger.ts:29">
P2: Avoid mutating the caller’s log object; clone it before sanitizing so logging doesn’t change application state.</violation>
</file>
<file name="apps/api/src/app/deployment/services/deployment-pr-linking.service.ts">
<violation number="1" location="apps/api/src/app/deployment/services/deployment-pr-linking.service.ts:174">
P3: Typo in error message: "Depoloyed" should be "Deployed".</violation>
<violation number="2" location="apps/api/src/app/deployment/services/deployment-pr-linking.service.ts:200">
P2: Guard for missing pullRequest.tracking (or use a safe update) before calling `pullRequestTracking.update`, otherwise PRs without tracking rows will throw and stop deployment linking.</violation>
</file>
<file name="apps/api/src/app/github/services/github-pull-request.service.ts">
<violation number="1" location="apps/api/src/app/github/services/github-pull-request.service.ts:204">
P2: Missing error handling on REST API call. If this GitHub API call fails (rate limit, 404 for deleted repo, network error), the entire `upsertPullRequest` flow will throw and the PR sync will fail. Wrap in try/catch and return `undefined` on error, similar to `getFirstCommit`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/lib/logger.ts (1)
42-42:⚠️ Potential issue | 🟡 MinorSentry breadcrumb still receives the unsanitized
objThe
pick-based redaction applied tocleanObj(lines 31–35) is bypassed here — Sentry receives the raw original object with all fields intact. The prior commit addressed the mutation issue but left the breadcrumb data unchanged.🛡️ Proposed fix
addBreadcrumb({ message: msg, category: "log", level: "info", - data: obj, + data: cleanObj, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/logger.ts` at line 42, The Sentry breadcrumb is currently being sent the unsanitized original object (data: obj) and thus bypasses the redaction done earlier with pick/cleanObj; update the breadcrumb to use the sanitized/derived value (e.g., data: cleanObj or JSON.stringify(cleanObj) if serialization is required) and ensure you do not mutate obj—refer to the existing cleanObj and pick logic and replace the usage of obj in the breadcrumb creation (the data: field) with the sanitized cleanObj.
🧹 Nitpick comments (1)
apps/api/src/lib/logger.ts (1)
37-37:cleanObj || {}is always the truthy branch — the fallback is dead code
cleanObjis initialised unconditionally to a spread object literal on line 29, so it is never falsy.♻️ Proposed simplification
- pinoLogger.info(cleanObj || {}, msg); + pinoLogger.info(cleanObj, msg);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/logger.ts` at line 37, The expression uses a dead fallback: remove the redundant "|| {}" and pass the actual cleanObj directly to pinoLogger.info (update the call site pinoLogger.info(cleanObj || {}, msg) to pinoLogger.info(cleanObj, msg)); if the original intent was to allow undefined, instead make cleanObj possibly undefined at its initializer and keep the single-argument call consistent. Target the pinoLogger.info call and the cleanObj variable in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/api/src/lib/logger.ts`:
- Line 42: The Sentry breadcrumb is currently being sent the unsanitized
original object (data: obj) and thus bypasses the redaction done earlier with
pick/cleanObj; update the breadcrumb to use the sanitized/derived value (e.g.,
data: cleanObj or JSON.stringify(cleanObj) if serialization is required) and
ensure you do not mutate obj—refer to the existing cleanObj and pick logic and
replace the usage of obj in the breadcrumb creation (the data: field) with the
sanitized cleanObj.
---
Duplicate comments:
In `@apps/api/src/app/deployment/services/deployment-pr-linking.service.ts`:
- Around line 200-212: The update call to
getPrisma(workspaceId).pullRequestTracking.update can throw Prisma P2025 if
pullRequest.tracking is null; replace the update with an upsert on
pullRequestTracking (use pullRequestId and workspaceId in the where clause, set
the same fields in data, and provide a create object with pullRequestId,
workspaceId, firstDeployedAt and timeToDeploy) so missing tracking rows are
created atomically and the Promise.all won't fail due to a missing row; locate
the call in deployment-pr-linking.service.ts (the block referencing
pullRequest.tracking and getTimeToDeploy) and swap update -> upsert and populate
create and update payloads accordingly.
In `@apps/api/src/lib/logger.ts`:
- Around line 31-34: The loop indexing causes TypeScript errors because cleanObj
is typed as {} and loggableFields is a narrow union; update the loop to use a
runtime guard "if (key in loggableFields)" and narrow/cast types before
indexing: treat cleanObj as Record<string, unknown> (or cast cleanObj[key] as
any/unknown) and cast loggableFields[key] to the appropriate type when calling
pick so TypeScript accepts cleanObj[key] and loggableFields[key]; adjust the
for-loop that iterates Object.keys(cleanObj) and references cleanObj,
loggableFields, and pick accordingly.
---
Nitpick comments:
In `@apps/api/src/lib/logger.ts`:
- Line 37: The expression uses a dead fallback: remove the redundant "|| {}" and
pass the actual cleanObj directly to pinoLogger.info (update the call site
pinoLogger.info(cleanObj || {}, msg) to pinoLogger.info(cleanObj, msg)); if the
original intent was to allow undefined, instead make cleanObj possibly undefined
at its initializer and keep the single-argument call consistent. Target the
pinoLogger.info call and the cleanObj variable in this file.
Contributor
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts">
<violation number="1" location="apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts:76">
P2: timeToDeploy should be null until the PR is merged; falling back to createdAt computes a misleading duration for unmerged PRs. Use mergedAt as the start time so calculateTimeForEvent returns null when it hasn’t merged yet.</violation>
</file>
<file name="apps/web/src/components/card-pull-request/use-badges.tsx">
<violation number="1" location="apps/web/src/components/card-pull-request/use-badges.tsx:183">
P2: The early return requires `isDeployed` to be true, which makes the “Not deployed”/“Stuck on deploy” label logic unreachable. This prevents the deploy badge from showing warning/error states for merged-but-not-deployed PRs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts (1)
40-87:⚠️ Potential issue | 🟠 MajorCompute timeToDeploy only after merge to avoid misleading durations.
When
mergedAtis null, the current fallback usescreatedAt, which inflates “pending deploy” for unmerged PRs. UsemergedAtonly so open PRs don’t get a deploy timer.🛠️ Fix timeToDeploy start point
timeToDeploy: calculateTimeForEvent({ - from: pullRequest.mergedAt || pullRequest.createdAt, + from: pullRequest.mergedAt, duration: tracking.timeToDeploy, pullRequest, uselessAfterMerge: false, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts` around lines 40 - 87, The timeToDeploy currently falls back to pullRequest.createdAt when mergedAt is null, inflating deploy time for open PRs; change the timeToDeploy field so it only calls calculateTimeForEvent when pullRequest.mergedAt is present (e.g., timeToDeploy: pullRequest.mergedAt ? calculateTimeForEvent({ from: pullRequest.mergedAt, duration: tracking.timeToDeploy, pullRequest, uselessAfterMerge: false }) : null) so open/unmerged PRs return null/absent deploy timing instead of a misleading duration.
🧹 Nitpick comments (3)
apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts (1)
24-37: Return explicit nulls for new fields when tracking is absent.Keeps the output shape stable and avoids
undefinedleaking to clients.♻️ Suggested tweak
return { size: PullRequestSize.MEDIUM, changedFilesCount: 0, linesAddedCount: 0, linesDeletedCount: 0, firstApprovalAt: null, firstReviewAt: null, timeToCode: null, + firstDeployedAt: null, + timeToDeploy: null, timeToFirstApproval: null, timeToFirstReview: null, timeToMerge: null, cycleTime: null, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts` around lines 24 - 37, When tracking is falsy in pull-request-tracking.transformer.ts, the fallback object currently omits some new fields which can lead to undefined values leaking to clients; update the early return where it checks if (!tracking) to include explicit nulls for every new nullable field (e.g., firstApprovalAt, firstReviewAt, timeToCode, timeToFirstApproval, timeToFirstReview, timeToMerge, cycleTime) while keeping existing defaults (size: PullRequestSize.MEDIUM, changedFilesCount: 0, linesAddedCount: 0, linesDeletedCount: 0) so the output shape from the transformer is stable and all fields are defined.apps/api/src/app/deployment/services/deployment-pr-linking.service.ts (2)
145-152: Defensive mergedAt check is redundant but acceptable as a safety net.
findMergedPullRequestsByCommitHashes(line 291) already filters withmergedAt: { not: null }, sofilteredPullRequestsshould never contain un-merged PRs. This check is purely defensive. If you keep it, consider downgrading from a thrown exception to a warning log with early return, since throwing here aborts the entire linking flow for what would be a database-level inconsistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/deployment/services/deployment-pr-linking.service.ts` around lines 145 - 152, The current defensive check in handleDeploymentPullRequestAutoLinking that throws DataIntegrityException when filteredPullRequests contains a PR with a falsy mergedAt should be changed to log a warning and early return instead of throwing; locate the filteredPullRequests usage and the call site tied to findMergedPullRequestsByCommitHashes (which already filters mergedAt: { not: null }) and replace the throw with a processLogger.warn or equivalent warning that includes the filteredPullRequests and the fact that mergedAt was unexpectedly missing, then return early from handleDeploymentPullRequestAutoLinking to avoid aborting the whole linking flow.
160-177:Promise.allSettledresults are discarded — failures are silently swallowed.The
DataIntegrityExceptionthrown at line 163 and any Prisma errors fromupdatePullRequestDeploymentTrackingwill be captured as rejected settlements, but since the return value ofPromise.allSettledis never inspected, these failures are completely silent. This makes debugging deployment tracking issues difficult.Consider either logging rejected results or switching to
Promise.allif you want failures to propagate.♻️ Option: Log rejected settlements
- await Promise.allSettled( + const results = await Promise.allSettled( filteredPullRequests.map(async (pr) => { if (!pr.tracking) { throw new DataIntegrityException( "[updatePullRequestDeploymentTracking] Pull Request tracking not found", { extra: { pr }, } ); } return updatePullRequestDeploymentTracking({ pullRequest: pr as PullRequest & { tracking: PullRequestTracking }, deployment, workspaceId, }); }) ); + + const rejected = results.filter( + (r): r is PromiseRejectedResult => r.status === "rejected" + ); + + if (rejected.length > 0) { + logger.error( + "handleDeploymentPullRequestAutoLinking: Some PR tracking updates failed", + { + deploymentId, + workspaceId, + errors: rejected.map((r) => r.reason), + } + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/deployment/services/deployment-pr-linking.service.ts` around lines 160 - 177, The Promise.allSettled call over filteredPullRequests currently discards settlement results so thrown DataIntegrityException and any errors from updatePullRequestDeploymentTracking are silently ignored; change this by either replacing Promise.allSettled with Promise.all to let rejections propagate (so the caller sees failures) or keep allSettled but capture its result array, iterate over the settlements and log or rethrow rejected entries (including the reason and the associated pr) before returning; update the block referencing Promise.allSettled, filteredPullRequests, updatePullRequestDeploymentTracking, and DataIntegrityException accordingly to ensure errors are surfaced or logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/api/src/app/deployment/workers/deployment-triggered-by-pull-request-merge.worker.ts`:
- Around line 27-41: The code currently checks workspaceId but not
pullRequestId, so add a defensive guard for pullRequestId (from job.data) before
calling findPullRequestById: if pullRequestId is missing or falsy, throw a
ResourceNotFoundException("Pull request not found", { extra: { data: job.data }
}) to mirror the workspaceId check and avoid passing an undefined pullRequestId
into findPullRequestById; update any surrounding comments and ensure the thrown
exception type and extra payload match the existing workspaceId guard.
In
`@apps/api/src/app/pull-requests/resolvers/queries/pull-requests-tracking.query.ts`:
- Around line 26-33: transformPullRequestTracking is being fed Invalid Date
because parseISO is called on Prisma Date objects; change the construction of
the pullRequest object so you only call parseISO when the value is a string and
otherwise pass the Date through (or convert to ISO string if
transformPullRequestTracking expects strings). Concretely, update the fields
createdAt and mergedAt in the object passed to transformPullRequestTracking to:
if typeof pullRequest.createdAt === 'string' then
parseISO(pullRequest.createdAt) else pullRequest.createdAt (and same conditional
for mergedAt), so parseISO is never given a Date instance.
In `@apps/web/src/components/card-pull-request/use-badges.tsx`:
- Line 183: getLabel() currently has unreachable branches because the guard if
(!isNumber(timeToDeploy) || !isDeployed) return null forces isDeployed===true
for the rest of the function, so the "Stuck on deploy" and "Not deployed"
branches can never run; fix by either (A) simplifying getLabel() to only return
the "Deployed" label and remove the dead "Stuck on deploy"/"Not deployed"
branches if undeployed states are out of scope, or (B) support undeployed states
by changing the guard and/or inputs: stop gating on isDeployed with
timeToDeploy, introduce or use a separate metric (e.g., timeSinceMerge or
mergeTimestamp) and branch on isDeployed separately inside getLabel() so you can
return "Not deployed" (when !isDeployed and threshold not exceeded) or "Stuck on
deploy" (when !isDeployed and elapsed > threshold), updating any callers that
rely on timeToDeploy/isDeployed accordingly.
---
Outside diff comments:
In
`@apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts`:
- Around line 40-87: The timeToDeploy currently falls back to
pullRequest.createdAt when mergedAt is null, inflating deploy time for open PRs;
change the timeToDeploy field so it only calls calculateTimeForEvent when
pullRequest.mergedAt is present (e.g., timeToDeploy: pullRequest.mergedAt ?
calculateTimeForEvent({ from: pullRequest.mergedAt, duration:
tracking.timeToDeploy, pullRequest, uselessAfterMerge: false }) : null) so
open/unmerged PRs return null/absent deploy timing instead of a misleading
duration.
---
Duplicate comments:
In `@apps/web/src/components/card-pull-request/timeline-pull-request.tsx`:
- Around line 126-152: The "Coding" Timeline.Item currently renders just the
duration value (using timeToCode and humanizeDuration) but needs the same "in
{duration}" prefix used elsewhere; update the JSX inside Timeline.Item (where
timeToCode and humanizeDuration are used) to render the literal "in " before the
duration (e.g., output "in " + humanizeDuration(timeToCode) only when
isNumber(timeToCode) is true) so the display matches other steps.
---
Nitpick comments:
In `@apps/api/src/app/deployment/services/deployment-pr-linking.service.ts`:
- Around line 145-152: The current defensive check in
handleDeploymentPullRequestAutoLinking that throws DataIntegrityException when
filteredPullRequests contains a PR with a falsy mergedAt should be changed to
log a warning and early return instead of throwing; locate the
filteredPullRequests usage and the call site tied to
findMergedPullRequestsByCommitHashes (which already filters mergedAt: { not:
null }) and replace the throw with a processLogger.warn or equivalent warning
that includes the filteredPullRequests and the fact that mergedAt was
unexpectedly missing, then return early from
handleDeploymentPullRequestAutoLinking to avoid aborting the whole linking flow.
- Around line 160-177: The Promise.allSettled call over filteredPullRequests
currently discards settlement results so thrown DataIntegrityException and any
errors from updatePullRequestDeploymentTracking are silently ignored; change
this by either replacing Promise.allSettled with Promise.all to let rejections
propagate (so the caller sees failures) or keep allSettled but capture its
result array, iterate over the settlements and log or rethrow rejected entries
(including the reason and the associated pr) before returning; update the block
referencing Promise.allSettled, filteredPullRequests,
updatePullRequestDeploymentTracking, and DataIntegrityException accordingly to
ensure errors are surfaced or logged.
In
`@apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts`:
- Around line 24-37: When tracking is falsy in
pull-request-tracking.transformer.ts, the fallback object currently omits some
new fields which can lead to undefined values leaking to clients; update the
early return where it checks if (!tracking) to include explicit nulls for every
new nullable field (e.g., firstApprovalAt, firstReviewAt, timeToCode,
timeToFirstApproval, timeToFirstReview, timeToMerge, cycleTime) while keeping
existing defaults (size: PullRequestSize.MEDIUM, changedFilesCount: 0,
linesAddedCount: 0, linesDeletedCount: 0) so the output shape from the
transformer is stable and all fields are defined.
Contributor
Author
Contributor
Author
|
@CodeRabbit review again |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/graphql-types/frontend/gql.ts (1)
37-37: Auto-generated file — changes look consistent with the new tracking fields.The new
timeToCode,timeToDeploy,firstDeployedAt, andcycleTimefields are added uniformly across all relevant query documents and their corresponding overloads.One observation: the PR tracking field set is duplicated verbatim 4 times within
TeamPullRequestsInProgressand 3 times withinTeamWorkLog. If the source.graphqlfiles (or inlinegraphql()calls) used a shared GraphQL fragment, the generated output would be smaller and future field additions would only need a single-point change.Also applies to: 66-66, 74-74, 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/graphql-types/frontend/gql.ts` at line 37, The generated queries duplicate the pull-request tracking field set (timeToCode, timeToDeploy, firstDeployedAt, cycleTime and other tracking subfields) across TeamPullRequestsInProgress and TeamWorkLog; update the source .graphql files or the inline graphql() calls that define TeamPullRequestsInProgress and TeamWorkLog to extract that repeated selection into a shared GraphQL fragment (e.g., PullRequestTrackingFragment) and reference that fragment from each query so the generated gql.ts (types.DeploymentDocument / the TeamPullRequestsInProgress and TeamWorkLog query documents) no longer include verbatim duplicates.apps/api/src/app/deployment/workers/deployment-triggered-by-pull-request-merge.worker.ts (1)
49-53: Consider separating the "PR not found" and "tracking not found" error cases.
!pullRequest?.trackingconflates two distinct failure modes: the pull request itself not existing, and the PR existing but lacking a tracking record. Both throw the same"Pull request not found"message. While theextrapayload includespullRequestfor debugging, splitting these into two explicit checks would make log triage faster.💡 Suggested separation
- if (!pullRequest?.tracking) { - throw new ResourceNotFoundException("Pull request not found", { - extra: { data: job.data, pullRequest }, - }); - } + if (!pullRequest) { + throw new ResourceNotFoundException("Pull request not found", { + extra: { data: job.data }, + }); + } + + if (!pullRequest.tracking) { + throw new ResourceNotFoundException("Pull request tracking not found", { + extra: { data: job.data, pullRequestId: pullRequest.id }, + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/deployment/workers/deployment-triggered-by-pull-request-merge.worker.ts` around lines 49 - 53, Split the combined null-check into two explicit checks so the code distinguishes "pull request not found" from "tracking not found": first check if pullRequest is falsy and throw ResourceNotFoundException with message "Pull request not found" and include extra: { data: job.data, pullRequest } (or omit pullRequest if null), then separately check if pullRequest.tracking is falsy and throw ResourceNotFoundException with message "Pull request tracking not found" including extra: { data: job.data, pullRequestId: pullRequest?.id, tracking: pullRequest?.tracking } to aid debugging; update any surrounding uses (e.g., in the worker handler where pullRequest is read) to assume tracking exists only after the second check.apps/web/src/api/teams.api.ts (1)
169-183: Recommend extracting GraphQL fragments to reduce code duplication.The full PR field selection—including all 15 fields in the
trackingblock—is repeated verbatim acrossdrafted,pendingReview,changesRequested, andpendingMerge. This became evident when adding the four new tracking fields (timeToCode,timeToDeploy,firstDeployedAt,cycleTime), which required synchronized changes in all four locations.GraphQL named fragments would collapse this to a single definition:
♻️ Proposed refactor using named fragments
graphql(/* GraphQL */ ` + fragment PrTrackingFields on PullRequestTracking { + size + changedFilesCount + linesAddedCount + linesDeletedCount + timeToCode + timeToFirstReview + timeToFirstApproval + timeToMerge + timeToDeploy + firstReviewAt + firstApprovalAt + firstDeployedAt + cycleTime + } + + fragment PrInProgressFields on PullRequest { + id + title + gitUrl + commentCount + changedFilesCount + linesAddedCount + linesDeletedCount + state + createdAt + mergedAt + closedAt + tracking { + ...PrTrackingFields + } + author { + id + avatar + handle + name + } + repository { + id + name + fullName + } + } + query TeamPullRequestsInProgress( $workspaceId: SweetID! $teamId: SweetID! ) { workspace(workspaceId: $workspaceId) { id team(teamId: $teamId) { id pullRequestsInProgress { - drafted { - id - title - ... - } - pendingReview { - id - title - ... - } - changesRequested { - id - title - ... - } - pendingMerge { - id - title - ... - } + drafted { ...PrInProgressFields } + pendingReview { ...PrInProgressFields } + changesRequested { ...PrInProgressFields } + pendingMerge { ...PrInProgressFields } } } } } `)Also applies to: 208-222, 247-261, 286-300
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/api/src/app/deployment/workers/deployment-triggered-by-pull-request-merge.worker.ts`:
- Around line 13-53: No changes required: the previous issues are resolved by
adding the pullRequestId guard in deploymentTriggeredByPullRequestMergeWorker
and switching to fetching the PullRequest via findPullRequestById (with include:
{ tracking: true }) instead of serializing the full PullRequest in job data;
keep the current guards and call site type assertion as-is.
---
Nitpick comments:
In
`@apps/api/src/app/deployment/workers/deployment-triggered-by-pull-request-merge.worker.ts`:
- Around line 49-53: Split the combined null-check into two explicit checks so
the code distinguishes "pull request not found" from "tracking not found": first
check if pullRequest is falsy and throw ResourceNotFoundException with message
"Pull request not found" and include extra: { data: job.data, pullRequest } (or
omit pullRequest if null), then separately check if pullRequest.tracking is
falsy and throw ResourceNotFoundException with message "Pull request tracking
not found" including extra: { data: job.data, pullRequestId: pullRequest?.id,
tracking: pullRequest?.tracking } to aid debugging; update any surrounding uses
(e.g., in the worker handler where pullRequest is read) to assume tracking
exists only after the second check.
In `@packages/graphql-types/frontend/gql.ts`:
- Line 37: The generated queries duplicate the pull-request tracking field set
(timeToCode, timeToDeploy, firstDeployedAt, cycleTime and other tracking
subfields) across TeamPullRequestsInProgress and TeamWorkLog; update the source
.graphql files or the inline graphql() calls that define
TeamPullRequestsInProgress and TeamWorkLog to extract that repeated selection
into a shared GraphQL fragment (e.g., PullRequestTrackingFragment) and reference
that fragment from each query so the generated gql.ts (types.DeploymentDocument
/ the TeamPullRequestsInProgress and TeamWorkLog query documents) no longer
include verbatim duplicates.
Contributor
Additional Comments (1)
The null-tracking fallback object explicitly sets all other fields to |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx (1)
106-111: LGTM — arrow direction now correctly reflectshigherIsBetter.All four sign ×
higherIsBettercombinations produce the expected arrow, and the logic is consistent withgetTrendColor().One optional simplification: since this block is already guarded by
change !== 0,change >= 0is effectivelychange > 0, so the whole ternary condition collapses to(change > 0) === higherIsBetter.♻️ Optional simplification
- {(change >= 0 && higherIsBetter) || - (change < 0 && !higherIsBetter) ? ( + {(change > 0) === higherIsBetter ? ( <IconArrowUpRight size="1rem" stroke={1.5} /> ) : ( <IconArrowDownRight size="1rem" stroke={1.5} /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx` around lines 106 - 111, The ternary rendering for IconArrowUpRight/IconArrowDownRight can be simplified: since the surrounding code already ensures change !== 0, replace the condition ((change >= 0 && higherIsBetter) || (change < 0 && !higherIsBetter)) with the simpler expression (change > 0) === higherIsBetter so the arrow direction logic (used with IconArrowUpRight and IconArrowDownRight) is clearer and remains consistent with getTrendColor().apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts (1)
92-101:pullRequestparameter should be required, not optionalTyping
pullRequestas optional (pullRequest?: PullRequest) means a future caller could omit it, silently bypassing theCLOSEDanduselessAfterMergeguards (lines 107–113). All current call sites always provide it.♻️ Proposed change
const calculateTimeForEvent = ({ from, duration, pullRequest, uselessAfterMerge = false, }: { from: Date | null; duration: bigint | null; - pullRequest?: PullRequest; + pullRequest: PullRequest; uselessAfterMerge?: boolean; }) => {With this change the two optional-chaining accesses (
pullRequest?.state) become plain property accesses (pullRequest.state), removing unnecessary guards.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts` around lines 92 - 101, Change the calculateTimeForEvent function signature to require the pullRequest parameter (make pullRequest: PullRequest instead of pullRequest?: PullRequest), update the internal checks that use optional chaining (replace pullRequest?.state with pullRequest.state) and ensure the existing CLOSED/uselessAfterMerge guard logic still runs; this prevents callers from omitting pullRequest and lets you remove the unnecessary null-safe accesses while preserving the current behavior around pullRequest.state and uselessAfterMerge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/deployment/services/deployment-pr-linking.service.ts`:
- Around line 161-182: Promise.allSettled on filteredPullRequests currently
discards results so rejections from updatePullRequestDeploymentTracking are
swallowed; change it to collect the settled results, iterate over them, and call
captureException for any result.status === "rejected" (include the rejection
reason and contextual info such as the associated pr, deployment, and
workspaceId). Locate the Promise.allSettled call around
filteredPullRequests.map(...) and the updatePullRequestDeploymentTracking
invoke, then after awaiting Promise.allSettled inspect each settled entry and
captureException with the error (and optionally wrap in a
DataIntegrityException-like message) for any rejected promise so errors are
logged instead of ignored.
In
`@apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts`:
- Around line 77-82: The timeToDeploy field is incorrectly using
pullRequest.mergedAt || pullRequest.createdAt, causing open PRs to yield their
age; change the call in the timeToDeploy assignment to pass only
pullRequest.mergedAt (remove the "|| pullRequest.createdAt" fallback) so
calculateTimeForEvent can hit its existing !from guard and return null for
un-merged PRs; update the timeToDeploy assignment where calculateTimeForEvent is
invoked to use from: pullRequest.mergedAt and keep uselessAfterMerge: false.
- Around line 56-64: The live calculation for timeToFirstApproval is using
tracking.firstApprovalAt as the earliest "from" candidate which yields a
negative/incorrect elapsed time when duration is null; change the call to
calculateTimeForEvent inside timeToFirstApproval to remove
tracking.firstApprovalAt from the from chain so it uses tracking.firstReadyAt ||
pullRequest.createdAt as the baseline (leave tracking.timeToFirstApproval as the
duration check intact because the precomputed-duration path already covers the
approved case).
---
Nitpick comments:
In
`@apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts`:
- Around line 92-101: Change the calculateTimeForEvent function signature to
require the pullRequest parameter (make pullRequest: PullRequest instead of
pullRequest?: PullRequest), update the internal checks that use optional
chaining (replace pullRequest?.state with pullRequest.state) and ensure the
existing CLOSED/uselessAfterMerge guard logic still runs; this prevents callers
from omitting pullRequest and lets you remove the unnecessary null-safe accesses
while preserving the current behavior around pullRequest.state and
uselessAfterMerge.
In
`@apps/web/src/app/metrics-and-insights/components/card-dora-metric/dora-card-stat.tsx`:
- Around line 106-111: The ternary rendering for
IconArrowUpRight/IconArrowDownRight can be simplified: since the surrounding
code already ensures change !== 0, replace the condition ((change >= 0 &&
higherIsBetter) || (change < 0 && !higherIsBetter)) with the simpler expression
(change > 0) === higherIsBetter so the arrow direction logic (used with
IconArrowUpRight and IconArrowDownRight) is clearer and remains consistent with
getTrendColor().
Contributor
Additional Comments (1)
This means Consider using the freshly computed |
Contributor
Author
|
@coderabbitai resume |
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/src/app/metrics-and-insights/page.tsx">
<violation number="1" location="apps/web/src/app/metrics-and-insights/page.tsx:133">
P2: The fallback for previousAmount never triggers because the string concatenation happens before the `||`, leading to "undefined deployments" when previousAmount is missing. Apply the fallback before concatenating the suffix.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
✅ Actions performedReviews resumed. |
sweetrdev
reviewed
Feb 18, 2026
Contributor
sweetrdev
left a comment
sweetrdev
left a comment
There was a problem hiding this comment.
Please address all agents comments
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
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)
171-186:⚠️ Potential issue | 🟡 MinorFallback
"0 hours"is inconsistent with the abbreviated duration format.When data is present,
getAbbreviatedDurationproduces strings like"2d 5h"or"3h 20m", and returns"0s"for zero input. The fallback"0 hours"on lines 178 and 185 doesn't match this style.Proposed fix
amount={ metrics.meanTimeToRecover?.currentAmount ? getAbbreviatedDuration( metrics.meanTimeToRecover?.currentAmount, ) - : "0 hours" + : "0s" } previousAmount={ metrics.meanTimeToRecover?.previousAmount ? getAbbreviatedDuration( metrics.meanTimeToRecover?.previousAmount, ) - : "0 hours" + : "0s" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/metrics-and-insights/page.tsx` around lines 171 - 186, The MTTR fallback strings are inconsistent with getAbbreviatedDuration output; replace the literal "0 hours" used in the CardDoraMetric props for amount and previousAmount with a call to getAbbreviatedDuration(0) (or the function's zero-value equivalent) so metrics.meanTimeToRecover?.currentAmount ? getAbbreviatedDuration(...) : getAbbreviatedDuration(0) and likewise for previousAmount, referencing CardDoraMetric and metrics.meanTimeToRecover to locate the code.
🧹 Nitpick comments (3)
apps/api/src/lib/logger.ts (1)
49-93: Field sanitization is only applied tologger.info; other levels (warn,error,debug,trace,fatal) pass the rawobjthrough.If the intent is to guard sensitive fields uniformly, the same
cleanObjloop should be extracted into a shared helper and reused across all levels. As-is, alogger.error("...", { workspace, pullRequest })call would still expose unredacted fields to both pino and Sentry.♻️ Proposed refactor — extract sanitizer helper
+const sanitize = (obj?: object): Record<string, unknown> => { + const cleanObj: Record<string, unknown> = { ...(obj || {}) }; + for (const key of Object.keys(cleanObj)) { + if ( + key in loggableFields && + typeof cleanObj[key] === "object" && + cleanObj[key] !== null + ) { + cleanObj[key] = pick( + cleanObj[key] as object, + loggableFields[key as keyof typeof loggableFields] + ); + } + } + return cleanObj; +}; + export const logger = { info: (msg: string, obj?: object) => { - const cleanObj = { ...(obj || {}) }; - for (const key of Object.keys(cleanObj)) { - if ( - loggableFields[key] && - typeof cleanObj[key] === "object" && - cleanObj[key] !== null - ) { - cleanObj[key] = pick(cleanObj[key], loggableFields[key]); - } - } + const cleanObj = sanitize(obj); pinoLogger.info(cleanObj, msg); addBreadcrumb({ message: msg, category: "log", level: "info", data: cleanObj }); }, warn: (msg: string, obj?: object) => { - pinoLogger.warn(obj || {}, msg); - addBreadcrumb({ message: msg, category: "log", level: "warning", data: obj }); + const cleanObj = sanitize(obj); + pinoLogger.warn(cleanObj, msg); + addBreadcrumb({ message: msg, category: "log", level: "warning", data: cleanObj }); }, // ... repeat for error, debug, trace, fatal🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/lib/logger.ts` around lines 49 - 93, The other logger methods (warn, error, debug, trace, fatal) pass raw obj through while only info applies field sanitization; extract the sanitization loop into a shared helper (e.g., sanitizeObj or cleanObj) and call it at the top of each method (warn, error, debug, trace, fatal) so you pass sanitizedObj to pinoLogger.* and to addBreadcrumb, and also correct the trace breadcrumb level to "trace" instead of "debug"; update references to the existing pinoLogger.warn/error/debug/trace/fatal and addBreadcrumb calls to use the sanitized object.apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts (2)
97-101:pullRequesttyped as optional but always required at every call site — consider making it required.All callers pass
pullRequest. Keeping the parameter optional weakens type safety: a future caller that omits it would silently receive a live calculation even forCLOSEDorMERGEDPRs, bypassing both state guards.♻️ Proposed fix
- pullRequest?: PullRequest; + pullRequest: PullRequest;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts` around lines 97 - 101, The parameter object in the transformer that currently types pullRequest as optional (the destructured param with from: Date | null; duration: bigint | null; pullRequest?: PullRequest; uselessAfterMerge?: boolean) should be changed so pullRequest is required (remove the optional marker) to reflect that all call sites always supply it; update the type in the function signature in pull-request-tracking.transformer.ts (and any exported types/interfaces used there) to make pullRequest: PullRequest, then run TypeScript to fix any leftover signatures — callers shouldn't need changes but this enforces the invariant and prevents future omissions.
42-45: Optional:startedCodingAtternary reads counterintuitively.The condition
firstCommitAt > pullRequest.createdAtis true when the commit is later, yet the truthy branch takescreatedAt(the earlier date). The logic is correct (effectivelymin(firstCommitAt, createdAt)), but the reversed sense makes it easy to misread in future. A helper clarifies intent:♻️ Proposed refactor
- const startedCodingAt = - tracking.firstCommitAt && tracking.firstCommitAt > pullRequest.createdAt - ? pullRequest.createdAt - : (tracking.firstCommitAt ?? pullRequest.createdAt); + const startedCodingAt = tracking.firstCommitAt + ? new Date(Math.min(tracking.firstCommitAt.getTime(), pullRequest.createdAt.getTime())) + : pullRequest.createdAt;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts` around lines 42 - 45, The ternary that computes startedCodingAt is written with a reversed sense (tracking.firstCommitAt && tracking.firstCommitAt > pullRequest.createdAt ? pullRequest.createdAt : (tracking.firstCommitAt ?? pullRequest.createdAt)) which is correct but hard to read; change it to explicitly take the earlier of tracking.firstCommitAt and pullRequest.createdAt (e.g., add a small helper like getEarlierDate(a,b) or use a clear min-date expression) and then set startedCodingAt using that helper/clear expression so the intent is obvious; update references to startedCodingAt, tracking.firstCommitAt and pullRequest.createdAt in pull-request-tracking.transformer.ts 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 `@apps/api/src/app/github/services/github-pull-request.service.ts`:
- Line 212: The Octokit field response.data.merge_commit_sha is string | null,
which can leak null into the local mergeCommitSha (typed string | undefined);
change the code to normalize null to undefined (e.g. assign
response.data.merge_commit_sha ?? undefined or use a conditional ternary) before
storing into mergeCommitSha or returning it so the value becomes string |
undefined; update the return expression that currently returns
response.data.merge_commit_sha to return response.data.merge_commit_sha ??
undefined instead (or explicitly map null -> undefined wherever mergeCommitSha
is assigned).
In
`@apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts`:
- Around line 56-64: The live calculation for timeToFirstApproval currently uses
tracking.firstReviewAt as the primary "from" baseline which undercounts waiting
time; change the baseline ordering in the calculateTimeForEvent call so it
prefers the PR entering the review queue (tracking.firstReadyAt) first, then
falls back to pullRequest.createdAt (and only use tracking.firstReviewAt as a
last-resort fallback if needed), keeping the existing duration
(tracking.timeToFirstApproval) and options (e.g., uselessAfterMerge: true)
intact; update the from expression in the timeToFirstApproval invocation (the
calculateTimeForEvent call) accordingly.
---
Outside diff comments:
In `@apps/web/src/app/metrics-and-insights/page.tsx`:
- Around line 171-186: The MTTR fallback strings are inconsistent with
getAbbreviatedDuration output; replace the literal "0 hours" used in the
CardDoraMetric props for amount and previousAmount with a call to
getAbbreviatedDuration(0) (or the function's zero-value equivalent) so
metrics.meanTimeToRecover?.currentAmount ? getAbbreviatedDuration(...) :
getAbbreviatedDuration(0) and likewise for previousAmount, referencing
CardDoraMetric and metrics.meanTimeToRecover to locate the code.
---
Duplicate comments:
In `@apps/api/src/app/deployment/services/deployment-pr-linking.service.ts`:
- Around line 161-182: Replace the Promise.all on filteredPullRequests with
Promise.allSettled and handle each settled result individually: call
updatePullRequestDeploymentTracking (same args: pullRequest pr as PullRequest &
{ tracking: PullRequestTracking }, deployment, workspaceId) for every PR, then
iterate the allSettled results and on any rejected result or returned
DataIntegrityException-related failure call captureException with context
(include pr and deployment/workspaceId) and do not let one failure short-circuit
others; ensure the function still awaits completion of all per-PR operations
before returning.
In
`@apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts`:
- Around line 77-82: Remove the fallback to pullRequest.createdAt in the
timeToDeploy call so that calculateTimeForEvent receives from:
pullRequest.mergedAt only; specifically update the timeToDeploy construction
(where calculateTimeForEvent is called) to pass from: pullRequest.mergedAt (not
pullRequest.mergedAt || pullRequest.createdAt) so the existing !from guard
inside calculateTimeForEvent will return null for unmerged/open PRs and avoid
computing the PR's age, keeping uselessAfterMerge: false as-is.
In `@apps/api/src/lib/logger.ts`:
- Around line 29-38: cleanObj and loggableFields cause TS index errors because
cleanObj is inferred as {} and loggableFields isn't indexed by string; update
types and add a runtime/type guard: declare cleanObj as Record<string, unknown>
(or cast obj to Record<string, unknown>) so cleanObj[key] is valid, and change
loggableFields' type to Record<string, string[]> (or assert key is keyof typeof
loggableFields) and add a guard like "if (key in loggableFields)" before
indexing; keep the existing typeof/nonnull check and continue to call
pick(cleanObj[key], loggableFields[key]) once the guards and types are fixed.
- Line 46: The Sentry breadcrumb currently gets the original unsanitized obj
(data: obj) which leaks fields excluded by loggableFields; replace that with the
sanitized object produced earlier (e.g., use the sanitized/filtered variable
returned by your sanitize/filter function) so the breadcrumb's data uses only
allowed fields, updating the code paths in the logger function where obj is
passed to Sentry (look for the breadcrumb or Sentry.addBreadcrumb call and the
obj / sanitized variable names) to pass the sanitized payload instead of obj.
---
Nitpick comments:
In
`@apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts`:
- Around line 97-101: The parameter object in the transformer that currently
types pullRequest as optional (the destructured param with from: Date | null;
duration: bigint | null; pullRequest?: PullRequest; uselessAfterMerge?: boolean)
should be changed so pullRequest is required (remove the optional marker) to
reflect that all call sites always supply it; update the type in the function
signature in pull-request-tracking.transformer.ts (and any exported
types/interfaces used there) to make pullRequest: PullRequest, then run
TypeScript to fix any leftover signatures — callers shouldn't need changes but
this enforces the invariant and prevents future omissions.
- Around line 42-45: The ternary that computes startedCodingAt is written with a
reversed sense (tracking.firstCommitAt && tracking.firstCommitAt >
pullRequest.createdAt ? pullRequest.createdAt : (tracking.firstCommitAt ??
pullRequest.createdAt)) which is correct but hard to read; change it to
explicitly take the earlier of tracking.firstCommitAt and pullRequest.createdAt
(e.g., add a small helper like getEarlierDate(a,b) or use a clear min-date
expression) and then set startedCodingAt using that helper/clear expression so
the intent is obvious; update references to startedCodingAt,
tracking.firstCommitAt and pullRequest.createdAt in
pull-request-tracking.transformer.ts accordingly.
In `@apps/api/src/lib/logger.ts`:
- Around line 49-93: The other logger methods (warn, error, debug, trace, fatal)
pass raw obj through while only info applies field sanitization; extract the
sanitization loop into a shared helper (e.g., sanitizeObj or cleanObj) and call
it at the top of each method (warn, error, debug, trace, fatal) so you pass
sanitizedObj to pinoLogger.* and to addBreadcrumb, and also correct the trace
breadcrumb level to "trace" instead of "debug"; update references to the
existing pinoLogger.warn/error/debug/trace/fatal and addBreadcrumb calls to use
the sanitized object.
Contributor
Additional Comments (1)
The JSDoc says "Time between the first approval (or PR creation date)" but the implementation now falls through |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Greptile Summary
This PR adds deployment tracking for pull requests — recording when a PR was first deployed and computing the time-to-deploy metric (time between merge and deployment). It also exposes two previously-computed-but-unexposed fields (
timeToCode,firstDeployedAt) via the GraphQL API and updates all frontend queries and timeline UI to display them.Key changes:
updatePullRequestDeploymentTrackingfunction indeployment-pr-linking.service.tsthat updatesfirstDeployedAtandtimeToDeployon PR tracking records after a deployment, with idempotency (skips if a prior deployment is earlier).fetchPullRequestMergeCommitShaadded as a fallback REST API call when the webhook doesn't provide the merge commit SHA (e.g., during batch resyncs).calculateTimeForEventrefactored with a newuselessAfterMergeflag to correctly suppress live-timer calculations for metrics that don't apply after merge (review, approval, merge times) while enabling them for deploy and cycle time.higherIsBetter, fixed operator precedence bug in deployment frequencypreviousAmount, and switched MTTR to abbreviated duration format.workspaceandpullRequestobjects before logging.isLiveguard on resync cooldown to allow unlimited resyncs in development.Confidence Score: 4/5
apps/api/src/lib/logger.ts(issues flagged in prior threads),apps/api/src/app/pull-requests/resolvers/transformers/pull-request-tracking.transformer.ts(timeToDeploy live-timer for open PRs flagged in prior thread)Important Files Changed
updatePullRequestDeploymentTrackingfunction and renamesfindPullRequestsByCommitHashestofindMergedPullRequestsByCommitHasheswithmergedAtfilter. Includes proper error handling and early-return when previous deployment is earlier.updatePullRequestDeploymentTrackingafter linking PRs to deployment. Clean integration of new deploy tracking.trackingrelation. Contains a type assertion that is safe due to prior null check.getTimeToDeployfunction and updatesgetTimeToMergeto acceptfirstReadyAtparameter in its fallback chain. JSDoc forgetTimeToMergeis now slightly outdated.fetchPullRequestMergeCommitShawith try/catch error handling. Falls back to REST API when webhook data is unavailable. Also addscaptureExceptionto existinggetFirstCommiterror handler and passesfirstReadyAttogetTimeToMerge.transformPullRequestTrackingandcalculateTimeForEventto support deploy tracking and newuselessAfterMergeflag. AddstimeToCode,timeToDeploy, andfirstDeployedAtfields. Changes live-timer behavior for merged PRs.workspaceandpullRequestobjects to reduce log noise. Creates a shallow copy to avoid mutating caller's object. Issues with incorrect field names and unsafe property access were flagged in prior review threads.previousAmountdisplay, capitalizes metric names, and switches MTTR togetAbbreviatedDuration. Previous operator precedence bug was fixed.getColorto use string-based level names. UsesisNumberguards for duration display.getDeployBadgefunction for deploy status badges with time-based thresholds. Only shows badge for merged and deployed PRs.Sequence Diagram
sequenceDiagram participant Webhook as GitHub Webhook participant SyncWorker as syncPullRequestWorker participant PRService as github-pull-request.service participant DeployWorker as deploymentTriggeredByPullRequestMergeWorker participant DB as Database participant PRLinking as deployment-pr-linking.service participant CreateMerge as deployment-create-from-merge.service Webhook->>SyncWorker: PR merged event SyncWorker->>PRService: syncPullRequest(webhookMergeCommitSha) PRService->>PRService: fetchPullRequestMergeCommitSha (fallback if no webhook SHA) PRService->>DB: upsertPullRequest (with mergeCommitSha) PRService->>DB: upsertPullRequestTracking (timeToMerge with firstReadyAt) SyncWorker->>DeployWorker: enqueue(pullRequestId) DeployWorker->>DB: findPullRequestById (include tracking) DeployWorker->>CreateMerge: createDeploymentFromPullRequestMerge CreateMerge->>DB: upsertDeployment CreateMerge->>PRLinking: linkPullRequestsToDeployment CreateMerge->>PRLinking: updatePullRequestDeploymentTracking PRLinking->>DB: Update firstDeployedAt & timeToDeploy Note over PRLinking: Auto-linking path (external deployments) PRLinking->>DB: findMergedPullRequestsByCommitHashes PRLinking->>PRLinking: filterPullRequestsBySubdirectory PRLinking->>DB: linkPullRequestsToDeployment PRLinking->>DB: Update firstDeployedAt & timeToDeploy (per PR)Last reviewed commit: 494f742