Improve notifications

[biosfood]

Description

Streamline the user experience by making the inbox counter more responsive.

Fixes #1744

Right now, lemmy sends a cache header for some frequently queried data, like api/v3/user/unread_count or /api/v3/private_message/list of 60s.

When updating a message to be displayed as read, I would ideally like to just refresh the notification counter, but get old cached data from the browser.

@Nutomic @dessalines Is this caching behaviour intended or should it be changed on the backend? If a client is acting correctly, it should just avoid sending a request more than every minute but shortly after making changes, I think it would be wise to allow it to update.

An alternative might be to just update the view on the client and purely hope the server registers the changes but a user might find it useful to get immediate feedback of the new data that is stored on the server (to avoid desync of client and server).

[dessalines]

A much simpler solution would be to just refetch those counts after a message is marked as read / unread.

Those routes shouldn't be using a 60s cache, I thought that was only in use for public / non-logged-in users?

[biosfood]

A much simpler solution would be to just refetch those counts after a message is marked as read / unread.

That is exactly what I was trying to do. However, the response headers for api/v3/user/unread_count and also the private message query and some more have this:

cache-control: public, max-age=60

Because the client already fetches the unread count every 30s, the cache present and I get back the old data.

Edit: found the code you were refering to earlier, somehow the check for weather a jwt was sent fails at just the cache part

@dessalines

I think I found out what goes wrong here:
https://github.com/LemmyNet/lemmy/blob/6735a98d35e293c97d56c1bba6636f9bfcb78fae/src/session_middleware.rs#L71 is where the check you mentions happens. It (to me) seems to just look for a request HEADER "auth" or a COOKIE "auth".

However, the lemmy-ui client and the respective request on the server expect a field "auth" either in the supplied JSON for POST requests or the URL parameters (query string). example: auth field in https://github.com/LemmyNet/lemmy/blob/6735a98d35e293c97d56c1bba6636f9bfcb78fae/crates/api_common/src/post.rs#L179.

Currently, lemmy doesn't seem to insert any cookies and the js client also doesn't insert any request header fields.

Let me know if I should open a PR on the lemmy backend

Edit 2:
It seems the whole authentication system is being reworked right now in LemmyNet/lemmy#3946, so I'll just wait with this until that auth system is done. From what I have seen of it so far, the issue should be gone by then (auth just passed around with a header, right?)

[dessalines]

It seems the whole authentication system is being reworked right now in LemmyNet/lemmy#3946, so I'll just wait with this until that auth system is done. From what I have seen of it so far, the issue should be gone by then (auth just passed around with a header, right?)

Yep, that's entirely correct. Once we update lemmy-ui to account for the new auth method, hopefully that will fix the caching issue.

[biosfood]

@SleeplessOne1917 Thanks for adopting the improved authentication. Things are working pretty well now. When marking a message as read, the counter will immediately update. Let me know if any more changes are needed.

[dessalines]

Why is all this global state necessary now tho? Why not just re-fetch the unread counts after an update, that'd be much cleaner and more reliable (esp since two ppl could be doing reports at the same time)

[biosfood]

To avoid requerying the counts almost every few seconds to make things responsive, I need to trigger the reloading of the updates from other components (especially the inbox, but also when marking reports as read etc.). All other options I have considered seem not really suitable (very frequent queries, having a reference to the navbar in the inbox component, adding a global variable to notify the navbar that it (probably) needs to update its counts).

When this is merged, I would plan to update the other "inboxes" (reports and applications) to work the same way the inbox does now. So the whole thing should be updatable from any component.

[dessalines]

You don't need to re-query them every few seconds tho, only after an action (like mark read, etc). And those counts are very inexpensive for a server.

Also stuff like inbox replies does need recurring fetches, because it comes from other's actions.

[biosfood]

What I was trying to say is that I don't see another option than to introduce a global object that can be notified by any component that it probably has to update itself. The component model in inferno generally does not allow one component to call a method on another component, because they don't know of each other (except their children).

I can't just call something like Navbar.notifyChange() from the Inbox component, because there might be multiple navbars or the only one is currently not being rendered. This is why I introduced this global service and state, to get around this issue and not have do do a lot of API calls, only when necessary.

[SleeplessOne1917]

If you're trying to keep state in one place that can be accessed by several components, you could try a context like you mentioned in the other comment. The context API in inferno should work the same way as described in these old React docs (inferno makes its API similar to pre-hooks react, which is why the docs I'm suggesting are old).

[biosfood]

That is of course something I can do (have done it in the past), just tell me weather you would prefer a proper context over what is currently present. Might take a day or two though as I'm quite at the moment

[SleeplessOne1917]

I think the context is the way to go since we're dealing with state that can be changed from several different places in the app.

[dessalines]

Run git show 2b1af707c3df6126b3e6890106c03c60ad49b1be , and look at navbar.tsx and UserService.ts

That was when we used proper rxjs observables to update the navbar, with UserService being the main source of truth:

public unreadReportCountSub: BehaviorSubject<number> = new BehaviorSubject<number>(0); .

Then any component like the navbar could subscribe to it to get real-time changes:

this.unreadInboxCountSub = UserService.Instance.unreadInboxCountSub.subscribe(res => {this.setState({ unreadInboxCount: res });});

You can update it like:

UserService.Instance.unreadInboxCountSub.next(DATA)

[dessalines]

Much better, thx!