feat(autofix): PR iteration frontend changes#118049
Conversation
the main change is we update the code changes card to display the feedback PR iteration feedback that was passed for each PR iteration we also update metrics, the next step logic, and the "autofix section" construction to support PR iteration blocks feature flag fixes feat(autofix): add reset to code changes card for PR iteration for PR iteration we want to enable the reset button in the top right of the code changes card to open up an equivalent form as the PullRequestNextStep component at the top of the code changes card to provide feedback and trigger PR iteration it's just another place to start a PR iteration in the UI so that uses don't get confused
github-actions
Bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| return section; | ||
| } |
There was a problem hiding this comment.
Bug: A section's status is checked before merged file patches are added to its artifacts, which can cause code changes to not be displayed.
Severity: MEDIUM
Suggested Fix
Modify the status determination logic to account for the presence of merged_file_patches. This can be done by checking for merged patches before setting the section's status, ensuring that sections with patches are correctly marked as 'completed'.
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/components/events/autofix/useExplorerAutofix.tsx#L287-L291
Potential issue: In the `buildSection` function, a section's `status` is determined
before `merged_file_patches` are added to its `artifacts` array. If a `code_changes`
section only contains merged patches and does not meet other completion criteria (e.g.,
the run is still processing), its status will remain 'processing'. Consequently,
`getAutofixArtifactFromSection` will not retrieve the patches because it checks for
`status === 'completed'`, preventing the code changes from being displayed to the user.
Did we get this right? 👍 / 👎 to inform future reviews.
📊 Type Coverage Diff✅ no issues found |
| artifacts.length > 0 | ||
| ) { | ||
| section.status = 'completed'; | ||
| } |
There was a problem hiding this comment.
Section completes during PR iteration
Medium Severity
While the run is still processing, buildSection can mark the folded code_changes section completed when earlier blocks already have inline artifacts or the last block looks like a finished assistant message. The code changes card then skips the processing/iterating UI during an active PR iteration.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 95a5368. Configure here.
There was a problem hiding this comment.
this is wrong, but it's a code smell since this logic is spread across Sentry FE, BE and Seer
we should clean this up later
basically we're relying on the BE not setting the artifact for code changes blocks and we're setting the artifact AFTER this check in the FE code
| if (bucket) { | ||
| bucket.blocks.push(block); | ||
| } else { | ||
| buckets.set(currentStep, {blocks: [block], index: i}); |
There was a problem hiding this comment.
Repeated steps merge incorrectly
Medium Severity
getOrderedAutofixSections buckets blocks by step name only, so if the same step appears again after later steps (e.g. re-running root cause or solution), those blocks are merged into one section. section.index stays the first occurrence, so reset/re-run calls can send the wrong insert_index, and cards can mix stale and new blocks.
Reviewed by Cursor Bugbot for commit b0c4d7c. Configure here.
There was a problem hiding this comment.
this can't be true. there won't be multiple root cause or solution blocks in an autofix session.
when a step is re-ran, the blocks that come after the resetted step will be wiped. for example, if we had [root_cause, solution] block, and we re-run root_cause, it becomes [root_cause], and doesn't append the new root_cause to the end of the list of steps
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 119b439. Configure here.
| icon={<IconArrow size="md" direction="right" />} | ||
| busy={isSubmitting} | ||
| disabled={isPolling || !feedback.trim()} | ||
| onClick={handleSubmit} |
There was a problem hiding this comment.
Duplicate PR iteration submits
Medium Severity
The Submit control sets busy from isSubmitting but stays enabled whenever feedback is non-empty and isPolling is false. A second click or Enter can invoke handleSubmit again before polling starts, issuing duplicate pr_iteration startStep requests.
Reviewed by Cursor Bugbot for commit 119b439. Configure here.
There was a problem hiding this comment.
In useButtonFunctionality.tsx:27-33, the rendered button's onClick is the wrapper handleClick, which short-circuits before ever calling the user's onClick:
// Don't allow clicks when disabled or busy
if (props.disabled || props.busy) {
e.preventDefault();
e.stopPropagation();
return;
}
| {(autofix.runState?.status === 'completed' || | ||
| isLastStepPrIteration(autofix.runState)) && ( |
There was a problem hiding this comment.
Bug: The PR iteration feedback form is incorrectly shown for runs with status === 'error' because isLastStepPrIteration doesn't check the run status before rendering the component.
Severity: MEDIUM
Suggested Fix
Modify the rendering condition in content.tsx to also check for a non-error status. For example, change it to autofix.runState?.status === 'completed' || (autofix.runState?.status !== 'error' && isLastStepPrIteration(autofix.runState)). Alternatively, update isLastStepPrIteration to include a status check.
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/components/events/autofix/v3/content.tsx#L62-L63
Potential issue: The rendering condition for `SeerDrawerNextStep` is
`autofix.runState?.status === 'completed' || isLastStepPrIteration(autofix.runState)`.
The `isLastStepPrIteration` function only checks if the last step was a `pr_iteration`,
without considering the overall run status. Consequently, if a run fails and its status
becomes `'error'`, but the last attempted step was a PR iteration, the condition
evaluates to true, causing the PR iteration feedback form to be displayed. This is
incorrect UX, as users should not be prompted for feedback on a failed process. The
`isPolling` guard does not prevent this, as an errored state is not considered a polling
state.
Also affects:
static/app/components/events/autofix/useExplorerAutofix.tsx:52~55
There was a problem hiding this comment.
it makes sense to let the PR iteration feedback form to be displayed after a failed PR iteration step imo
2 participants
there's multiple changes happening in this PR:
Fixes https://linear.app/getsentry/issue/CW-1397