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

Live removal of issue comments using htmx websocket #28958

Open
wants to merge 82 commits into
base: main
Choose a base branch
from

Conversation

anbraten
Copy link
Contributor

@anbraten anbraten commented Jan 27, 2024

First step into "realtime" updating UI using htmx with ws extension as discussed in #2287 and #25661

As rendering comments was quite tough (a lot of code changes) I just implemented the updating UI part for removing a comment for now as a reference. All other events will be quite simple to add from now on and only require some rendering logic to be adjusted.

htxm logic based on #28908
websocket logic based on #26679 and #27806

Screencast.from.2024-02-16.17-50-20.webm

How it works:

  • a tab / client connects via websocket to the server /-/ws
  • the client sends on dom.load: { action: "subscrib", data: { url: "http://localhost:3000/anbraten/test/issues/1"}} to the server to let it know it is interested in updates relevant for this page
  • another client triggers some action (for now deletes a comment)
  • the notifier kicks in and sends the event as json via the pubsub system
  • the websocket_notifier gets the event from pubsub
  • the websocket_notifier checks for all locally connected sessions if their url is relevant for the issue comment and checks if the user of that session is allowed to receive that notification data
  • the websocket_notifier sends the updated dom element (matched based on #id) via websocket to all previously filtered client sessions

image

TODO

  • listen to events like comment adding (with permission handling)
  • send new issue comments templates through websocket
  • send sth like url from client to server via websocket to filter necessary events (the url could be used to detect that only updates for a specific issue and general updates like new notifications are relevant)

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 27, 2024
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 27, 2024
package.json Outdated Show resolved Hide resolved
webpack.config.js Outdated Show resolved Hide resolved
@anbraten
Copy link
Contributor Author

anbraten commented Jan 27, 2024

I will wait with further development until #28880 has a final decision for now. Just wanted to do this POC to see and show the team if it could be used for the discussed realtime topics as well.

web_src/js/ws.js Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 28, 2024
@lunny
Copy link
Member

lunny commented Jan 29, 2024

Wouldn't we say htmx will be used on simple situation? I think this page is very complex.

@yardenshoham
Copy link
Member

I agree with lunny, this is more complex than I imagined but hey, if it works that's awesome. Do we really have 2 way communication here though? Maybe SSE fits better for this use case

@anbraten

This comment was marked as resolved.

@silverwind
Copy link
Member

silverwind commented Jan 29, 2024

I agree with lunny, this is more complex than I imagined but hey, if it works that's awesome. Do we really have 2 way communication here though? Maybe SSE fits better for this use case

We need a Websocket in any case as SSE is too limiting:

  • max. 6 connections limitation per browser is a big problem when opening many tabs
  • no 2-way communication
  • SSE is seldomly used and therefor likely buggy depending on browser, websocket is a more stable and well-supported API

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 3, 2024
@anbraten
Copy link
Contributor Author

anbraten commented Feb 3, 2024

Is it fine to hook into services/notify to get updates about changes like the added comments?

@lunny
Copy link
Member

lunny commented Feb 3, 2024

Is it fine to hook into services/notify to get updates about changes like the added comments?

I think it should be. You need to implemente a new notifier like notification.

@silverwind
Copy link
Member

silverwind commented Apr 3, 2024

When using the url as topic, I would need to publish to all urls a specific event could possibly be interesting for. So ie an issue name change would be interesting for the issue page, but also for the issue list page and the issue list page of an org, right?

Maybe per-repo will be fine for a start, e.g. key repo:owner/name and server would then publish all changes to the repo. We don't have to over-complicate it at this stage, imho.

@silverwind
Copy link
Member

silverwind commented Apr 5, 2024

BTW if we keep melody, we would likely need to increase it's MaxMessageSize as the default is only 512 as I recently discovered. I don't know why it has that so low, other libs I used set 100MB default 😆.

@anbraten
Copy link
Contributor Author

anbraten commented Apr 6, 2024

Okay, I adjusted the code a bit so it sends messages through pubsub and to use subscriptions instead of the for loop iterating the sessions.

I currently see two options how the initial notifier event could be brought to a websocket client. What do you think?

  • Generate the templates per user, send it via pubsub to a topic like <user-id>:<repo-owner>/<repo-name> and forward the message to websocket client. Allows us to avoid marshalling the necessary data for the pub-sub message, however it would be tricky to know if we even need to generate the template as we don't know it a specific user is listening a topic.

  • Sending the notifier data as json via pubsub, inside the pubsub subscription handler of a client check the permissions of that user, generate the template and send it via websocket. I think marshalling the notifier data could be a bit tricky.

return
}

n.pubsub.Publish(ctx, pubsub.Message{
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the topic should still contain the issue and index. And it also includes the actions like delete/create/update. Because I think different pages of the same clients should not share the same web socket client.

}

ctx := goContext.Background() // TODO: use proper context
h.pubsub.Subscribe(ctx, msgData.Repo, func(msg pubsub.Message) {
Copy link
Member

@lunny lunny Apr 7, 2024

Choose a reason for hiding this comment

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

When an anonymous user are staying on an issue view page, shouldn't the page will be automatically updated?

@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/dependencies modifies/frontend modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants