feat(seer): Link failing checks to their runs on autofix overview - #122489
Conversation
| • | ||
| </Text> | ||
| <Text size="sm" align="left"> | ||
| {name} | ||
| {check.url ? ( | ||
| <ExternalLink href={check.url}>{check.name}</ExternalLink> | ||
| ) : ( | ||
| check.name | ||
| )} | ||
| </Text> | ||
| </Flex> | ||
| ))} |
There was a problem hiding this comment.
Bug: The Tooltip for failing checks is missing isHoverable={true}, causing it to close immediately when the user tries to move their cursor to click the links inside.
Severity: MEDIUM
Suggested Fix
Add the isHoverable={true} prop to the Tooltip component in issueCard.tsx. This will ensure the tooltip remains open long enough for the user to move their cursor from the trigger tag to the tooltip content and interact with the links inside.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: static/app/views/seerWorkflows/overview/issueCard.tsx#L258-L274
Potential issue: The `Tooltip` component used to display failing checks is not
explicitly configured with `isHoverable={true}`. The underlying `useHoverOverlay` hook
requires this prop to apply a delay before closing the tooltip when the mouse leaves the
trigger element. Without this delay, when a user hovers over the "Checks Failing" tag
and then moves their mouse towards the tooltip's content to click a link, the tooltip
will close instantly. This makes the links to the failing checks inaccessible to the
user, rendering a key part of the new functionality unusable.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Not applicable here — Tooltip from @sentry/scraps/tooltip already defaults isHoverable = true (tooltip.tsx:49) and threads it into useHoverOverlay (tooltip.tsx:64). So this tooltip is hoverable without an explicit prop, and the links are reachable. The RTL test hovers the tag and asserts the run link is present/clickable. No change needed.
c34044d to
f7553ec
Compare
| : null; | ||
| const failedChecks = | ||
| reviewPullRequest.checksStatus === 'failure' | ||
| ? (reviewPullRequest.failedChecks ?? []) | ||
| ? (reviewPullRequest.failedCheckDetails ?? []) | ||
| : []; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Bug: The frontend now only reads failedCheckDetails, but the backend still sends failedChecks. If deployed before the backend, failing check details will be silently lost.
Severity: MEDIUM
Suggested Fix
To ensure backward compatibility and prevent data loss during the deployment transition, the frontend should be updated to handle both the new and old fields. Check for failedCheckDetails first, but if it's not present, fall back to using the data from the failedChecks field. This will ensure the feature works correctly regardless of the deployment order of the frontend and backend changes.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: static/app/views/seerWorkflows/overview/issueCard.tsx#L211-L217
Potential issue: The frontend has been updated to exclusively read the
`failedCheckDetails` field from the API response, removing support for the old
`failedChecks` field. However, the backend has not yet been updated and still sends
`failedChecks`. If this frontend change is deployed before the corresponding backend
update, the `failedCheckDetails` field will be `undefined` in the API response. The code
will then fall back to an empty array `[]` due to the nullish coalescing operator. This
causes the UI to correctly indicate that checks are failing, but the list of specific
failing checks will be empty, resulting in a silent loss of information for the user.
Also affects:
static/app/views/seerWorkflows/overview/types.ts:159~165
There was a problem hiding this comment.
By design, and safe under our deploy order. The name-only failedChecks fallback was intentionally removed so the frontend reads only failedCheckDetails. The silent-loss window described here only exists if this ships before the backend — which the merge order prevents: this PR is gated on #122486 (which adds failedCheckDetails) landing and deploying first (it is a draft with a blocked-on banner). Once #122486 is live, every response includes failedCheckDetails, so there is no frontend-new / backend-old window. No change.
The autofix overview's 'N Checks Failing' tooltip lists each failing PR check by name as dead text. Render each check name as a link to its CI run when a run URL is available (from the failedCheckDetails API field), so users can jump straight to the failing run. Checks whose provider gives no run URL stay plain text.
f7553ec to
21f2ebe
Compare
…PI (#122490) > [!WARNING] > **Do not merge until the frontend (#122489) is deployed.** > This removes the `failedChecks` field that the currently-deployed frontend still reads. It is only safe once #122489 (which reads `failedCheckDetails`) has shipped everywhere. ## Summary Follow-up cleanup for the failing-check run-links work (#122486, now merged). The overview API temporarily carried **both** `failedChecks` (names only) and `failedCheckDetails` (name + run link); `failedChecks` existed solely to keep the pre-links frontend working across the non-atomic deploy. Now that the frontend reads `failedCheckDetails`, this drops the redundant field and its `PullRequestPayload` TypedDict entry, leaving one representation of failing checks. ## Deploy order 1. ~~#122486 (backend, adds `failedCheckDetails`)~~ — **merged** 2. **#122489** (frontend, reads `failedCheckDetails`) — merge + deploy 3. **this PR** — merge + deploy ## Tests Updates the overview endpoint tests to assert `failedChecks` is no longer in the payload and only `failedCheckDetails` is present.
Warning
Blocked on #122486 (backend) — do not merge until that PR is deployed.
This reads the
failedCheckDetailsAPI field added by #122486. Per the split-deploy rule (AGENTS.md), the backend must land and deploy first. Opened as a draft for early review.Summary
The Autofix Overview's "N Checks Failing" tooltip lists each failing PR check by name as dead text. This makes each check name a link to its CI run, so a user can jump straight from a failing check to the run that failed.
What changed
types.ts: addsFailedCheckDetail { name, url }and an optionalfailedCheckDetailsfield onOverviewPullRequest.issueCard.tsx: the failing-checks tooltip renders each check name as anExternalLinkto its run URL when one is present; checks without a URL stay plain text. The tooltip is already hoverable by default, so the links are clickable.Deploy dependency
Reads only
failedCheckDetails(no name-only fallback), so it hard-depends on #122486 being deployed first — which is the required merge order anyway. A failing check whose provider gives no run URL renders as plain text.Note: the backend validates that each
urlis http(s)-only (see #122486), preventing ajavascript:/data:scheme from reaching the anchorhref.Follow-up cleanup
The redundant
failedChecksnames field is removed from the backend in the stacked cleanup PR #122490 (merges after this deploys).Tests
Extends
index.spec.tsx: a failing check with a run URL renders an anchor to that URL; a check withurl: nullstays plain text; a failing PR with no check details shows the plain "Checks Failing" label.