jsdelivr / globalping-cli

A simple CLI tool to run networking commands remotely from hundreds of globally distributed servers
Mozilla Public License 2.0
129 stars 14 forks source link

Duplicating HTML output #60

Closed jimaek closed 1 year ago

jimaek commented 1 year ago

I caught it on camera only once, in the beginning, you can ignore the rest of the video https://dl.dropboxusercontent.com/s/y73yppnqyi7yyzz/WindowsTerminal_2023-05-01_12-39-32.mp4

jimaek commented 1 year ago

Not sure if related but now I am getting this issue. header, metadata, header

# go run main.go http https://www.jsdelivr.com --method GET
X-Cached-Since: 2023-05-09T16:31:34+00:00
X-ID: sum-up-gc94
> AS, IN, Vellanur, ASN:36236, NetActuate, Inc
HTTP/1.1 200
Server: nginx
Date: Tue, 09 May 2023 16:33:14 GMT
Content-Type: text/html; charset=utf-8

Its also possible that the original issue was fixed on the API side, but needs more testing. But this issue is happening right now after the API deploy

MartinKolarik commented 1 year ago

Ok I can reproduce this issue now as well with @jimaek's test params:

image

MartinKolarik commented 1 year ago

Looks like part of the issue here might be that we're printing rawOutput for in-progress instead of rawBody, which would lead to this messed up output. That should be fixed here.

A second issue is we don't actually have the rawBody field while the specific probe is in progress, that's up to the API to possibly improve (but the CLI should NOT use the rawOutput for the GET case regardless).

didil commented 1 year ago

in this branch https://github.com/jsdelivr/globalping-cli/pull/66:

MartinKolarik commented 1 year ago

Interestingly enough, I can't reproduce the issue with other targets, e.g. https://www.google.com or https://cdn.jsdelivr.net/npm/jquery so it could be somehow related to the number of lines in the output or some other property of the response structure.

MartinKolarik commented 1 year ago

Also, the issue is reproducible even with the v1.0.1 release (but only with the www.jsdelivr.com target) so it isn't related to any of the recent changes.

MarvinJWendt commented 1 year ago

Hi, creator of PTerm here.

After investigating the issue, I found that we had actually looked into this behavior about a year ago, but it had slipped my mind. Unfortunately, the content area cannot exceed the size of the terminal, and updating lines that aren't visible is not possible since the cursor cannot move outside the current viewing area. This is a limitation of how terminals work. So the content that is above the current view, would stay, while the content in the view will be replaced with the new content. However, if contentHeight < terminalHeight, then everything should function as expected.

We could potentially implement a View in PTerm that functions similarly to a textbox and responds to user scrolls, but this would require some time to develop. Alternatively, if the content doesn't need to be updated dynamically, it can be printed without the area. Another approach would be to create an alternative screen (virtual temporary terminal) using fmt.Print("\x1b[?1049h"). You could then print the content, clear the screen, and print the content again, to update it. However, it's important to keep in mind that alternative screens don't work with every terminal. Once you're finished with the alternative screen, you can close it by using fmt.Print("\x1b[?1049l"). This will clear everything on the screen and return to the original terminal. But I don't think that this workaround would look very nice.

Another option would be to use a package that already has dynamic textboxes (similar behavior to nano and vim). I personally like tview, it's also the interface library used by K9s. Their TextView could be used for this specific use case: https://github.com/rivo/tview/blob/51ba3688bcdd82bcc076bf59a55f561eaf586c9d/textview.go#L77 Tview can also be combined with PTerm, so you could use PTerms styling functions to output text into the TextView.

MartinKolarik commented 1 year ago

Thanks for the detailed response @MarvinJWendt!