Skip to content

ref(ui): Drop leftover unused optional args in issueDiff and pagination #123247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JonasBa wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ref(ui): Drop leftover unused optional args in issueDiff and pagination #123247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion static/app/components/charts/releaseSeries.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
environment: readonly string[];
project: readonly number[];
start: DateString;
cursor?: string;
query?: string;
statsPeriod?: string | null;
};
Expand Down Expand Up @@ -194,7 +193,7 @@
if (pageLinks) {
const paginationObject = parseLinkHeader(pageLinks);
hasMore = paginationObject?.next?.results ?? false;
conditions.cursor = paginationObject.next!.cursor;

Check failure on line 196 in static/app/components/charts/releaseSeries.tsx

View workflow job for this annotation

GitHub Actions / typescript 

Property 'cursor' does not exist on type 'ReleaseConditions'.
} else {
hasMore = false;
}
Expand Down
10 changes: 4 additions & 6 deletions static/app/components/core/form/field/meta.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,12 @@ function Label(props: {
);
}

function FieldStatus({disabled, error}: {disabled?: boolean | string; error?: string}) {
function FieldStatus({disabled}: {disabled?: boolean | string}) {
const field = useFieldContext();

const errorMessage =
error ??
(field.state.meta.isValid
? undefined
: field.state.meta.errors.map((e: Error | undefined) => e?.message).join(','));
const errorMessage = field.state.meta.isValid
? undefined
: field.state.meta.errors.map((e: Error | undefined) => e?.message).join(',');

if (errorMessage) {
return (
Expand Down
11 changes: 0 additions & 11 deletions static/app/components/core/pagination/pagination.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,17 +95,6 @@ describe('Pagination', () => {
expect(router.location.query).toEqual({foo: 'bar', cursor: '0:25:0'});
});

it('uses the to prop to override the default pathname', async () => {
const {router} = render(<Pagination pageLinks={pageLinks} to="/other/" />, {
initialRouterConfig: {location: {pathname: '/items/'}},
});

await userEvent.click(screen.getByRole('button', {name: 'Next'}));

expect(router.location.pathname).toBe('/other/');
expect(router.location.query.cursor).toBe('0:25:0');
});

it('calls custom onCursor with (cursor, path, query, delta)', async () => {
const onCursor = jest.fn();
render(<Pagination pageLinks={pageLinks} onCursor={onCursor} />, {
Expand Down
4 changes: 1 addition & 3 deletions static/app/components/core/pagination/pagination.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,9 @@ type Props = {
pageLinks?: string | null;
paginationAnalyticsEvent?: (direction: string) => void;
size?: ButtonProps['size'];
to?: string;
};

export function Pagination({
to,
className,
onCursor,
paginationAnalyticsEvent,
Expand All @@ -61,7 +59,7 @@ export function Pagination({
return null;
}

const path = to ?? location.pathname;
const path = location.pathname;
const query = location.query;
const links = parseLinkHeader(pageLinks);
const previousDisabled = disabled || links.previous?.results === false;
Expand Down
1 change: 0 additions & 1 deletion static/app/components/issueDiff/index.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ describe('IssueDiff', () => {
<IssueDiff
baseIssueId="base"
targetIssueId="target"
shouldBeGrouped="Yes"
hasSimilarityEmbeddingsProjectFeature
/>
);
Expand Down
41 changes: 12 additions & 29 deletions static/app/components/issueDiff/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,7 @@ const SKELETON_ROW_COUNT = 8;
interface IssueDiffProps {
baseIssueId: string;
targetIssueId: string;
baseEventId?: string;
hasSimilarityEmbeddingsProjectFeature?: boolean;
shouldBeGrouped?: string;
targetEventId?: string;
}

function getCombinedStacktrace({
Expand Down Expand Up @@ -57,10 +54,7 @@ function getCombinedStacktrace({
export function IssueDiff({
baseIssueId,
targetIssueId,
baseEventId = 'latest',
targetEventId = 'latest',
hasSimilarityEmbeddingsProjectFeature,
shouldBeGrouped,
}: IssueDiffProps) {
const organization = useOrganization();
const location = useLocation();
Expand All @@ -77,39 +71,31 @@ export function IssueDiff({
apiOptions.as<{eventID: string}>()(
'/organizations/$organizationIdOrSlug/issues/$issueId/events/$eventId/',
{
path:
baseEventId === 'latest'
? {
organizationIdOrSlug: organization.slug,
issueId: baseIssueId,
eventId: 'latest',
}
: skipToken,
path: {
organizationIdOrSlug: organization.slug,
issueId: baseIssueId,
eventId: 'latest',
},
staleTime: 60_000,
}
),
apiOptions.as<{eventID: string}>()(
'/organizations/$organizationIdOrSlug/issues/$issueId/events/$eventId/',
{
path:
targetEventId === 'latest'
? {
organizationIdOrSlug: organization.slug,
issueId: targetIssueId,
eventId: 'latest',
}
: skipToken,
path: {
organizationIdOrSlug: organization.slug,
issueId: targetIssueId,
eventId: 'latest',
},
staleTime: 60_000,
}
),
],
});

// 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;

@cursor cursor Bot Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f78e3c9. Configure here.


// Fetch actual event data once IDs are resolved
const {
Comment on lines 91 to 101

@sentry sentry Bot Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down Expand Up @@ -187,13 +173,11 @@ export function IssueDiff({
project_id: baseEventData?.projectID,
group_id: baseEventData?.groupID,
parent_group_id: targetEventData?.groupID,
shouldBeGrouped,
});
}, [
baseEventData,
hasSimilarityEmbeddingsFeature,
organization,
shouldBeGrouped,
targetEventData,
]);

Expand All @@ -215,7 +199,6 @@ export function IssueDiff({
LazyComponent={SplitDiffLazy}
base={combinedBase}
target={combinedTarget}
type="lines"
loadingFallback={<IssueDiffLoadingSkeletonRows />}
/>
</Stack>
Expand Down
24 changes: 3 additions & 21 deletions static/app/components/splitDiff.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import {useMemo} from 'react';
import styled from '@emotion/styled';
import type {Change} from 'diff';
import {diffChars, diffLines, diffWords} from 'diff';
import {diffLines, diffWords} from 'diff';

import {Container} from '@sentry/scraps/layout';

import {unreachable} from 'sentry/utils/unreachable';

// @TODO(jonasbadalic): This used to be defined on the theme, but is component specific and lacks dark mode.
export const DIFF_COLORS = {
removedRow: 'hsl(358deg 89% 65% / 15%)',
Expand All @@ -19,7 +17,6 @@
base: string;
target: string;
className?: string;
type?: 'lines' | 'words' | 'chars';
};

// this function splits the lines from diffLines into words that are diffed
Expand Down Expand Up @@ -48,25 +45,10 @@
return diffWords(leftText, rightText);
}

function SplitDiff({className, type = 'lines', base, target}: Props) {
function SplitDiff({className, base, target}: Props) {
// split one change that includes multiple lines into one change per line (for formatting)
const groupedChanges = useMemo((): Change[][] => {
let diffResults: Change[] | undefined;
switch (type) {
case 'lines':
diffResults = diffLines(base, target, {newlineIsToken: true});
break;
case 'words':
diffResults = diffWords(base, target);
break;
case 'chars':
diffResults = diffChars(base, target);
break;
default:
unreachable(type);
break;
}
const results = diffResults ?? [];
const results = diffLines(base, target, {newlineIsToken: true});

let currentLine: Change[] = [];
const processedLines: Change[][] = [];
Expand All @@ -92,7 +74,7 @@
processedLines.push(currentLine);
}
return processedLines;
}, [base, target, type]);

Check failure on line 77 in static/app/components/splitDiff.tsx

View workflow job for this annotation

GitHub Actions / typescript 

Cannot find name 'type'.

Check failure on line 77 in static/app/components/splitDiff.tsx

View workflow job for this annotation

GitHub Actions / eslint 

React Hook useMemo has an unnecessary dependency: 'type'. Either exclude it or remove the dependency array. Outer scope values like 'type' aren't valid dependencies because mutating them doesn't re-render the component

const displayRows = useMemo(
() =>
Expand Down
Loading