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

feat(hydration error): Create a tree-view that highlights dom mutations directly #80808

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

ryan953
Copy link
Member

@ryan953 ryan953 commented Nov 15, 2024

SCR-20241115-mtqw

Fixes #74358

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Nov 15, 2024
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 0% with 88 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...pp/utils/replays/hooks/useExtractDiffMutations.tsx 0.00% 79 Missing ⚠️
...app/components/replays/diff/replayMutationTree.tsx 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #80808      +/-   ##
==========================================
+ Coverage   74.97%   78.39%   +3.41%     
==========================================
  Files        7207     7208       +1     
  Lines      319509   319549      +40     
  Branches    44001    44003       +2     
==========================================
+ Hits       239546   250498   +10952     
+ Misses      73442    62671   -10771     
+ Partials     6521     6380     -141     

@ryan953 ryan953 marked this pull request as ready for review November 15, 2024 16:46
@ryan953 ryan953 requested a review from a team as a code owner November 15, 2024 16:46
Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

How many times does replayStepper run when we open this modal?

static/app/components/replays/diff/replayMutationTree.tsx Outdated Show resolved Hide resolved
queryFn: () =>
extractDiffMutations({replay, rangeStartTimestampMs, rangeEndTimestampMs}),
enabled: true,
gcTime: 0, // Infinity,
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, had this set to 0 for testing because i didn't want the cache to kick in so often.

Copy link

codecov bot commented Nov 16, 2024

Bundle Report

Changes will increase total bundle size by 44.89kB (0.14%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
app-webpack-bundle-array-push 31.9MB 44.89kB (0.14%) ⬆️

@ryan953
Copy link
Member Author

ryan953 commented Nov 16, 2024

I was thinking about adding a feature-flag around this.. but it's pretty deep inside the workflow, so i think I might not 'need' it even if it's a good habit in all cases. Exception to the rule!

@ryan953 ryan953 requested a review from billyvg November 19, 2024 06:51
@ryan953
Copy link
Member Author

ryan953 commented Nov 20, 2024

I had an idea that maybe we should be formatting the html. However the add/remove sections show potentially huge chunks of html though, which could be a perf hit. So with that, lets let formatting be a followup task if this proves an interesting tool.
The attributes section just shows one node, with children elided, so no formatting needed for that imo.

@ryan953
Copy link
Member Author

ryan953 commented Nov 20, 2024

Added a docs PR to go along with this new tool: getsentry/sentry-docs#11883

@ryan953 ryan953 merged commit f577af7 into master Nov 21, 2024
44 of 45 checks passed
@ryan953 ryan953 deleted the ryan953/hydration-mutation-tree branch November 21, 2024 18:00
@whitep4nth3r
Copy link

whitep4nth3r commented Nov 22, 2024

Hello!

I had a look at this feature this morning and am providing some UX feedback as a developer who:

  • knows about hydration errors
  • is a front end developer
  • does not know all the behind-the-scenes details of hydration errors but needs to debug them now and then

Th mutations tab is an interesting window into the internals happening with a Hydration Error, but I'm having trouble understanding how the mutations relate to the HTML Diff, and therefore finding the mutations tab largely useless.

e.g. This HTML diff begins at either a div or a style tag (depending on if you consider the before or after)
image

And here on the mutations tab, I don't really see anything that matches the HTML diff, and it doesn't give me any really useful information
image

The ways in which I could interpret this is The style attribute was changed on a span element. But that's not what the diff shows.

So my questions are:

  1. Is this a bug? Or is this the feature working correctly?
  2. If it is working correctly, how can we present this in a way that is useful for the user and for it to mean something so they can get debugging asap.
  3. Do we need to explain what they're looking at on the mutations tab, or at least provide a link to more info? Because I'm not entirely sure what I'm looking at. HTML diff, sure, I know what's going on, but how is the mutation causing the HTML diff? Is it even?

This is also a valid option: maybe we are assuming prior knowledge intentionally, and maybe, as someone who doesn't use Next.js all the time, maybe I'm not the target audience. But I would also argue that displaying these kinds of features in a more "you can understand what's going on here even if you don't have any prior experience with this particular thing" would help prove the use of Sentry earlier on.

I have two more questions for you:

  1. What fundamental user need is this feature solving?
  2. Is the need met as soon as it possibly can be?

I actually think the text diff was more immediately useful (showcased in this blog https://blog.sentry.io/sentry-cant-fix-react-hydration-errors-but-it-can-really-help-you-debug-them/)

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
3 participants