Skip redux middleware frames on jumping to location

[Mokshit06]

Experimenting with using babel to figure out which pause frames are of redux middlewares instead of dispatch.
It finds the identifier at the column and line where the first user frame paused and matches if the parent functions have the pattern store => next => action => {} using babel traversal. If so then skips that frame and moves to the next one.

This is a very basic test, and needs to check for more cases, like checking at each step that the parent function is returned the child one. eg. in next => action => {}, action => {} is the child function. It's parent function is next => {}, and next is returned action.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
devtools	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 31, 2023 6:50pm

[replay-io]

E2E Tests

71 replays were recorded for d1fe5da.

0 Failed

71 Passed

• authenticated/comments-01: Test add, edit, and delete comment functionality
• authenticated/comments-02: Test shared comments and replies
• authenticated/comments-03: Comment previews
• authenticated/logpoints-01: Shared logpoints functionality
• breakpoints-01: Test basic breakpoint functionality
• breakpoints-02: Test unhandled divergence while evaluating at a breakpoint
• breakpoints-03: Test stepping forward through breakpoints when rewound before the first one
• breakpoints-04: catch, finally, generators, and async/await
• breakpoints-05: Test interaction of breakpoints with debugger statements
• breakpoints-06: Test log point in a sourcemapped file
• breakpoints-07: rewind and seek using command bar and console messages
• breakpoints-08: should be temporarily disabled
• console_async: support console evaluations in async frames
• console_dock_node: Should show the correct docking behavior for recordings without video
• console_dock: Should show the correct docking behavior for recordings with video
• console_errors: Test that errors and warnings from various sources are shown in the console
• console_eval: support console evaluations
• console_warp-01: should support warping to console messages
• console_warp-02: support pausing, warping, stepping and evaluating console messages
• console-expressions-01: should cache input eager eval and terminal expressions per instance
• cypress-01: Basic Test Suites panel functionality
• cypress-02: Test Step timeline behavior
• cypress-03: Test Step interactions
• cypress-04: Test Step buttons and menu item
• focus_mode-01: should filter messages as regions based on the active focus mode
• highlighter: element highlighter works everywhere
• inspector-01: Test that scopes are rerendered
• inspector-02: element picker and iframe behavior
• inspector-03: Test that styles for elements can be viewed
• inspector-04: Test that styles for elements can be viewed
• inspector-05: Test that styles for elements can be viewed
• inspector-06: Test that styles for elements can be viewed
• inspector-07: Test that styles for elements can be viewed
• logpoints-01: log-points appear in the correct order and allow time warping
• logpoints-02: conditional log-points
• logpoints-03_chromium: should display event properties in the console
• logpoints-03: should display event properties in the console
• logpoints-04: should display exceptions in the console
• logpoints-05: should auto-complete based on log point location
• logpoints-06: should be temporarily disabled
• logpoints-07: should use the correct scope in auto-complete
• logpoints-08: should support jumping directly to a hit point via the capsule input
• logpoints-09: should support pending edits
• node_console-01: Basic node console behavior
• node_console-02: uncaught exceptions should show up
• node_control_flow: catch, finally, generators, and async/await
• node_logpoint-01: Basic node logpoints
• node_logpoint-02: Node exception logpoints
• node_object_preview: Showing console objects in node
• node_spawn: Basic subprocess spawning
• node_stepping-01: Test stepping in async frames and async call stacks
• node_worker-01: make sure node workers don't cause crashes
• object_preview-01: expressions in the console after time warping
• object_preview-02: should allow objects in scope to be inspected
• object_preview-03: Test previews when switching between frames and stepping
• object_preview-04: Test scope mapping and switching between generated/original sources
• object_preview-05: Should support logging objects as values
• quick_open_modal-01: Test basic searching functionality
• react_devtools: Test React DevTools.
• repaint: repaints the screen screen when stepping over code that modifies the DOM
• resizable-panels-01: Left side Toolbar should be collapsible
• scopes_rerender: Test that scopes are rerendered
• sourcemap_stacktrace: Test that stacktraces are sourcemapped
• stacking: Element highlighter selects the correct element when they overlap
• stepping-01: Test basic step-over/back functionality
• stepping-02: Test fixes for some simple stepping bugs
• stepping-03: Stepping past the beginning or end of a frame should act like a step-out
• stepping-04: Test stepping in a frame other than the top frame
• stepping-05_chromium: Test stepping in pretty-printed code
• stepping-05: Test stepping in pretty-printed code
• stepping-06: Test stepping in async frames and async call stacks

View test run on Replay ↗︎

Snapshot Tests

104 replays were recorded for d609cd2.

0 Failed

104 Passed

• should add true/false keywords to the list of suggestions
• should add this keyword to the list of suggestions
• should be able to toggle side filter menu open and closed
• should be filterable on complex content
• should be filterable
• should be searchable on complex content
• should be searchable
• should display list of messages
• should display toggleable stack for errors
• should display toggleable stack for exceptions
• should display toggleable stack for traces
• should display toggleable stack for warnings
• should escape object expressions in terminal expressions
• should evaluate and render invalid terminal expressions
• should evaluate terminal expressions at an execution point
• should evaluate terminal expressions without an execution point
• should expand and inspect arrays
• should expand and inspect objects
• should hide node_modules (and unpkg) if toggled
• should log events in the console
• should re-focus the search input when CMD+F is used again
• should re-focus the terminal input when search input is hidden
• should refine string tokens to avoid unnecessarily escaping quotation marks
• should remember filter toggle preferences between reloads
• should show a button to clear terminal expressions
• should show error object stacks by default
• should show the context menu on top of other messages and the current time indicator
• should suggest type-ahead options from prototype objects as well
• should support seeking to a message execution point
• should support setting focus range via the context menu
• should support terminal expressions with line breaks
• should render nested, recursive values (as ellipses) without crashing
• should copy arrays
• should copy dates
• should copy deep arrays
• should copy deep objects and their nested properties
• should copy errors
• should copy functions
• should copy html elements and text
• should copy maps
• should copy objects
• should copy primitive client values
• should copy regular expressions
• should copy sets
• should properly bucket properties for an overflowing array
• should render and inspect a Promise
• should render and inspect a Proxy
• should render and inspect arrays
• should render and inspect functions
• should render and inspect HTML elements
• should render and inspect maps
• should render and inspect objects
• should render and inspect regular expressions
• should render and inspect sets
• should render dates
• should render errors
• should render getters and setters correctly
• should render simple values
• should render sections correctly
• should show/hide object and array previews when toggled
• scroll position should be restored when switching between sources
• should account for column breakpoints with plain text
• should account for column breakpoints
• should allow log point badge colors to be toggled
• should auto-suggestion text based on the current expression
• should disable syntax highlight for long lines
• should expand and contract line height when log points are added and removed
• should gracefully handle invalid remote analysis
• should handle too many points to find
• should handle too many points to run analysis
• should hide results when search is closed, and remember previous search string if re-opened
• should include log points in search results
• should include log points when filtering data
• should not allow saving log points with invalid conditional
• should not allow saving log points with invalid content
• should not erase break point when logging is toggled
• should not erase log point content when breaking is toggled
• should overflow long log point text (without wrapping)
• should properly toggle breakable and loggable behaviors when there are multiple breakable positions on a line
• should remember search results count per source
• should render search results properly for lines with multiple breakable column positions
• should run local analysis for log points
• should run remote analysis for log points
• should scroll to line and column
• should show different background color and edit icon when log point disabled
• should show preview popup for instance properties
• should support break points
• should support conditional log points
• should support continue to next and previous functionality
• should support custom badge styles for log points
• should support fuzzy file search
• should support jumping to a line in the active source file
• should support multiple breakable column positions on a line
• should support new lines in log points
• should support text search in the active source file
• should update the current time when the next/previous log point buttons are clicked
• should render headers for complex data correctly
• should render primitive data correctly
• display an overflow message
• should expand and collapse sources
• should find matches in multiple sources
• should include or exclude external modules as requested
• should open a source and scroll to the correct line
• should reset collapsed state if search changes

View test run on Replay ↗︎

[jasonLaster]

it'd be nice to write some jest unit tests for this function

devtools/src/ui/utils/time.test.ts

Line 4 in 7731de4

describe("formatDuration", () => {

devtools/src/ui/utils/app.test.ts

Line 4 in 7731de4

describe("getNonLoadingRegionTimesFromLoadingRegions", () => {

[jasonLaster]

Now that this function is getting more complex, it might be nice to pull this logic for getting the matching frame step into a util

function getMatchingFrameStep(frameSteps, preferredLocation) {
  ...
}

btw i'm not sure if this is the best API.

[markerikson]

FWIW the real answer here is to implement https://linear.app/replay/issue/BAC-2915/stack-frames-should-come-with-execution-points and stop trying to calculate this ourselves.

[Mokshit06]

I've moved it into a separate util for now since ReactPanel.tsx and ReduxDevtools.tsx are doing the same thing but yes having frames have their execution point would make it alot simpler

[Mokshit06]

This should now be able to give more accurate results. It can identify code like store => next => action => {} and store => next => { return action => {} } and any combination of implicit & explicit returns as redux middleware.

Need to add code for handling cases where the callback gets declared first, and is later returned

[markerikson]

Not sure this is working as intended atm. If I look at https://devtools-git-fork-mokshit06-redux-skip-middleware-recordreplay.vercel.app/recording/breakpoints-01-test-basic-breakpoint-functionality--326d54a3-1523-411a-95ca-018537f5a887 , the jump locations are all still landing inside of the context middleware.

[Mokshit06]

Oh I know why this example isn't working. Babel isn't able to parse the typescript code. Will need to include typescript plugin for ts/tsx files. Try running it on JS examples like https://devtools-dk0k2y9tu-recordreplay.vercel.app/recording/e2etestscenariosjoinsreproductions22859-multi-nested-joins-wrong-aliasingcyspecjs--77ed307a-f284-4d5d-b0e0-b5176d252326

[markerikson]

@Mokshit06 I just realized there might be a simpler approach here.

All Redux middleware are going to be going through applyMiddleware():

Can we do something to find the dispatch method in the call stack that was defined inside of applyMiddleware(), and go back one frame before that?

[Mokshit06]

Oh that's a good point. How often does the name/path of this file change? Maybe that could be a good way to identify if it's the correct step. Another option could be to go through all the frame steps and based on the sourceOutlineCache check if the current pause location is between a function named applyMiddleware

[markerikson]

The filename shouldn't change, but there are some funky edge cases I've found where there can be more than one copy of node_modules/redux/es/redux.js in a project. In one case, Redux was getting added at the top level as node_modules/redux/, but also under node_modules/redux/@redux-saga/something/node_modules/redux/. So, there were two copies of redux.js, and only one of them was getting hit.

Was working on some of that logic over in #9410 for the perf analysis POC.

Want to try grabbing some of that "find the right Redux file" code from there and use it here to identify the right method here? I think you can actually borrow a bunch of the logic that was already trying to find "the right dispatch method inside of createStore", except it should be "inside of applyMiddleware instead".

[markerikson]

✋ It'll be smoother if we do the applyMiddleware function lookup once, then have a separate function that does the "is frame in function declaration" comparison synchronously

[markerikson]

Not sure this is a good place for this code to go, but I also don't have a better suggestion atm :)

[markerikson]

Looks reasonable at this point - can you fix up the merge conflict?

[markerikson]

Hmm. I just tried it on a Metabase recording, and ended up exactly inside of the original saveAnnotations() call:

• https://devtools-git-fork-mokshit06-redux-skip-middleware-recordreplay.vercel.app/recording/e2etestscenariosjoinsreproductions18512-cannot-join-two-saved-questions-with-same-implicit-explicit-grouped-fieldcyspecjs--f455216f-8167-40f4-b1e1-3d3af2a83f30

I definitely see the applyMiddleware -> dispatch method in the call stack, so we should have found that.

Can you do some more looking into this?

[Mokshit06]

Ah so while resolving the conflict I see this got changed to the initial point and time rather than the calculated ones.

I'm out right now but will fix this.

[jasonLaster]

@Mokshit06 looks like this needs a rebase

[Mokshit06]

I tried this on a recording of the Redux starter template and a call to applyMiddleware() isn't there in the call stack for some reason, so it instead jumps inside an RTK middleware instead of the preferable onClick handler.

https://devtools-bihivkc27-recordreplay.vercel.app/recording/replay-of-localhost5173--b90a2bd3-563a-4682-9c6e-bdddc61c7042

[markerikson]

Per discussion in Discord, Mokshit is going to try looking at figuring out "are we in a middleware" by looking just at source outline data to determine if this is a triply-nested function.

[Mokshit06]

@markerikson This seems to be working now. It tries to search the call stack initially for applyMiddlware call. If it finds one then uses that frame, else tries the source outline data, and if both fail then jumps to the third frame (the one after the 2 replay patched steps).

[markerikson]

Hah, I hate to keep asking you to make more tweaks, but I see one more filename we should exclude: bindActionCreators . Example, see https://devtools-git-fork-mokshit06-redux-skip-middleware-recordreplay.vercel.app/recording/breakpoints-01-test-basic-breakpoint-functionality--326d54a3-1523-411a-95ca-018537f5a887 , and jump to SET_FOCUSED_SOURCE_ITEM , which was dispatched by an older-style connect()-wrapped component.

[markerikson]

Okay, looks good! Just need to fix the merge conflicts and I think this is ready!

[markerikson]

Huh, looks like this is missing a process.env.AUTHENTICATED_TESTS_WORKSPACE_API_KEY somehow? that's causing all the E2E tests to fail and bail out.

[Mokshit06]

Hm that's weird, I don't see anything in this PR that would change the env variables

[markerikson]

Superseded by #9573 .