Skip to content

fix(merged): Always show a link for latests event of a merged group #81947

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

Merged
merged 5 commits into from
Dec 11, 2024
Merged

fix(merged): Always show a link for latests event of a merged group #81947

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cbc7865
♻️ Convert MergedItem to FC
leeandher Dec 10, 2024
cab3308
✨ Always render link
leeandher Dec 10, 2024
10b5bac
✅ Simple test
leeandher Dec 10, 2024
8d89c85
🔥 teardown properly
leeandher Dec 10, 2024
e82d978
✨ Add title
leeandher Dec 11, 2024
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
3 changes: 3 additions & 0 deletions static/app/views/issueDetails/groupMerged/index.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,5 +78,8 @@ describe('Issues -> Merged View', function () {
expect(title.parentElement).toHaveTextContent(
'Fingerprints included in this issue (2)'
);

const links = await screen.findAllByRole('button', {name: 'View latest event'});
expect(links).toHaveLength(mockData.merged.length);
});
});
202 changes: 104 additions & 98 deletions static/app/views/issueDetails/groupMerged/mergedItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,44 +1,37 @@
import {Component} from 'react';
import {useEffect, useState} from 'react';
import styled from '@emotion/styled';

import {Button} from 'sentry/components/button';
import {Button, LinkButton} from 'sentry/components/button';
import Checkbox from 'sentry/components/checkbox';
import {Flex} from 'sentry/components/container/flex';
import EventOrGroupHeader from 'sentry/components/eventOrGroupHeader';
import {Tooltip} from 'sentry/components/tooltip';
import {IconChevron} from 'sentry/icons';
import {IconChevron, IconLink} from 'sentry/icons';
import {t} from 'sentry/locale';
import type {Fingerprint} from 'sentry/stores/groupingStore';
import GroupingStore from 'sentry/stores/groupingStore';
import {space} from 'sentry/styles/space';
import type {Organization} from 'sentry/types/organization';
import {useLocation} from 'sentry/utils/useLocation';
import useOrganization from 'sentry/utils/useOrganization';
import {createIssueLink} from 'sentry/views/issueList/utils';

type Props = {
interface Props {
fingerprint: Fingerprint;
organization: Organization;
totalFingerprint: number;
};

type State = {
busy: boolean;
checked: boolean;
collapsed: boolean;
};

class MergedItem extends Component<Props, State> {
state: State = {
collapsed: false,
checked: false,
busy: false,
};
}

listener = GroupingStore.listen(data => this.onGroupChange(data), undefined);
export function MergedItem({fingerprint, totalFingerprint}: Props) {
const organization = useOrganization();
const location = useLocation();
const [busy, setBusy] = useState(false);
const [collapsed, setCollapsed] = useState(false);
const [checked, setChecked] = useState(false);

onGroupChange = ({unmergeState}) => {
function onGroupChange({unmergeState}) {
if (!unmergeState) {
return;
}

const {fingerprint} = this.props;
const stateForId = unmergeState.has(fingerprint.id)
? unmergeState.get(fingerprint.id)
: undefined;
Expand All @@ -48,42 +41,37 @@ class MergedItem extends Component<Props, State> {
}

Object.keys(stateForId).forEach(key => {
if (stateForId[key] === this.state[key]) {
return;
if (key === 'collapsed') {
setCollapsed(Boolean(stateForId[key]));
} else if (key === 'checked') {
setChecked(Boolean(stateForId[key]));
} else if (key === 'busy') {
setBusy(Boolean(stateForId[key]));
}

this.setState(prevState => ({...prevState, [key]: stateForId[key]}));
});
};
}

handleToggleEvents = () => {
const {fingerprint} = this.props;
function handleToggleEvents() {
GroupingStore.onToggleCollapseFingerprint(fingerprint.id);
};

// Disable default behavior of toggling checkbox
handleLabelClick(event: React.MouseEvent) {
event.preventDefault();
}

handleToggle = () => {
const {fingerprint} = this.props;
function handleToggle() {
const {latestEvent} = fingerprint;

if (this.state.busy) {
if (busy) {
return;
}

// clicking anywhere in the row will toggle the checkbox
GroupingStore.onToggleUnmerge([fingerprint.id, latestEvent.id]);
};
}

handleCheckClick() {
function handleCheckClick() {
// noop because of react warning about being a controlled input without `onChange`
// we handle change via row click
}

renderFingerprint(id: string, label?: string) {
function renderFingerprint(id: string, label?: string) {
if (!label) {
return id;
}
Expand All @@ -95,55 +83,76 @@ class MergedItem extends Component<Props, State> {
);
}

render() {
const {fingerprint, organization, totalFingerprint} = this.props;
const {latestEvent, id, label} = fingerprint;
const {collapsed, busy, checked} = this.state;
const checkboxDisabled = busy || totalFingerprint === 1;

// `latestEvent` can be null if last event w/ fingerprint is not within retention period
return (
<MergedGroup busy={busy}>
<Controls expanded={!collapsed}>
<FingerprintLabel onClick={this.handleToggle}>
<Tooltip
containerDisplayMode="flex"
disabled={!checkboxDisabled}
title={
checkboxDisabled && totalFingerprint === 1
? t('To check, the list must contain 2 or more items')
: undefined
}
>
<Checkbox
value={id}
checked={checked}
disabled={checkboxDisabled}
onChange={this.handleCheckClick}
useEffect(
() => {
GroupingStore.listen(data => onGroupChange(data), undefined);
},
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Member

@scttcper scttcper Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

you want to remove the listener on unmount by calling the returned function from listen

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch mb

[]
);

const {latestEvent, id, label} = fingerprint;
const checkboxDisabled = busy || totalFingerprint === 1;

const issueLink = latestEvent
? createIssueLink({
organization,
location,
data: latestEvent,
eventId: latestEvent.id,
referrer: 'merged-item',
})
: null;

// `latestEvent` can be null if last event w/ fingerprint is not within retention period
return (
<MergedGroup busy={busy}>
<Controls expanded={!collapsed}>
<FingerprintLabel onClick={handleToggle}>
<Tooltip
containerDisplayMode="flex"
disabled={!checkboxDisabled}
title={
checkboxDisabled && totalFingerprint === 1
? t('To check, the list must contain 2 or more items')
: undefined
}
>
<Checkbox
value={id}
checked={checked}
disabled={checkboxDisabled}
onChange={handleCheckClick}
size="xs"
/>
</Tooltip>
{renderFingerprint(id, label)}
</FingerprintLabel>

<Button
aria-label={
collapsed ? t('Show %s fingerprints', id) : t('Collapse %s fingerprints', id)
}
size="zero"
borderless
icon={<IconChevron direction={collapsed ? 'down' : 'up'} size="xs" />}
onClick={handleToggleEvents}
/>
</Controls>

{!collapsed && (
<MergedEventList>
{issueLink ? (
<Flex align="center" gap={space(0.5)}>
<LinkButton
to={issueLink}
icon={<IconLink color={'linkColor'} />}
aria-label={t('View latest event')}
borderless
size="xs"
style={{marginLeft: space(1)}}
/>
</Tooltip>

{this.renderFingerprint(id, label)}
</FingerprintLabel>

<Button
aria-label={
collapsed
? t('Show %s fingerprints', id)
: t('Collapse %s fingerprints', id)
}
size="zero"
borderless
icon={<IconChevron direction={collapsed ? 'down' : 'up'} size="xs" />}
onClick={this.handleToggleEvents}
/>
</Controls>

{!collapsed && (
<MergedEventList className="event-list">
{latestEvent && (
<EventDetails className="event-details">
<EventDetails>
<EventOrGroupHeader
data={latestEvent}
organization={organization}
Expand All @@ -152,12 +161,12 @@ class MergedItem extends Component<Props, State> {
source="merged-item"
/>
</EventDetails>
)}
</MergedEventList>
)}
</MergedGroup>
);
}
</Flex>
) : null}
</MergedEventList>
)}
</MergedGroup>
);
}

const MergedGroup = styled('div')<{busy: boolean}>`
Expand Down Expand Up @@ -202,10 +211,7 @@ const MergedEventList = styled('div')`
const EventDetails = styled('div')`
display: flex;
justify-content: space-between;

.event-list & {
padding: ${space(1)};
}
padding: ${space(1)};
`;

export default MergedItem;
1 change: 0 additions & 1 deletion static/app/views/issueDetails/groupMerged/mergedList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ function MergedList({
{fingerprintsWithLatestEvent.map(fingerprint => (
<MergedItem
key={fingerprint.id}
organization={organization}
fingerprint={fingerprint}
totalFingerprint={fingerprintsWithLatestEvent.length}
/>
Expand Down
Loading