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

[Gregg8]

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:

• It removed "layer"
• I couldn't click on an anything representing the 0 as that's just the vector offset
• So it needed to append 2 elements to the vector, not 1
• It did this but it clobbered the last one before doing so
• It fails in other ways too

(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:

• I note that clicking on both the key and the value will set the path
• Take this example path which points to a set containg strings: [:curves/processing-step :job/task-ids]
• Clicking on :job/task-ids correctly produces [:curves/processing-step :job/task-ids]
• But clicking on any of the elements in the set produces this incorrect path: [:curves/processing-step :job/task-ids :job/task-ids]

3 - The Copy REPL command fails

Steps I used to reproduce:

• Clicking on this button just dumps a massive data structure into the clipboard, probably app-db itself
• Sometimes it works and sometimes:
  • I get the Page Unreponsive popup
  • I can watch RAM being gobbled up until I kill the page or it dies on its own

4 - Expand arrow becomes unclickable

Steps I used to reproduce:

• I have left this as a placeholder because I have not been able to reliably reproduce
• But I recall setting a number of paths, then clearing it out

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

Steps I used to reproduce:

• When I right-click > Copy path while I'm nested in the tree, it clears the nested path to empty and returns me back to the full app-db
• It should leave the current path as is

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]

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]

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):

• If I copy the path while NOT nested (that is, path is currently empty), it works
• 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"]

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):

• When I right-click > Copy path while I'm nested in the tree, it clears the nested path to empty and returns me back to the full app-db. It should leave the current path as is

[MawiraIke]

• 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]

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]

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]

The following tasks are highlighted in this issue,

• Incorrect path generation in nested objects
• Incorrect path generation in sets
• Copy REPL command fails
• Expand arrow stops working
• Current path is erased when path is copied from Menu

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

• Expand arrow stops working. I can not reproduce this reliably enough to determine the issue.
• Copy REPL command fails. I have not been able to reproduce this issue yet.

[Gregg8]

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

Outstanding issues

Expand arrow stops working

• I can reliably make it happen for me
• Open a new inspector
  • PS: I'm thinking the default for a new inspector should be opened because the first thing I do when I create one is open it. Maybe others will disagree, depending on their usage, in which case, a checkbox in Settings?
• Double-click on one of the top-level keys
• At this point I find that the top-level expand arrow does not work, but some of the map keys under it do (and some don't)
• Note: I can always close and open the inspector itself and this fixes everything (reattaches all event handlers)

Copy REPL command fails

• This continues to happen for me every time (and another colleague)
• I wonder if it's because we are working with a large app-db?

Further feedback:

• The Expand button (as opposed to the expand arrow discussed above) is both a wonderful and terrible tool
  • Click it on a full app-db and, assuming you're using a decent size app and not Todo MVC. The UI is now dead
  • This is because it is trying to render potentially hundreds of thousands of lines of data
  • I wonder what the best way is to determine in advance that expanding the current object will kill it
  • As I said, it's also wonderful, when you're nested into app-db and you just want to quickly look at all data. Works a treat
• The button with the print icon can freeze up the UI for a long time (for a large app-db)
  • Eventually it dumps the current inspector into the console using js/console.log
  • Froze for 20 seconds for me
  • I'm guessing it must be realising the entire app-db as a string somewhere along the way
  • This button doesn't have a tooltip, btw. How about "Dump inspector data into DevTools"?
• Expand button tooltip
  • While we're on tooltips, I believe the should be changed from "Expand app-db" to "Expand all nodes in this inspector"
• Large lists/vectors no longer paginate
  • Previously when you expanded one, it would reveal the first 100, and add a more... link, which would reveal the next 100, and so on
  • Now it renders all items, which can freeze or crash the UI
  • Please restore this functionality

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]

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 Fix menu positioning #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 Fix menu positioning #356

[Gregg8]

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]

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]

2. 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: raygun4js hijacking console.log stack trace in developer tools, even when not enabled. MindscapeHQ/raygun4js#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]

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:

• We can't put the whole app-db in the clipboard. Why don't we just use "@re-frame.db/app-db" instead?
• The popup menu doesn't open on collections, making "copy object" seem kind of pointless.
  • At least in todomvc, I don't see a need to copy numbers and keywords this way.
  • For now, I changed it to copy the value of the parent node instead.
• What does "copy REPL command" actually do for the user? Have we articulated this anywhere?