ref(ui): Drop leftover unused optional args in issueDiff and pagination - #123247
ref(ui): Drop leftover unused optional args in issueDiff and pagination#123247JonasBa wants to merge 1 commit into
Conversation
Follow-up leftovers after the unused-args split. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
the
Scope: Frontend
|
cc @TkDodo @nsdeschenes — leftover unused-args after the first split. Could not request review (fork token). |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f78e3c9. Configure here.
| const resolvedTargetEventId = | ||
| targetEventId === 'latest' ? targetLatestQuery.data?.eventID : targetEventId; | ||
| const resolvedBaseEventId = baseLatestQuery.data?.eventID; | ||
| const resolvedTargetEventId = targetLatestQuery.data?.eventID; |
There was a problem hiding this comment.
Compare diffs latest events only
High Severity
IssueDiff no longer accepts baseEventId and targetEventId and always loads latest for both issues. Merged-issue Compare passes two fingerprint event IDs on the same issue through openDiffModal, so the modal now diffs that issue's latest event against itself.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f78e3c9. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| } | ||
| ), | ||
| ], | ||
| }); | ||
|
|
||
| // Derive resolved IDs reactively from the query results | ||
| const resolvedBaseEventId = | ||
| baseEventId === 'latest' ? baseLatestQuery.data?.eventID : baseEventId; | ||
| const resolvedTargetEventId = | ||
| targetEventId === 'latest' ? targetLatestQuery.data?.eventID : targetEventId; | ||
| const resolvedBaseEventId = baseLatestQuery.data?.eventID; | ||
| const resolvedTargetEventId = targetLatestQuery.data?.eventID; | ||
|
|
||
| // Fetch actual event data once IDs are resolved | ||
| const { |
There was a problem hiding this comment.
Bug: Removing baseEventId and targetEventId from IssueDiff breaks the merged fingerprint comparison, as it will now always diff the 'latest' event against itself.
Severity: HIGH
Suggested Fix
Reinstate the baseEventId and targetEventId props in the IssueDiff component. The component's logic should prioritize using these props to fetch specific events when they are provided, and only fall back to fetching the 'latest' event if they are absent. This will restore the functionality for features like the merged fingerprint comparison.
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/issueDiff/index.tsx#L71-L101
Potential issue: The removal of `baseEventId` and `targetEventId` props from the
`IssueDiff` component introduces a functional regression. The merged fingerprint
comparison feature, used in `mergedToolbar.tsx`, relies on passing these specific event
IDs to compare two different events within the same merged issue group. With this
change, `IssueDiff` will always fetch the 'latest' event for both the base and target
sides of the diff. Since the issue ID is the same for both in this scenario, the
component will end up fetching the same event twice and diffing it against itself,
rendering the comparison feature useless.
Did we get this right? 👍 / 👎 to inform future reviews.
1 participant
Leftover unused-args findings after #123200–#123237. These files were not in the first split (~92 LOC).
Summary
static/app/components.IssueDiffalways loadslatestevents;SplitDiffalways diffs lines.Paginationalways useslocation.pathname.Files
static/app/components/charts/releaseSeries.tsxstatic/app/components/core/form/field/meta.tsxstatic/app/components/core/pagination/pagination.spec.tsxstatic/app/components/core/pagination/pagination.tsxstatic/app/components/issueDiff/index.spec.tsxstatic/app/components/issueDiff/index.tsxstatic/app/components/splitDiff.tsxTest plan
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.