Skip to content

fix: Send read marker only when session is active - #19097

Open
SystemKeeper wants to merge 2 commits into
mainfrom
fix/18734/session-state-unread-marker
Open

fix: Send read marker only when session is active#19097
SystemKeeper wants to merge 2 commits into
mainfrom
fix/18734/session-state-unread-marker

Conversation

@SystemKeeper

@SystemKeeper SystemKeeper commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

☑️ Resolves

Not risky to browser differences / client

Since the mechanism is the same as before, I think it should not be risky?

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

🏁 Checklist

  • 🌏 Tested with different browsers / clients:
    • Chromium (Chrome / Edge / Opera / Brave)
    • Firefox
    • Safari
    • Talk Desktop
    • Integrations with Files sidebar and other apps
    • Not risky to browser differences / client
  • 🖌️ Design was reviewed, approved or inspired by the design team
  • ⛑️ Tests are included or not possible
  • 📗 User documentation in https://github.com/nextcloud/documentation/tree/master/user_manual/talk has been updated or is not required

@SystemKeeper SystemKeeper added this to the ⛅ Next Beta/RC (35) milestone Aug 26, 2026
@SystemKeeper SystemKeeper self-assigned this Aug 26, 2026
@SystemKeeper SystemKeeper added bug feature: chat 💬 Chat and system messages feature: frontend 🖌️ "Web UI" client labels Aug 26, 2026
@nextcloud-command nextcloud-command added the AI assisted This PR contains AI-assisted commits label Aug 26, 2026
@SystemKeeper SystemKeeper changed the title Fix/18734/session state unread marker fix: Send unread marker only when session is active Aug 26, 2026

@Antreesy Antreesy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

General idea makes sense to me, let's polish it slightly

I think it should not be risky?

Chrome doesn't fire 'visibilitychange' event same way as e.g. Firefox, so you never know =)

Comment thread src/composables/useActiveSession.js Outdated
Comment thread src/composables/useActiveSession.js Outdated
Comment thread src/composables/useActiveSession.js
@SystemKeeper
SystemKeeper force-pushed the fix/18734/session-state-unread-marker branch from 7fa910a to 386acb3 Compare August 26, 2026 16:28
Assisted-by: ClaudeCode:claude-opus-5
Signed-off-by: Marcel Müller <marcel-mueller@gmx.de>
@SystemKeeper
SystemKeeper force-pushed the fix/18734/session-state-unread-marker branch from 386acb3 to 18c61e9 Compare August 26, 2026 16:29
Signed-off-by: Marcel Müller <marcel-mueller@gmx.de>
@SystemKeeper
SystemKeeper force-pushed the fix/18734/session-state-unread-marker branch from 18c61e9 to 7f7a20b Compare August 26, 2026 16:30
@SystemKeeper
SystemKeeper requested a review from Antreesy August 26, 2026 16:30
@SystemKeeper SystemKeeper changed the title fix: Send unread marker only when session is active fix: Send read marker only when session is active Aug 26, 2026
@TiagoRaposoBR

Copy link
Copy Markdown

I ran into this too, and ended up building an AI-assisted client-side override for my instance (not a patch to Talk). It's been running for the last two weeks and the behaviour has been flawless, so I'm sharing the field data plus what the override does differently, expressed as changes to this branch. I don't want to capture the PR, but instead provide feedback into some gaps I noticed, so they are solved before merging into main.

The following message was generated by the AI that build the solution, and reviewed by me, and is ready to feed to another AI if needed (key words are there).

What we were seeing (six people, same server, Talk 23.0.9 self-hosted):

  • With a conversation open, switching to another application silenced notifications for 3 minutes. Reproducible every time, and it hits the busiest channel hardest, because most replies land inside that window.
  • Which path fired was unpredictable from the user's side: minimizing or switching tabs marks the session inactive immediately, while merely covering the window with another app leaves the document "visible" and takes the full 3 minutes. Same gesture, different outcome — that is what made it feel random rather than reproducible.
  • Notifications arrived on the phone and disappeared, or messages were found already read although nobody had read them.
  • Mentions went missing too — which was the clue that finally split the problem in two. shouldMentionedUserBeNotified() never looks at the session, so a missing mention can't be suppression; it had to be the read marker deleting a notification that was already delivered. Worth noting for anyone debugging this: an absent row in oc_notifications does not prove suppression.

What this PR fixes: the read marker no longer follows document visibility, so once the session is inactive the client stops deleting the conversation's notifications. That's the "delivered, then withdrawn" half, and it's the important one.

What I think is still open, and what my override does instead. Roughly in order of impact:

  1. handleMouseEnter still calls setSessionAsActive(), which only early-returns when the state is already ACTIVE. Once the session has gone inactive, moving the pointer over the unfocused window re-sends state=1 and re-arms the timer — so the window isn't capped at 60s, it can be re-opened indefinitely. My override doesn't treat mouse movement as a presence signal at all; the mouseenter/mouseleave listeners are gone entirely.

  2. The blur window goes from 180s to 60s, not to zero. Suppression is decided once, at message time, so a message inside that window doesn't notify late — it never notifies. My override has no timer on the blur path: the predicate is document.visibilityState === 'visible' && document.hasFocus(), evaluated on focus, blur and visibilitychange only, with a 300ms coalesce purely as request hygiene for focus-follows-mouse window managers. Two weeks in, the state flapping I expected from dropping the timer hasn't materialized.

  3. The watch(token) race can be closed instead of waited out.
    // Updating right away would race with joining the conversation is correct — useGetToken() is useRouteParams('token'), so it fires before the join request is even issued, and losing that race goes through the 404 handler and triggers a rejoin. But src/stores/token.ts already exports currentConversationIsJoined, updated from handleSignalingJoinRoom(). Watching that instead removes the race with no delay, and without depending on the timeout being longer than a join round-trip on a slow link. That also frees INACTIVE_TIME_MS to be purely a blur policy, which is a separate decision from a race guard.

  4. The swallowed read marker is never replayed. With this PR, when focus returns the marker position is only re-evaluated on the next scroll or the next message, so a conversation stays visually unread after being read. My override replays the request it swallowed as soon as the predicate turns true; in this branch the equivalent would be calling updateReadMarkerPosition() once when isChatActive flips back to true.

  5. One thing to double-check: this PR changes if (!supportSessionState) to if (!supportSessionState.value). Since supportSessionState is a computed, the ref object is always truthy — that guard has never fired. This PR makes it live for the first time, and it's read once at composable setup. Conversations without the session-state capability (the FIXME mentions federation) would now stop reporting state entirely, which leaves the session ACTIVE for as long as the conversation is open. Might be intended, but it isn't mentioned in the description.

My analysis and the override were both AI-assisted, so the reasoning deserves your review rather than trust — but the runtime behaviour of the resulting predicate has been solid in daily use for two weeks with six people. Happy to test a build of this branch against the same setup.

@Antreesy

Copy link
Copy Markdown
Contributor

Thanks, @TiagoRaposoBR !

1-2 were deliberate choises in the past to keep session alive, but they indeed shift expectations when tested live. Need to revisit those
3 probably true, was introduced recently and need to be checked
4 is a large unresolved issues that would only go as a follow-up
5 mostly meant for federated conversations, currently WIP

@SystemKeeper changes are good already, I'll check tomorrow, and would likely merge it as-is and draft a follow-up for these new + my findings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI assisted This PR contains AI-assisted commits bug feature: chat 💬 Chat and system messages feature: frontend 🖌️ "Web UI" client

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Messages marked as read when session is inactive

4 participants