Use JsonTree in network detail view#478
Conversation
Summary of ChangesHello @snappdevelopment, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience in the network detail view by introducing an interactive JSON tree viewer for response bodies. This change provides a more user-friendly way to inspect JSON data, offering syntax highlighting and collapsible sections. Furthermore, the threshold for displaying large response bodies has been substantially increased, reducing the need for manual override for moderately sized responses and improving the default display behavior. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a great enhancement, integrating JsonTree to provide a much better experience for inspecting JSON responses in the network detail view. The implementation is solid, including a fallback to a plain text view for non-JSON content or in case of parsing errors. I've found one high-severity issue regarding state management in Jetpack Compose that could lead to incorrect UI behavior when switching between different network requests. Addressing this will make the feature robust.
| } | ||
| if (!displayBody) { | ||
| Column( | ||
| var jsonError by remember { mutableStateOf(false) } |
There was a problem hiding this comment.
The jsonError state should be reset when the response changes. Currently, if an invalid JSON response sets jsonError to true, it will persist even when viewing a subsequent valid JSON response, preventing FloconJsonTree from being displayed. To fix this, you should key the remember call to response.body.
| var jsonError by remember { mutableStateOf(false) } | |
| var jsonError by remember(response.body) { mutableStateOf(false) } |
There was a problem hiding this comment.
Added that change
|
@snappdevelopment Other things, since you are the author of JsonTree. It it possible to add a system to customize highlight ? To make a diff between 2 jsons ? A bit like a PR review on GitHub |
Interesting idea. What would be your use case for this? And how do you image it to look like? |
|
@snappdevelopment I imaging it like GitHub side by side in PR Review, we see line highlighted and word to see difference. It would be useful to compare Json between 2 requests |
|
@rteyssandier I see. I thought about this over the weekend a bit. I will try to build something and let you guys know if it works. We can move this topic to the discussion section if you want to unblock this MR, or I will just open a MR at some point. |
|
Yes totally, #490 |
Flocon is already using JsonTree when opening a response in a separate window. I think it is also useful to have syntax highlighting and expand/collapse functionality directly in the network detail view, so I added JsonTree there as well.
With my change, JsonTree is used to render response bodies and only if an error occurs the plain text fallback is used (which still applies a length limit like before).
JsonTree uses a LazyColumn internally so it needs a fixed height (I gave it 600.dp) to make it work in a scrollable container like the network detail view.
I saw this issue so I tested some large responses with JsonTree and with the plain text view.