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.

Sign up for GitHub

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

Jump to bottom

Logs over websockets #794

Merged
merged 9 commits into from
Mar 29, 2023
Merged

Logs over websockets #794

merged 9 commits into from
Mar 29, 2023

Conversation

novemberkilo
Copy link
Contributor

@novemberkilo novemberkilo commented Mar 22, 2023 •
edited
Loading

This PR changes a few things.

  1. It introduces the machinery for jsoo into the project
  2. A new route is introduced that provides a websocket connection for log data
  3. The logic for constructing the appropriate markup for streaming log data is moved over to javascript by way of jsoo
  4. A websockets client is also introduced in jsoo to consume the log data and present it within the step page
  5. Following https://css-tricks.com/books/greatest-css-tricks/pin-scrolling-to-bottom/ once we scroll to the bottom of the page, the scrolling stays pinned there and there is no need to scroll further as logs continue to appear

Along the way I found a bug in Brr that has resulted in us having to hand roll percent-encoding of some things (so that the parentheses that show up in variants don't get lost in the routing). I will file an issue on Brr accordingly.

MisterDA
MisterDA reviewed Mar 22, 2023
View reviewed changes
Copy link
Contributor

@MisterDA MisterDA left a comment

Choose a reason for hiding this comment

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

Looking great! That will definitively be an improvement.

web-ui/static/js/step-page-websocket-client.js Outdated Show resolved Hide resolved
web-ui/controller/git_forge.ml Outdated Show resolved Hide resolved
patricoferris
patricoferris reviewed Mar 22, 2023
View reviewed changes
Copy link
Contributor

@patricoferris patricoferris left a comment

Choose a reason for hiding this comment

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

A few comments on the Brr code :))

web-ui/jsoo/main.ml Outdated Show resolved Hide resolved
web-ui/jsoo/main.ml Outdated Show resolved Hide resolved
benmandrew
benmandrew suggested changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@benmandrew benmandrew left a comment

Choose a reason for hiding this comment

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

Thanks for this! One note is that on Safari the overflow-anchor property is not supported. It has no effect so it's good to keep it in for the other browsers.

web-ui/jsoo/main.ml Outdated Show resolved Hide resolved
maiste
maiste reviewed Mar 23, 2023
View reviewed changes
web-ui/jsoo/main.ml Outdated Show resolved Hide resolved
web-ui/router.ml Outdated Show resolved Hide resolved
web-ui/static/js/step-page-websocket-client.js Outdated Show resolved Hide resolved
web-ui/view/step.mli Outdated
@@ -32,5 +32,6 @@ module Make : functor (_ : Git_forge_intf.Forge) -> sig
can_cancel:bool ->
?flash_messages:(string * string) list ->
string * int64 ->
Dream.response Lwt.t
string
(* Dream.response Lwt.t *)
Copy link
Member

Choose a reason for hiding this comment

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

Why does this have to be a string and not a Dream.response?

@maiste maiste added type/enhancement New feature or request type/ui UI related feature/bug labels Mar 23, 2023
benmandrew
benmandrew approved these changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@benmandrew benmandrew left a comment

Choose a reason for hiding this comment

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

Renders well for me now, thanks!

tmcgilchrist
tmcgilchrist approved these changes Mar 29, 2023
View reviewed changes
Copy link
Member

@tmcgilchrist tmcgilchrist left a comment

Choose a reason for hiding this comment

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

Looks good.

A few tidy up comments required.

dune-project Outdated Show resolved Hide resolved
web-ui/jsoo/dune Outdated Show resolved Hide resolved
web-ui/jsoo/main.ml Outdated Show resolved Hide resolved
web-ui/jsoo/main.ml Outdated Show resolved Hide resolved
web-ui/view/git_forge_intf.ml Outdated Show resolved Hide resolved
web-ui/view/step.ml Outdated Show resolved Hide resolved
@novemberkilo novemberkilo merged commit 84a767b into ocurrent:master Mar 29, 2023
novemberkilo added a commit that referenced this pull request Mar 29, 2023
@novemberkilo
Revert "Logs over websockets (#794)"
06bb80a 
This reverts commit 84a767b.
novemberkilo added a commit to novemberkilo/ocaml-ci that referenced this pull request Apr 3, 2023
@novemberkilo
Revert "Revert "Logs over websockets (ocurrent#794)""
224d24d 
This reverts commit 06bb80a.
novemberkilo added a commit to novemberkilo/ocaml-ci that referenced this pull request Apr 3, 2023
@novemberkilo
Revert "Revert "Logs over websockets (ocurrent#794)""
7ba6749 
This reverts commit 06bb80a.
This was referenced Apr 3, 2023
Merged
Open
novemberkilo added a commit to novemberkilo/ocaml-ci that referenced this pull request Apr 4, 2023
@novemberkilo
Revert "Revert "Logs over websockets (ocurrent#794)""
e660bf1 
This reverts commit 06bb80a.
novemberkilo added a commit that referenced this pull request Apr 4, 2023
@novemberkilo @tmcgilchrist
Add step-logs.js to crunch. (#799)
cf3930c 
* Revert "Revert "Logs over websockets (#794)""

This reverts commit 06bb80a.

* Update crunch rules to bring in step-logs.js

* Updates shas in Dockerfile.web to latest and drop dream pins.

* Cleanup following review comments.

* Update dune-project

Co-authored-by: Tim McGilchrist <[email protected]>

---------

Co-authored-by: Tim McGilchrist <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maiste maiste maiste left review comments

@patricoferris patricoferris patricoferris left review comments

@MisterDA MisterDA MisterDA left review comments

@benmandrew benmandrew benmandrew approved these changes

@tmcgilchrist tmcgilchrist tmcgilchrist approved these changes

Assignees
No one assigned
Labels
type/enhancement New feature or request type/ui UI related feature/bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@novemberkilo @tmcgilchrist @MisterDA @benmandrew @patricoferris @maiste