Fix detection of async parent pauses outside of the focus window #10573
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![replay-soc2[bot]](https://avatars.githubusercontent.com/u/60818315?s=60&v=4){kind=small}
|
How hard would it be to add tests for both scenarios? I'm not familiar with the test setup here so I'm just genuinely curious how much effort is required to add new tests |
That shouldn't be too hard and we should get back into the habit of writing tests so I'll create one. |
bvaughn
reviewed
Jun 18, 2024
src/devtools/client/debugger/src/components/SecondaryPanes/Frames/NewFrames.tsx
Show resolved
Hide resolved
bvaughn
reviewed
Jun 18, 2024
src/ui/suspense/util.ts
Outdated
Comment on lines
28
to
34
| const executionPoint = steps[0].point; | ||
| if ( | ||
| focusWindow === null || | ||
| !isExecutionPointsWithinRange(executionPoint, focusWindow.begin.point, focusWindow.end.point) | ||
| ) { | ||
| return null; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Returning true here if focusWindow === null seems funky?
I don't think focusWindow window is ever null (it's either the full recording or a subset) but if it were null, that would be the same as the full recording, wouldn't it? In which case, returning true here seems like it would always be misleading.
bvaughn
approved these changes
Jun 19, 2024
Comment on lines
+49
to
+51
| if (focusWindow === null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Returning null here still feels odd. I guess this will only ever happy briefly during initialization so maybe it doesn't matter.
markerikson
added a commit
that referenced
this pull request
Jun 28, 2024
* Jump-to-source falls back to parent user event if marker frame cannot be found (#10561) ### [Loom overview](https://www.loom.com/share/6d697c9136274fb8892325d39283fdac) Looking at [go/r/bb82f5c3-af5c-4e20-9f1c-5efa4ee6bd29](https://app.replay.io/recording/appreplayio--bb82f5c3-af5c-4e20-9f1c-5efa4ee6bd29), I observed that the `getCypressMarkerFrame` lookup fails to find a "marker frame" because there are only two sources in the call stack– the Replay plugin itself and Cypress. That method expects to find user code in between for some reason, so this breaks the jump-to-source behavior. In this case, the source is: ```js cy.contains('Product added to cart!').should('be.visible'); ``` That gets rendered in the UI as this:  Clicking on the failed _assert_ (or trying to "jump to source") would ideally jump to the `should('be.visible')` but it fails because of the "marker frame" issue I mentioned above. The idea occurred to me that in this case, jumping to the parent command (`contains('Product added to cart!')`) is probably better than nothing. (In this case, they're even on the same line, but that won't always be true.) * Add data-private attributes (#10562) * Remove [data-private] CSS style (#10563) * Reset selected test event when test changes (#10565) * Improve visual style for React/Reduxt not-loaded error messages (#10566) * Log point panel should never show 1/0 (#10568) * Fix video toggle for horizontal DEV layout (#10569) * Support multi-line print statements (#10570) Support multi-line print statements to improve readability and text editing experience. * Stop mousedown event propagation in the Focuser (#10575) * Adjusting contrast (#10572) * Always use hoverTime for the screenshot when the user is hovering (#10578) * Fix detection of async parent pauses outside of the focus window (#10573) * Revert "Improve message shown when async stack can't be loaded (#10557)" This reverts commit 585e560. * Fix detection of async parent pauses outside of the focus window * Remove LogRocket from Devtools (#10577) * Improve heuristics for finding the user frame that dispatched a Redux… (#10576) * Improve heuristics for finding the user frame that dispatched a Redux action The original logic was iterating backwards across the point stack frames looking for an applyMiddleware frame, and going backwards one more frame from there. This is problematic because I'm seeing variance in whether or not applyMiddleware actually shows up in stack frames. For some reason it shows up in Metabase recording stack traces, but not in our own E2E tests (and we're both using RTK). That meant that the existing logic could keep right on walking backwards long past the frame it "should" have found. What I've settled on is using the logic we had that tries to identify "is this function in a Redux middleware" based on source outlines (looking for the triply-nested function signature of a middleware definition), and stopping at the first frame that appears to be not a middleware and not applyMiddleware. This seems to be producing pretty reasonable results. * Update to a recent `breakpoints-01` recording * Force re-running the J2C routine * Add a util for verifying J2C behavior * Add a test for Redux J2C behavior * Ignore wrong lint error * Update RDT-02 test to match current example recording * Log point panel saves pending edits when removing condition (#10579) * Add option to notify user when processing is complete (#10582) * Guard against undefined value (#10581) * Source viewer font-size fixes (#10580) Print statement panel dynamic sizing bug fixes: - Fixed minor font-size and line-height inconsistencies for print statement panel - Handle edge case with empty last line (aka Shift+Enter)  Also updated the `useSourceListCssVariables` hook to reevaluate CSS variables (width of line numbers and hit counts) when the font size preference changes. * Better view element icon (#10583) * Enable asserts (#10584) * Handle lack of `point.frame` (#10586) * Remove unused `jumpToClickEventFunctionLocation` (#10585) * Improve video cursor display, especially for Cypress tests (#10587) * Extract utils to check for user click/keyboard test steps * Simplify UserActionEventRow J2C checks * Use mouse event times for hover display for click steps * Narrow time that we show a mouse click indicator * Only show mouse clicks within the current focus window * Revert focus window filtering for mouse cursors * Update the Cypress steps test to check for hover cursor position (#10588) * Update cypress/bankaccounts.spec example * Add test for Cypress cursor positioning * Use dashed/camel-cased data names --------- Co-authored-by: Brian Vaughn <[email protected]> Co-authored-by: Holger Benl <[email protected]> Co-authored-by: Jon Bell <[email protected]> Co-authored-by: Domi <[email protected]> Co-authored-by: Mateusz Burzyński <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#9368 broke the detection: before that PR
getAsyncParentPauseIdSuspense()would returnundefinedif there is no async parent pause ornullif it is outside the focus window, after that PR it always returnednull.With this PR
getAsyncParentPauseIdSuspense()will return abooleanin these cases instead of abusingnull/undefined, which obviously caused confusion leading to a regression.