File tree improvements #9916
File tree improvements #9916
Conversation
- Use a single ?subtree query parameter instead for suggestions and actions. - Construct the url with the query param in the web directory. - Do not refresh tree when panel is open
|
I am going to merge this, I am quite sure that the cc @felixfbecker in case you want to take another look since you left the original PR comments. |
web/src/tree/Tree.tsx
Outdated
| queryParams.delete('action') | ||
| // Strip the ?subtree query param. Handle both when going from ancestor -> child and child -> ancestor. | ||
| if (queryParams.has('subtree')) { | ||
| queryParams.delete('subtree') |
There was a problem hiding this comment.
I'd move this after the if so it would remove false values to (delete() is a noop if the param does not exist)
There was a problem hiding this comment.
Do you mean the entire content of this if statement? Can't move just the delete statement out since the history update needs the new queryParams.
There was a problem hiding this comment.
But if you move it after the if, the history update inside the if will still have access to it, wouldn't it?
There was a problem hiding this comment.
To be clear I mean only the delete call.
There was a problem hiding this comment.
The history update will have access to the old query params... without the subtree deleted. We want to delete the subtree param before updating history, otherwise it's just going to update to the same URL infinitely.
There was a problem hiding this comment.
oh, you're right, I misunderstood the intention. I'd change my proposal to move it above the if.
|
I can't find any mention of the |
Co-Authored-By: Felix Becker <felix.b@outlook.com>
Co-Authored-By: Felix Becker <felix.b@outlook.com>
Codecov Report
@@ Coverage Diff @@
## master #9916 +/- ##
=======================================
Coverage 42.76% 42.76%
=======================================
Files 1347 1347
Lines 73949 73947 -2
Branches 6641 6636 -5
=======================================
Hits 31625 31625
+ Misses 39464 39462 -2
Partials 2860 2860
|
Co-Authored-By: Felix Becker <felix.b@outlook.com>
Co-Authored-By: Felix Becker <felix.b@outlook.com>
Co-Authored-By: Felix Becker <felix.b@outlook.com>
|
@felixfbecker I just added it in the latest commit. |
- Use a single ?subtree query parameter instead for suggestions and actions. - Construct the url with the query param in the web directory. - Do not refresh tree when panel is open
Addresses outstanding comments in https://github.com/sourcegraph/sourcegraph/pull/9842.
Also fixes a small unintended bug in #9842. When the go-to-def or find refs button is clicked, the
?subtreeparam would be included in the URL, so when the references or definition panel appears, the file tree updates to only show the current directory, which is not what we want. So I check to make sure thetab=parameter is not present before updating the tree. cc @uwedeportivo I would like to get this cherry-picked into the release.