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

feat(#593): Show real response in image preview #622

Merged
merged 3 commits into from
Oct 19, 2023

Conversation

Its-treason
Copy link
Member

@Its-treason Its-treason commented Oct 16, 2023

Fixes: #609, #593 & #519

Now the original body is encoded as Base64 and send to the browser. This allows us to render the original Image response as a preview.

I also refactored the QueryResult-component into to separate components, this makes extending it with more preview modes easier.

This PR does change quite a lot, so I would appreciate anyone testing it locally to check if there are any issues.


Now that we have the original response, we can start working on other issues, like: #539 and implement other preview modes.

@@ -296,6 +296,14 @@ const registerNetworkIpc = (mainWindow) => {
/** @type {import('axios').AxiosResponse} */
const response = await axiosInstance(request);

const dataBuffer = Buffer.from(response.data);
// Overwrite the original data for backwards compatability
response.data = dataBuffer.toString('utf-8');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using charset from "Content-Type" with a fallback to UTF-8? Something like this:

      const content_type = response.headers['content-type'];
      // RegExp magic from https://stackoverflow.com/a/33192813
      let charset = new RegExp('/charset=([^()<>@,;:\"/[\]?.=\s]*)/i').exec(content_type);
      if (charset == null) {
        charset = "utf-8"

@martinsefcik
Copy link
Contributor

martinsefcik commented Oct 17, 2023

@Its-treason Great!

One small note. Can we use raw response body size now for displaying in UI in case content-length header is missing, because I think the fix implemented in #520 was I would say just temporary workaround which does not show real size of transferred body.

And maybe also in cases if content-length is present I would prefer raw body size to be displayed.

@Sl-Alex
Copy link
Contributor

Sl-Alex commented Oct 17, 2023

@Its-treason I like the way you did it. I tried to do the same before raising #609, and it even worked, but your solution is definitely better.
I have only one general comment: do we need more or less the same for CLI?

@Its-treason
Copy link
Member Author

Its-treason commented Oct 17, 2023

Hey everyone, thank you for feedback! Really appreciate it!

I update my PR: I noticed that runners were broken, because I need to convert response.data from the buffer to a string/object.

What about using charset from "Content-Type" with a fallback to UTF-8? Something like this:

      const content_type = response.headers['content-type'];
      // RegExp magic from https://stackoverflow.com/a/33192813
      let charset = new RegExp('/charset=([^()<>@,;:\"/[\]?.=\s]*)/i').exec(content_type);
      if (charset == null) {
        charset = "utf-8"

That sounds like a very good idea, I implemented it!

@Its-treason Great!

One small note. Can we use raw response body size now for displaying in UI in case content-length header is missing, because I think the fix implemented in #520 was I would say just temporary workaround which does not show real size of transferred body.

And maybe also in cases if content-length is present I would prefer raw body size to be displayed.

Also didn't think of this at first, also implemented it!

@Its-treason I like the way you did it. I tried to do the same before raising #609, and it even worked, but your solution is definitely better. I have only one general comment: do we need more or less the same for CLI?

Yes, we could also add this in CLI, but It's currently not needed because: CLI has its own code for request preparation and the buffer is not accessible to tests and assertions in the app.

We should probably change this with: #226

@Its-treason Its-treason force-pushed the feature/better-image-preview branch from eb38fb4 to b3ee0af Compare October 17, 2023 21:04
@helloanoop
Copy link
Contributor

@Its-treason I am going to review this today and get it merged.

Is this PR Good to Merge from your side ?

@helloanoop helloanoop added this to the v1 milestone Oct 18, 2023
@Its-treason
Copy link
Member Author

@Its-treason I am going to review this today and get it merged.

Is this PR Good to Merge from your side ?

Sorry for the late reply, I was still testing this during work with a “production” environment. Everything was working fine, so It's ready to merge!

@helloanoop helloanoop merged commit 922c1ff into usebruno:main Oct 19, 2023
2 checks passed
@helloanoop
Copy link
Contributor

Fantastic, @Its-treason !! Many Thanks.

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.

[feature] Use raw response data instead of the default output from axios
4 participants