Fix search element selection

[tsriram]

This fixes #26 (hopefully)

It seems that the bug reported with search selection occurs because we update selectedElementID inside if condition which is likely to fail most of the times. searchIndex & prevSearchIndex would be 0 when there are elements in searchResults array and they don't seem to change if user doesn't manually move to previous / next search result. And even if the searchResults array is updated, searchIndex could still be 0 and point to a different element in the tree which should be highlighted. Updating the selectedElementID fixes this issue. I'm not sure why was this if condition added and removing this would impact anything else. Please let me know if this is not the right fix :)

Before:

After:

[bvaughn]

Thanks for the PR @tsriram! I'm excited to review it when I get to a good stopping point with the profiler. That may take a few days. Just wanted to say "thanks" in the meanwhile.

[bvaughn]

The idea behind this was to avoid having to pay the cost of getIndexOfElementID every time the context re-rendered. That lookup isn't too expensive but it's also something I'd like to avoid doing unnecessarily. In theory, if the index didn't change, we shouldn't need to look things up again.

I wonder if we could treat 0 as a special case...

Ideally we could just detect the case where we need to re-run the computation for index 0, and reset prevSearchIndex to e.g. null at the start of the reducer.

[bvaughn]

Did you come up with a small repro steps by chance? 😄 I forget what sequence used to trigger this.

[tsriram]

Yeah, we could handle index 0 as a special case and updating the original if condition with an additional check for index 0 makes it work. Does this make sense?

Reproduction is quite easy (if I understand it correctly 😉):

1. Go to https://react-devtools-experimental.now.sh/
2. In the devtools search box, type list - first element <List> should be selected
3. Continue typing and enter listitem - ideally the second element in the tree <Memo(ListItem)> should be selected but the first element is still shown as selected.

You could see another example in my original screen grabs as well in the PR.

[bvaughn]

I think Dan found a slightly better fix for this (#62) and I'm now wondering if this PR is also still necessary. I'd love to hear your thoughts?

[tsriram]

Oh yeah, that's definitely a better fix. Will close this PR. Thanks!

[bvaughn]

Thanks for taking a look and thanks for the PR! 😅 Sorry it took me a few days to dig in

[tsriram]

No worries @bvaughn 😄