Live removal of issue comments using htmx websocket

[anbraten]

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

Concept

By using htmx we use a websocket to send dom elements matched by #id to clients to update specific parts of the UI.

Some examples where this could be used

• Issue comment updates: A dom element of the issue comment is send when the content was changed or a reaction was added etc. The client would just replace that element and the new comment would be visible.
• User got a new notification: We render the template for the notification bell & counter and send it to the user. In addition we send a list item for the notification page if the user is currently on that page.
• Issue comment removed: We send a special empty dom element that removes a comment from the issue page.

[!NOTE]
As rendering comments was changing a few templates I started implementing the concept for live updates for removing a comment for now to focus on the websocket logic. Other updating logic will be straightforward to add > in followup PRs.

How it works in detail:

• 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 and checks for all locally connected sessions:
  • if their url is relevant for the issue comment and
  • 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

Screencast from 2024-02-16 17-50-20.webm

Using a pub/sub system the notifier events are also sent to other servers which forward them again to all relevant & locally connected clients:

TODO

• [x] listen to events like comment adding (with permission handling)
• [x] send new issue comments templates through websocket
• [x] 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)

websocket logic based on #26679 and #27806

[anbraten]

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.

[lunny]

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

[yardenshoham]

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]

Was thinking about the best approach for updating UI via events for some time already. Lets take the issue page as an example for what could be updated in the long term:

• meta data (title, description, labels, milestone, ...)
• adding comments
• updating comments
• removing comments
• reactions for the issue & comments

So there would be a few solutions for it looking at the used libs / frameworks in this project:

• Vue:
  • How: In case of the issue page this would probably be implemented using a component (or multiple nested ones) for the whole issue page. A websocket would be opened by that component and json updates would be received to update related component details.
  • Pros: Would allow a highly interactive UI, that could show changes before even sent to the server.
  • Cons: the issue templates would need to be defined for Vue and go or just for Vue loosing server-side-rendering. Using the same websocket across multiple components would require some store etc otherwise each UI part would need its own websocket (like the issue page & the notifications counter in the header)
• htmx:
  • How: like this PR
  • Pros: allows reusing go templates, allows to simply re-use the same websocket for all UI updates
  • Cons: special websocket needs to be used sending html that is only useful for htmx and not for external clients, not sure how remvoing an element would work
• vanilla JS / jQuery:
  • How: Websocket would be opened, events would be parsed and UI updated using DOM functions accordingly
  • Pros: 🤔
  • Cons: A lot of custom code would probably be required and therefore reinventing the wheel

(Just a disclaimer: this was my first time using htmx as I am mainly using Vue in all other projects)

I agree with lunny, this is more complex than I imagined

Honestly I think this is mainly as it is sth totally new to add live updates logic. The htmx websocket part is below 10 lines of code. So doing the same in Vue / vanilla JS / jQuery would probably result in a similar or higher amount of code.

Do we really have 2 way communication here though? Maybe SSE fits better for this use case

SSE have some limitations compared to websockets. Sending data to the server could be nicely used to subscribe to specific updates for example as suggested in this PR. By sending sth like the url the client could subscribe to all changes of an issue. Using SSE we would probably need to open multiple connections which in general results into more keep-alive traffic.

[silverwind]

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

[anbraten]

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

[lunny]

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.

[anbraten]

I tried extracting the template for a single comment from comments.tmpl:

{{range .Issue.Comments}}
	{{if call $.ShouldShowCommentType .Type}}
		{{if and (eq .Type 29) (or (gt .CommitsNum 0) .IsForcePush)}}
			<!-- If PR is closed, the comments whose type is CommentTypePullRequestPush(29) after latestCloseCommentID won't be rendered. //-->
			{{if and .Issue.IsClosed (gt .ID $.LatestCloseCommentID)}}
				{{continue}}
			{{end}}
		{{end}}

		{{template "repo/issue/view_content/comment" .}}
	{{end}}
{{end}}

But somehow I seem to have missed sth as its not aware of $.:

Any ideas what I might be missing?

[lunny]

I tried extracting the template for a single comment from comments.tmpl:

{{range .Issue.Comments}}
	{{if call $.ShouldShowCommentType .Type}}
		{{if and (eq .Type 29) (or (gt .CommitsNum 0) .IsForcePush)}}
			<!-- If PR is closed, the comments whose type is CommentTypePullRequestPush(29) after latestCloseCommentID won't be rendered. //-->
			{{if and .Issue.IsClosed (gt .ID $.LatestCloseCommentID)}}
				{{continue}}
			{{end}}
		{{end}}

		{{template "repo/issue/view_content/comment" .}}
	{{end}}
{{end}}

But somehow I seem to have missed sth as its not aware of $.:

Any ideas what I might be missing?

Use {{ctx}} instead of {{$.Context}}.

[anbraten]

I removed the part to render a comment for now as it was quite some code and only left the part to delete a comment on the according event so this PR can focus on getting the websocket part added and in a follow-up PRs it will be easier to just add the corresponding events (comment added, comment updated, label updated, ...).

[lunny]

per https://github.com/go-gitea/gitea/pull/26679#issuecomment-1689626654, it will not work for multiple nodes Gitea because melody keeps the session on memory but not redis.

[anbraten]

per https://github.com/go-gitea/gitea/pull/26679#issuecomment-1689626654, it will not work for multiple nodes Gitea because melody keeps the session on memory but not redis.

Having the session on mem should be fine. What's missing currently is that the notifier implementation of gitea is a single node implementation (fine for sth like sending an email as you just want one email to be sent). For the future to support multinode live updates it will be necessary to share the events (sth like comment deleted) locally, but also broadcast them via sth like a redis channel to all other nodes.

Would like to add this later on in another PR as currently the UI update would just be skipped resulting in the same result as before this PR where you had to reload the page to see that for example a comment was removed.

[silverwind]

I think single-instance support is fine intially. I previously proposed a notification mechanism that works in multi-instance as well:

• The triggering node (e.g. the one that did the database change) publishes the message to redis as JSON, containing username and all data relevant for the notification
• All nodes subscribe to messages
• On message receival, all nodes on which username has active websockets broadcast the message to all those websockets. melody makes this trivial with its broadcast function.

[anbraten]

Is there anything else I should adjust?

[silverwind]

Running into this JS error in Firefox currently when testing the branch:

[silverwind]

Trying to fix the JS...

[lunny]

I still don't think this is ready to be merged. My 2 points.

• I think this is a complicated place which should not be implemented with htmx since we have said htmx should be kept in a simple scenario
• The backend sub/pub system is memory only and melody has no possible to be extented we have to rewrite it in the future.

[silverwind]

htmx will not be the final websocket solution, that will be in custom JS code in a SharedWorker. But I do think it's important we get the backend side of the websocket up and running now so that it can be improved upon, ideally during the 1.23 cycle.

[anbraten]

I think this is a complicated place which should not be implemented with htmx since we have said htmx should be kept in a simple scenario

I had a look at different options, but let me explain why I finally decided to give htmx a try in this case. From the used frameworks I see 3 options how UI updates could be done in the frontend in general:

• Vanilla JS go-template: Browser receives the result of a rendered go template from the backend via websocket and uses vanilla JS functions to update the dom
  • Pros: re-using go-templates would require less refactoring
  • Cons: custom JS helpers would be needed to updated UI elements in DOM (shouldn't be to hard to implement, but seems to be what htmx is actually for?), Some go-templates need to be refactored to allow them to be rendered for a smaller context (example: the issue comments template is currently only available for all comments at once and not for single issue)
• Vanilla JS JSON: Browser receives some JSON from the backend and updates the DOM manually by writing specific handlers for all different UI elements
  • Pros: If done correctly could be doing some super smart updates of just the specific dom elements instead of replacing a complete area
  • Cons: Would be a complete mess to implement for each respective element IMO, basically the reason why frameworks like vue / react render based on a declarative template approach
• Vue: Components that should be updatable are converted to Vue and JSON send over the websocket updates the Vue data and therefore rerenders the dom
  • Pros: Vue components could smartly render UI and would easily be updated on data changes
  • Cons: A lot of UI elements would need to be converted from go templates to Vue components, Vue components would not be server rendered and therefore bad seo & experience with disabled JS browsers
• htmx: The page stays as it is, in the backend updated UI elements are generated using go templates and send via websocket to the browser which updates elements with a matching id
  • Pros: similar to the vanilla JS go-template approach (just a lib for updating dom elements)
  • Cons: same as for the vanilla JS go-template approach

The backend sub/pub system is memory only and melody has no possible to be extented we have to rewrite it in the future.

Sure this is currently the case, however this could be done as shown in #29719 by adding some pubsub interface that sends the updates from code.gitea.io/gitea/services/notify via a pubsub channel, so other servers could forward it to their clients as well.

[yardenshoham]

I'm convinced

[silverwind]

Needs a make tidy.

PS: I need to read through above but I'm generally okay with everything to finally get this websocket in so we can also start removing the EventSource, which is rather buggy currently. Also, I would like to attempt the SharedWorker work after this.

[silverwind]

And needs a make fmt 😆

[lunny]

This is not a high-efficiency implementation to iterate all the WebSocket sessions. Maybe the best way is to use Pubsub. So when a WebSocket client connects to the server, it will send his topic(maybe the URL could be the topic), when the client disconnects, the subscription should be removed. When an event is fired, the topic subscriters will receive the notifications.

To implement that, I think maybe we can remove melody and filterSessions because they will iterate the whole websocket sessions. For a big site, it may be big. I like your pubsub implementation in #29719. And maybe we can also have a redis based implementation. So how about merge the two PRs into this one and remove melody? Removing melody is not a MUST but we should not depend on it's sessions.

[anbraten]

Sure, I can give it a try.

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?

How should we implement the "rendering" part. As we want to send the template for the specific user connected (for example to show the issue comment author / owner badges) should I pass and use the user inside the subscription callback function and render the content in there?

[silverwind]

when the client disconnects, the subscription should be removed

This will work for main thread websocket that reconnects on every page load, but once we move it to SharedWorker, it could be common that user has multiple tabs open and the SharedWorker would only disconnect when the last tab is closed. So for that scenario, a unsubscribe message would also needed.

[silverwind]

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]

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]

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.