Refresh button when branch changes

[maxbol]

Fixes #18427

Had a quick go at the problem. Events are sent at the end of hook_post_receive.go, to all users returned by access_model.GetRepoReaders() for the updated repo. The FE event handler filters incoming events by owner, repo and branch. Both changes to the base and head target triggers the reveal of the refresh button.

Working good as far as I can tell, some more polish might be needed.

Questions:

• Should there be a polling fallback for this behavior if EventSource is not available in the browser?
• What color and position should be used for the button?
• Is the behavior displayed exactly the one discussed in the original issue, or did I miss something?
• Are all service workers supposed to use notification-worker for their name? Seemed to be the case in both the currently existing ones, so I followed that example... not sure if it makes sense or not.

Please give me your feedback!

EDIT: Removed WIP label

[silverwind]

Could it be that we are generally doing SharedWorker wrong by creating a separate one for each event type? I think there should likely be only one worker thread that handles all events and JS should distinguish these events by a type property in the sent data.

[maxbol]

Could it be that we are generally doing SharedWorker wrong by creating a separate one for each event type? I think there should likely be only one worker thread that handles all events and JS should distinguish these events by a type property in the sent data.

This was my thought as well.

EDIT: And the type property is already there, so it should enough to instantiate the SharedWorker as a singleton in a separate file and import it where it's needed.

[maxbol]

Refactored the use of SharedWorker per @silverwind s suggestion:

1d71ee6

Gonna go ahead and remove the WIP label from this, as I think it successfully addresses the original feature request. If there is anything missing from this solution, please let me know!

[silverwind]

@zeripath what should we do with this in regards to #20543?

[zeripath]

First off thanks for the PR.

But as far as I can see the current code would inform any logged user of updates to any branch in any repo. This is a huge security hole. At the very least you need to check if the repo can be seen by the user before you push it up their eventstream but its still a huge amount of events that are not needed and this is why I think this problem needs a different approach using websocket to register which repo and branch you're actually interested in.

I'm not sure I understand the restructuring that has occurred in the JS (I'm on my phone) but it looks to me like it's still using the eventstream that was originally there.

[silverwind]

I too think we should probably just move everything to websocket, completely removing EventSource.

[zeripath]

Well if you can figure out why the Dom isn't updating in my WIP PR we can get going with that but right now although I have a working websocket that is producing the eventstream events and thus could replace the event source, the events won't update the webpage.

[maxbol]

But as far as I can see the current code would inform any logged user of updates to any branch in any repo.

Maybe I've misunderstood some part of the access model, but isn't this restricted by access_model.GetRepoReaders() here:

https://github.com/go-gitea/gitea/pull/20511/files#diff-4e06ddb19de4d68ed031e1381dc794902f732ae8a731192d00b5869b311a1601R53

It should literally only send events to users who have read access to the repo, no?

[maxbol]

With regards to websockets vs SSE, I have no horse in that race - just responding to the original feature request and bounty, which clearly separated the issues: #18427 (comment)

Although I'm not entirely certain what pros websockets (a bidirectional, stateful stream) have over SSE here. SSE seems to generally fit the pattern of an original, client-initiated request to setup a unidirectional flow from server to client better. And HTTP/2 makes WS obsolete. Maybe just refactor the EventSource module somewhat server side so you can use other targets than uids (like for instance repos, refnames or orgs). Authorization could then be handled on subscription, when the client initiates the request, or something like that.

[maxbol]

This was my thought as well. A shared singleton might make more sense and cut down on the number of concurrent TCP connections. tors 28 juli 2022 kl. 19:11 skrev silverwind ***@***.***>:

…

Could it be that we are generally doing SharedWorker wrong by creating a separate one for each event type? I think there should likely be only one that handles all events and distinguish these events by a type property in the sent data. — Reply to this email directly, view it on GitHub <#20511 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAI7Q7LUVFJZBR6CRQH5WDTVWK5LHANCNFSM5422R2MQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[lunny]

SendMessage via event source will not always work within multiple replication Gitea system. So I think we need a redis based abstract layer to do that. Let's move this to next release to think more.

[silverwind]

SendMessage via event source will not always work within multiple replication Gitea system. So I think we need a redis based abstract layer to do that. Let's move this to next release to think more.

What could be done is have all servers being SUBSCRIBEd to a certain redis/kafka/etc channel, and the server that wants to send the message PUBLISHes it to the channel, after which all servers including the sender receive the message and send it on to all their connected and/or affected clients.

I think such an approach is universal for server-to-client communication in distributed setups. Ideally this would be done via websockets, thought.

[lafriks]

Would be good if such subscription/event model would be made abstract like we have with queues so that multiple types of subscription providers can be implemented as at least both internal (default) and redis could be used for this.