Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/flow-tree-view #6673

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

lups2000
Copy link
Contributor

@lups2000 lups2000 commented Feb 20, 2024

Description

This PR should implement the flows tree view feature requested here: #5627. For the initial structure I took inspiration from the work of @Rishabhg71 (#5996). Then, I tried to reuse the component FlowTable that handles a lot of stuff already such as the sorting, selection, highlighting etc.
I am opening this draft PR so you can check what I am doing.

Screen.Recording.2024-02-20.at.14.50.47.mov

Checklist

  • Adjust UI
  • Adjust sorting since now is "global"
  • I have updated tests where applicable.
  • I have added an entry to the CHANGELOG.

@lups2000
Copy link
Contributor Author

Hello @mhils , I saw the sorting has been initially implemented in the state, so for the tree view now is "global" (as we can see in the recording). Basically, if I sort by time in the first table I sort by time in the other tables as well, which is not the best. What do you think? Should we consider refining this behavior? In case of change, I must understand properly the logic behind it to avoid to break something :)

@Prinzhorn
Copy link
Member

Judging from the video this looks interesting and I can see this being helpful. But that's not a tree and you can achieve similar things by filtering for a given host. The way I understand the linked issue is that if you have hundreds of flows in deeply nested routes such as /api/*/*/*/* there is no high-level way to view this structure to understand it and look at sub-routes etc.

@lups2000
Copy link
Contributor Author

lups2000 commented Feb 21, 2024

Judging from the video this looks interesting and I can see this being helpful. But that's not a tree and you can achieve similar things by filtering for a given host. The way I understand the linked issue is that if you have hundreds of flows in deeply nested routes such as /api/*/*/*/* there is no high-level way to view this structure to understand it and look at sub-routes etc.

ah I see, I've understood that in a different way (much easier ahah). Basically just group the flows (children) by host(parent). I am new to this project and I didn't take into consideration that case!

@lups2000
Copy link
Contributor Author

@Prinzhorn can I use libraries that implement the UI component of a Tree or should I implement the components from scratch?

@mhils
Copy link
Member

mhils commented Feb 26, 2024

I'd be inclined to say that this should be possible without a third-party library.

But more importantly, the flow table is a really core UX element and we shouldn't rush to implement things here. For example, it could also make sense to have this tree in a dedicated sidebar with checkboxes for individual path parts. Have we looked at what other tools do here in terms of UX?

@lups2000
Copy link
Contributor Author

I'd be inclined to say that this should be possible without a third-party library.

But more importantly, the flow table is a really core UX element and we shouldn't rush to implement things here. For example, it could also make sense to have this tree in a dedicated sidebar with checkboxes for individual path parts. Have we looked at what other tools do here in terms of UX?

yes regarding the third-party library it's fine! For the tree, you mean that we could have a tree on one side where we can select the paths we want to display in the table?
Ex.

  • Mitmproxy.com
    - Mitmproxy.com/1
    -Mitmproxy.com/1/1
    - Mitmproxy.com/2
  • example.com
  • google.com

we can select the parent Mitmproxy and display just this subtree in the table? basically acting like a filter. correct ?

@mhils
Copy link
Member

mhils commented Feb 26, 2024

Yes, basically that. I'm not saying it's a better UI/UX, just pointing out there are other options and we should look at existing work. :)

@lups2000
Copy link
Contributor Author

Yes, basically that. I'm not saying it's a better UI/UX, just pointing out there are other options and we should look at existing work. :)

yes yes, you're right. I can define a prototype and then send you the result to get a feedback :)

@mhils
Copy link
Member

mhils commented Feb 26, 2024

My suggestion would be to look at what others are doing and write that up. Before writing a single line of code. 😉

@lups2000
Copy link
Contributor Author

@mhils So far, I've only encountered examples where they provide the possibility to switch between the two modalities through a button. That's also the way I prefer the most! However, from what I've seen, it's quite challenging to reuse some of the table components because they are specifically designed for use with a table. So, if we want to proceed in this manner, we'll need to implement some new components and handle the filters in a different way(this is probably the hardest part). Otherwise, we can move forward with your idea, which seems a bit easier to implement and probably won't require too many changes (we just need to decide where to display this tree with the checkboxes).

@Prinzhorn
Copy link
Member

So far, I've only encountered examples where they provide the possibility to switch between the two modalities through a button.

Could you please share screenshots and name the tools? Because all I've ever seen (e.g. Burp, ZAP, Charles) has this as a sidebar that allows you to filter the table. I can't really imagine how what you describe is supposed to work. The tree can have an arbitrary number of levels and on each level you could have a mix of sub-levels and then also the flows at the current level? How does the UX work? It would also be hard to virtualize this list mix I would assume.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants