Skip to content

fix(sounds): fall back when the server has no play-sounds capability - #19239

Merged
Antreesy merged 2 commits into
nextcloud:mainfrom
bakiburakogun:fix/play-sounds-fallback
Sep 5, 2026
Merged

fix(sounds): fall back when the server has no play-sounds capability#19239
Antreesy merged 2 commits into
nextcloud:mainfrom
bakiburakogun:fix/play-sounds-fallback

Conversation

@bakiburakogun

@bakiburakogun bakiburakogun commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

☑️ Resolves

Since Talk 24 the sounds store reads the user's "play sounds" setting from the capabilities (config.call.play-sounds). That works against a Talk 24+ server, but servers before Talk 24 don't have that capability at all, so getTalkConfig('local', 'call', 'play-sounds') returns undefined there: sounds are off after every start and the toggle in the settings dialog doesn't stick, which is the talk-desktop issue above again (the 2.2.x desktop client bundles the Talk 24 frontend and still supports NC 32/33 servers), just with the default flipped to silent.

The capability is read once at module level and stays the first source for users; for older servers there are two fallbacks:

  1. capability (Talk 24+)
  2. value remembered in browser storage
  3. enabled, as before Talk 24

Guests are unchanged: browser storage first, then the capability (guests_play_sounds), then enabled. On change, setPlaySounds() gets hasUserAccount && hasPlaySoundsCapability, so a server that can't hand the value back isn't written to either and the setting stays in the browser. Against Talk 24+ nothing changes.

Known limitation on older servers: a change made in another browser/device isn't picked up until the toggle is used locally, since there is no way to read the value back. That's the same trade-off @Antreesy described in the linked issue.

Added a small spec for the store's initial value and the action; the store computes its initial value at import time, so the tests reload the module.

🖼️ Screenshots

n/a

🚧 Tasks

  • Code
  • Tests

🏁 Checklist

@nickvergessen

Copy link
Copy Markdown
Member

Hi @bakiburakogun thanks for your frequent involvement. In case you are interested you can send me an email to <my github handle>@nextcloud.com so I can invite you to our Talk Developer chat.
Also the Nextcloud Conference and Contributor Week is happening soon in Berlin, if you are close and want to join us there: https://nextcloud.com/conference-2026/

@bakiburakogun

Copy link
Copy Markdown
Contributor Author

Thanks a lot @nickvergessen, glad it's useful. I'll get in touch by email about the developer chat, and the conference sounds great, I'll see if I can make it to Berlin.

Side note: the Psalm failure here doesn't seem related to this change (no PHP touched), the other recent PRs show the same error in RoomPlugin.php.

@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.

Thanks! Left some notes on the new changes: approach is correct to close the gap Talk Desktop has with old server, but we can do it in a simpler way

Don't worry about Static analysis / static-psalm-analysis CI - it might just be blocked on forks

Comment thread src/stores/sounds.js Outdated
Comment thread src/stores/sounds.js Outdated
Comment thread src/stores/sounds.js Outdated
@bakiburakogun
bakiburakogun force-pushed the fix/play-sounds-fallback branch from 6a103d8 to 88e0bf6 Compare September 4, 2026 19:42
@bakiburakogun

bakiburakogun commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

All three applied.

The capability is read once at module level now, and the fallback order is the one you sketched. I kept your order as written, so a guest with a remembered value still wins over the capability. Say so if you meant the capability to come first for guests as well.

Dropping loadState had a knock-on effect worth mentioning. With hasUserAccount && hasPlaySoundsCapability as the first argument, setPlaySounds() already takes its BrowserStorage.setItem() branch when the server cannot take the value, so the explicit mirror in setShouldPlaySounds() became a second write of the same key. I removed it rather than keep both.

Tests updated accordingly: the two loadState cases are gone, and the old-server case now asserts setPlaySounds(false, 'no') instead of a direct BrowserStorage.setItem(). The ten cases pass locally and eslint is clean on both files.

Since Talk 24 the sounds store takes the user's "play sounds" setting from
the capabilities (config.call.play-sounds). Servers before Talk 24 don't
expose it there, so against such a server getTalkConfig() returns undefined,
sounds are off after every start and the toggle in the settings dialog
doesn't stick.

Read the capability once. Guests keep what this browser remembered,
otherwise the capability decides; without it, fall back to the value in
browser storage and finally to enabled. On change, pass the capability
along to setPlaySounds: a server that cannot hand the value back is not
written to either, and the setting stays in the browser.

Ref nextcloud/talk-desktop#1087

Signed-off-by: Baki Burak Ogun <63836730+bakiburakogun@users.noreply.github.com>
@bakiburakogun
bakiburakogun force-pushed the fix/play-sounds-fallback branch from 88e0bf6 to fc22ff9 Compare September 4, 2026 19:56
- setPlaySounds mock was unnecessary, since it skipped BrowserStorage call
- mock axios.post instead

Assisted-by: ClaudeCode:claude-sonnet-5
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>

@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.

Checked locally, adjusted test so they don't contradict (e.g. BrowserStorage.setItem should be called for guests) - see last commit

a guest with a remembered value still wins over the capability

Correct, otherwise they won't be able to rewrite it

I removed it rather than keep both.

Yup, service function already takes care of it

Thanks again!

@Antreesy

Antreesy commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

/backport to stable35

@Antreesy

Antreesy commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

/backport to stable34

@Antreesy
Antreesy merged commit 7f90b0c into nextcloud:main Sep 5, 2026
53 of 54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants