Skip to content
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.

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

ref(feedback): fix show/hide logic for trace section, use placeholder while loading #79284

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

aliu39
Copy link
Member

@aliu39 aliu39 commented Oct 17, 2024

Ref #68481

The show/hide logic had a bug where we were showing only if the same-trace issue is a duplicate of a crash report linked error. Which is the opposite of what we want. Example: JAVASCRIPT-2VE1. Analytics are still correct, but atm no timelines (2+ events) are shown.

Also:

  • use placeholder when the data is fetching.
  • use error boundary.
  • remove the hasProject prop. It makes the logic hard to read, and was only used for the crash-report-dup condition. Comparing the issue ids is sufficient for this.

@aliu39 aliu39 requested a review from a team as a code owner October 17, 2024 17:58
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Oct 17, 2024
@aliu39 aliu39 changed the title ref(feedback): use placeholder for trace section while loading, fix show/hide logic ref(feedback): fix show/hide logic for trace section, use placeholder while loading Oct 17, 2024
(!hasProject || !crashReportId || oneOtherIssueEvent?.id === crashReportId);
// Note traceEvents includes the current event (feedback).

const [isCrashReportDup, setIsCrashReportDup] = useState(false);
Copy link
Member

Choose a reason for hiding this comment

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

what is this used for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whether to show or hide the whole section - see return statement

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops the condition should be !isCrashReportDup !!

Copy link
Member

Choose a reason for hiding this comment

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

what exactly does is crash report dup mean? Sorry I don't really understand the bug

Copy link
Member Author

Choose a reason for hiding this comment

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

Feedbacks submitted with an associated event ID have a section called "Linked Error" (CrashReportSection in code). When oneOtherIssueEvent is defined, it means only one other event happens in the same trace. If the linked error and this event are the same, these sections would display a link to the same thing, and that's kinda redundant.

The bug was displaying this section only when it's a duplicate, instead of when it's not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically the other section gets error data from a SDK-submitted, explicit event ID, while this section gets it from the trace. In some cases having both would be redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

Screenshot 2024-10-17 at 11 23 02 AM
From my linked example

Copy link
Member

Choose a reason for hiding this comment

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

do we need the state here? it seems like const isCrashReportDupe = !isError && !isLoading && !!crashReportId && oneOtherIssueEvent?.id === crashReportId; would be the same thing.

</Section>
) : null;
}

function eventIsCrashReportDup(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function eventIsCrashReportDup(
function eventIsCrashReportDupe(

@aliu39 aliu39 enabled auto-merge (squash) October 17, 2024 20:18
@aliu39 aliu39 merged commit 17dc703 into master Oct 17, 2024
42 of 43 checks passed
@aliu39 aliu39 deleted the aliu/trace-section-placeholder branch October 17, 2024 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants