search

day8 / re-frame-10x

A debugging dashboard for re-frame. X-ray vision as tooling.
MIT License
636 stars 68 forks source link

[Bug]: Path inspector left and right-click issues #347

Closed Gregg8 closed 1 year ago

Gregg8 commented 2 years ago

Firstly, I must say I love this feature.

Congrats @MawiraIke for doing the hard work!

1 - Vectors can cause incorrect path to be generated

Steps I used to reproduce:

Take for example, this path which I manually constructed and works...

[:vega/specs :curve-lines "layer" 0 "mark" "size"]

(1) I started with a blank inspector and started clicking down the tree...

Clicked on :vega/specs: => [:vega/specs]

Then clicked on :curve-lines: => [:vega/specs :curve-lines]

Then clicked on layer: => [:vega/specs :curve-lines "layer"]

Then clicked on mark: => [:vega/specs :curve-lines 0 "mark"]

This is the point at which it failed:

(2) Starting again (from a blank inspector) I click on item 2...

Clicked on :curve-lines: => [:vega/specs :curve-lines]

Then clicked on layer: => ["layer"]

This time it removed all 2 elements before appending

(3) Starting again, I click on item 3, layer: => [:vega/specs :curve-lines "layer"]

Then clicked on mark: => [0 "mark"]

(4) Starting again, I click on item 4, mark and this works: => [:vega/specs :curve-lines "layer" 0 "mark"]

(5) Starting again, I click on item 5, size and this works: => [:vega/specs :curve-lines "layer" 0 "mark" "size"]

So it seems to have a problem with vector/array boundaries when you are already nested down the tree.

2 - Sets can cause incorrect path to be generated

Steps I used to reproduce:

3 - The Copy REPL command fails

Steps I used to reproduce:

4 - Expand arrow becomes unclickable

Steps I used to reproduce:

5 - Current path is erased when you right-click > Copy path

Steps I used to reproduce:

10x Version

1.2.1

Reagent Version

1.1.0

React Version

17.0.2-0

re-frame Version

1.2.0

What browsers are you seeing the problem on?

Chrome

MawiraIke commented 2 years ago

Thanks @Gregg8 for spotting this. Can you please confirm if the paths returned by devtools in issue 1(Vectors can cause incorrect path to be generated) and issue 2 (Sets can cause incorrect path to be generated) are correct? If you are running a development version of 10x you can print the devtools-path and index variables on this line. On this line, we conj the path of a clicked object (index) to the existing nested path (devtools-path.) If devtools path is correct, then that line is the obvious culprit.

Gregg8 commented 2 years ago

When you said "paths returned by devtools", I initially interpreted that to mean "paths returned by right-click > Copy path", so I tested that.

I get mixed results (which might also be useful info when you're working on this):

I am not set up with a dev version of 10x and not sure how much time I have to do that. I'm hoping with all the above detail, you would be able to set up some test data to reproduce the issues.

Note: While testing that, I found another thing I'd consider to be an issue (have added it above):

MawiraIke commented 2 years ago
  • While nested, I get different wrong results (e.g. if the path is currently [:vega/specs :curve-lines] and I copy the path of layer, I get ["layer"] instead of [:vega/specs :curve-lines "layer"]

This would mean that the issue is in how we merge paths after receiving them from devtools which would point to this line. I will try to reproduce this issue and provide a fix. Thank you again.

MawiraIke commented 2 years ago

After some digging it seems like issues 1(Vectors can cause incorrect path to be generated) and 2(Sets can cause incorrect path to be generated) are an effect of this line in devtools. I think the issue of wrong paths is only occurring in sequential collections which contain integers, specifically in the paths of the integers and the values immediately after the integers within the collection. All other paths in the collection should be correct if I am right. I will push a fix to devtools to fix this issue.

MawiraIke commented 2 years ago

More details about these issues. In issue 1 - Vectors can cause incorrect path to be generated. I have made the following discovery, Using the data below as a vector:

[:vega/specs :curve-lines "layer" 0 "mark" "size"]

When you click the values in the vector the path displayed should be a number/index. That means, that instead of the following paths

=> [:vega/specs]

=> [:vega/specs :curve-lines]

=> [:vega/specs :curve-lines "layer"]

=> [:vega/specs :curve-lines 0 "mark"]

I should get 0, 1, 2, (Skipped 3 - explained below) and 4. This is exactly what I get for this simple vector (Would be different if the vector was nested in another data structure.) The issue I get is that, when I click on 0 instead of getting the path 3, I get 0. This is the issue that I can reproduce and fix and occurs in all sequential collections. As indicated in the previous comment, this is as a result of these two lines 1 and 2

For issue 2, I am polishing a fix to devtools that should include extra code implementation for sets. Sets are not currently handled correctly in the implementation of paths. I have got to a point where path generation in sets works normally unless the set has children which are composite data types like vectors, lists, maps etc. Then, the paths of the composite children will have their position in the set missing. This is due to sets having random order.

e.g Assuming the data below

{:a 2 
 :c #{2 3 [4 5]}
 :b 3}

The current implementation will have the values 4 and 5 with invalid paths that look like [:c 0] and [:c 1] respectively. Note that their position in the set is skipped.

I cannot reproduce issue 3 yet and I am still having issues reproducing issue 4 consistently. More details will come later.

MawiraIke commented 2 years ago

The following tasks are highlighted in this issue,

I have attached a PR that fixes most of the issues mentioned above except 2;

Gregg8 commented 2 years ago

Have been testing the items you checked off and they all now look good to me 👍

Outstanding issues

Expand arrow stops working

Copy REPL command fails

Further feedback:

Finally...

Just a comment (and possibly an enhancement request), I note that in terms of path inspector nesting, we're a bit stuck in terms of lists. Comparing get-in with vectors and lists:

Works with vectors:
(get-in [{:a 1}] [0 :a])
=> 1

Not with lists:
(get-in '({:a 1}) [0 :a])
=> nil

I haven't looked up why get-in works with vectors but not lists. There's probably a good reason.

Since the path in the inspector is used with get-in, you can't currently narrow down to individual list items, which could themselves contained nested data.

I note that nth DOES work with lists so perhaps there's a solution here.

MawiraIke commented 2 years ago

Thanks @Gregg8 for the feedback. Some comments on the above mentioned issues:

  1. Copy REPL command fails. If you are using a very large db, the issue causing this could be loading the db to memory. This can also be thrown when you "Copy Object" of the same. I am looking at some fixes that could prevent the crashing or warn users when working with these kind of objects.
  2. The button with the print icon can freeze up the UI for a long time. I have updated the print button tooltip. As of #356 the freezing issue seems to be gone. This could be possibly as a result of reverting to display only the first 100 items in devtools.
  3. Large lists/vectors no longer paginate. I have reverted to reveal the first 100 items reverting my previous commit. This also happened to be the cause of UI freezing when a very large db was used. This change is already included in release 1.2.4.
  4. After reverting pagination to reveal the first 100 items. I have discovered that devtools returns the index of every batch of 100 items starting from 0 - 99. That is, there is no continuation of paths after :max-number-body-items. This will need a new release in devtools to fix that issue before it can be seen in re-frame-10x. EDIT: ~This should be fixed after this PR is merged in devtools and re-frame-10x~. This has been merged and the new devtools version committed in #356
Gregg8 commented 2 years ago

Tested 1.2.6:

  1. In my experience, where "Copy object" will work just fine, "Copy REPL" on the same object still always completely crashes the web page
  2. The print button now works. But it does slow down relative to the size of the app-db branch you're displaying. For example, dumping my entire app-db took almost 30 seconds. I am guessing there's a point in the code where it is creating a string from the object. Perhaps this is necessary, hopefully not
  3. Yes, this is now working as expected
  4. Great
MawiraIke commented 2 years ago
  1. I am confirming that this is true. I'm guessing the reason why this happens is because "Copy object" could copy a small object within a larger (non copy able object in the current state of 10x) while to "Copy repl", the full app db is included in the copied string.
  2. I am finding that the printing is almost instant in my case. The output is a devtools header that can be expanded. Expansion is almost instant too and only the first 100 items are displayed.
Gregg8 commented 2 years ago
  1. We confirmed that this problem is due to the raygun library we're using for exception logging:
    • It now monkey patches the console.log.toString function
    • As part of its custom handling it turns the payload into a string which is the cause of the slowness of printing large objects to the console
    • Others have issue with it also: https://github.com/MindscapeHQ/raygun4js/issues/348
    • We've since made changes so that this patching is disabled when we're debugging the app
    • Now a full app-db is pretty much instant to dump to the console

Note: with our new code in place, I retested Copy REPL command and it still crashes the browser page

kimo-k commented 1 year ago

I think I've narrowed down all the issues still open here. Let's open a new ticket if anything else comes up.

I did a rewrite of the popup menu, since it was made of all imperative interop stuff. Now it's standard reagent/re-frame: 9c27b45

My takeaways so far: