Live removal of issue comments using htmx websocket #28958

anbraten · 2024-01-27T14:02:24Z

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:https://localhost:3000/anbraten/test/issues/1"}} to the server to let it know it is interested in updates relevant to this page
another client triggers some action (for now deletes a comment)
the notifier kicks in and the websocket_notifier receives the event
the websocket_notifier checks for all 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

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)

package.json

webpack.config.js

routers/web/websocket/websocket.go

anbraten · 2024-01-27T14:24:56Z

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

lunny · 2024-01-29T01:50:17Z

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

yardenshoham · 2024-01-29T06:58:42Z

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

silverwind · 2024-01-29T11:46:49Z

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

routers/web/websocket/websocket.go

anbraten · 2024-02-03T10:51:18Z

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

lunny · 2024-02-03T11:32:59Z

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 · 2024-03-11T15:04:20Z

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 · 2024-03-11T17:52:01Z

I'm convinced

silverwind · 2024-03-21T16:19:17Z

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 · 2024-03-21T16:36:11Z

And needs a make fmt 😆

…updates

lunny · 2024-04-03T13:25:22Z

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 · 2024-04-03T17:51:23Z

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 · 2024-04-03T18:07:07Z

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 · 2024-04-03T18:12:36Z

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 · 2024-04-05T08:55:01Z

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 · 2024-04-06T14:28:26Z

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.

lunny · 2024-04-07T09:10:46Z

services/websocket/issue_comment_notifier.go

+ return
+ }
+
+ n.pubsub.Publish(ctx, pubsub.Message{


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.

lunny · 2024-04-07T09:13:47Z

services/websocket/websocket.go

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


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

Update issue comments

2b247dd

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 bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 27, 2024

github-actions bot added modifies/frontend modifies/internal labels Jan 27, 2024

anbraten mentioned this pull request Jan 27, 2024

Decide to use htmx or not #28880

Closed

silverwind reviewed Jan 27, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

silverwind reviewed Jan 27, 2024

View reviewed changes

webpack.config.js Outdated Show resolved Hide resolved

move dep

5005245

silverwind reviewed Jan 27, 2024

View reviewed changes

routers/web/websocket/websocket.go Outdated Show resolved Hide resolved

techknowlogick reviewed Jan 28, 2024

View reviewed changes

web_src/js/ws.js Outdated Show resolved Hide resolved

fix htmx ws loading

7cf50ef

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

This comment was marked as resolved.

Sign in to view

Merge remote-tracking branch 'upstream/main' into issue-updates

4947abc

silverwind reviewed Jan 30, 2024

View reviewed changes

routers/web/websocket/websocket.go Outdated Show resolved Hide resolved

add listeners

3b35637

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 added 2 commits February 3, 2024 10:54

Merge branch 'main' into issue-updates

e85f85b

add websocket service

d77073b

anbraten added 2 commits February 3, 2024 14:11

improve websocket service

17f4faf

cleanup

948f1bd

anbraten mentioned this pull request Mar 11, 2024

Add pubsub system for websocket server #29719

Draft

Merge remote-tracking branch 'upstream/main' into issue-updates

0aa3d5e

github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/js modifies/dependencies labels Mar 21, 2024

tidy

40fc078

fmt

c6abd32

silverwind removed the modifies/frontend label Mar 31, 2024

anbraten added 3 commits April 3, 2024 07:04

Merge branch 'main' into issue-updates

e9502e0

Merge remote-tracking branch 'upstream/main' into issue-updates

b8dab69

Merge branch 'issue-updates' of github.com:anbraten/gitea into issue-…

088454f

…updates

anbraten mentioned this pull request Apr 3, 2024

Plans for Gitea 1.23 #30261

Open

13 tasks

add comma

43ad9f5

anbraten added 2 commits April 6, 2024 14:41

Merge remote-tracking branch 'upstream/main' into issue-updates

7cb8e2f

adjust websocket

602a42a

lunny reviewed Apr 7, 2024

View reviewed changes

silverwind mentioned this pull request Apr 16, 2024

Remove jQuery .text() #30506

Merged

Merge remote-tracking branch 'upstream/main' into issue-updates

f83d6fe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Live removal of issue comments using htmx websocket #28958

Live removal of issue comments using htmx websocket #28958

anbraten commented Jan 27, 2024 •

edited

Loading

anbraten commented Jan 27, 2024 •

edited

Loading

lunny commented Jan 29, 2024

yardenshoham commented Jan 29, 2024

This comment was marked as resolved.

silverwind commented Jan 29, 2024 •

edited

Loading

anbraten commented Feb 3, 2024

lunny commented Feb 3, 2024

anbraten commented Mar 11, 2024 •

edited

Loading

yardenshoham commented Mar 11, 2024

silverwind commented Mar 21, 2024 •

edited

Loading

silverwind commented Mar 21, 2024

lunny commented Apr 3, 2024 •

edited

Loading

anbraten commented Apr 3, 2024 •

edited

Loading

silverwind commented Apr 3, 2024 •

edited

Loading

silverwind commented Apr 3, 2024 •

edited

Loading

silverwind commented Apr 5, 2024 •

edited

Loading

anbraten commented Apr 6, 2024 •

edited

Loading

lunny Apr 7, 2024

lunny Apr 7, 2024 •

edited

Loading

Live removal of issue comments using htmx websocket #28958

Are you sure you want to change the base?

Live removal of issue comments using htmx websocket #28958

Conversation

anbraten commented Jan 27, 2024 • edited Loading

How it works:

TODO

anbraten commented Jan 27, 2024 • edited Loading

lunny commented Jan 29, 2024

yardenshoham commented Jan 29, 2024

This comment was marked as resolved.

silverwind commented Jan 29, 2024 • edited Loading

anbraten commented Feb 3, 2024

lunny commented Feb 3, 2024

anbraten commented Mar 11, 2024 • edited Loading

yardenshoham commented Mar 11, 2024

silverwind commented Mar 21, 2024 • edited Loading

silverwind commented Mar 21, 2024

lunny commented Apr 3, 2024 • edited Loading

anbraten commented Apr 3, 2024 • edited Loading

silverwind commented Apr 3, 2024 • edited Loading

silverwind commented Apr 3, 2024 • edited Loading

silverwind commented Apr 5, 2024 • edited Loading

anbraten commented Apr 6, 2024 • edited Loading

lunny Apr 7, 2024

Choose a reason for hiding this comment

lunny Apr 7, 2024 • edited Loading

Choose a reason for hiding this comment

anbraten commented Jan 27, 2024 •

edited

Loading

anbraten commented Jan 27, 2024 •

edited

Loading

silverwind commented Jan 29, 2024 •

edited

Loading

anbraten commented Mar 11, 2024 •

edited

Loading

silverwind commented Mar 21, 2024 •

edited

Loading

lunny commented Apr 3, 2024 •

edited

Loading

anbraten commented Apr 3, 2024 •

edited

Loading

silverwind commented Apr 3, 2024 •

edited

Loading

silverwind commented Apr 3, 2024 •

edited

Loading

silverwind commented Apr 5, 2024 •

edited

Loading

anbraten commented Apr 6, 2024 •

edited

Loading

lunny Apr 7, 2024 •

edited

Loading