Skip to content

fix(client): align device preference persistence with permission and track end events#2196

Merged
jdimovska merged 2 commits intomainfrom
device-preferences-permission
Apr 10, 2026
Merged

fix(client): align device preference persistence with permission and track end events#2196
jdimovska merged 2 commits intomainfrom
device-preferences-permission

Conversation

@jdimovska
Copy link
Copy Markdown
Contributor

@jdimovska jdimovska commented Apr 9, 2026

💡 Overview

This PR fixes a bug where device preferences were applied when the browser had not yet granted permissions, which could trigger unexpected permission prompts. Now applyPersistedPreferences is gated behind a browserPermissionState === 'granted' check, so preferences are only restored when the user has already granted access. Additionally, when a track ends unexpectedly (tab close, device disconnect), we no longer store that state to device preferences, preventing incorrect muted/disabled preferences from being persisted.

📝 Implementation notes

🎫 Ticket: https://linear.app/stream/issue/REACT-944/align-device-preferences-with-browser-permissions

📑 Docs: https://github.com/GetStream/docs-content/pull/

Summary by CodeRabbit

  • Bug Fixes
    • Camera, microphone, and speaker preferences now respect browser permission state: persisted preferences are applied and stored only when permission is explicitly "granted"; otherwise they are not applied or saved.
  • Tests
    • Added and updated unit tests to cover permission-gated apply/store behavior and track-end persistence scenarios across camera, microphone, speaker, and device manager logic.

@jdimovska jdimovska requested a review from oliverlaz April 9, 2026 22:00
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 9, 2026

⚠️ No Changeset found

Latest commit: c148e85

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 9, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7bf600a4-e3ed-4679-a92b-902bbff6c546

📥 Commits

Reviewing files that changed from the base of the PR and between 887f398 and c148e85.

📒 Files selected for processing (1)
  • packages/client/src/devices/DeviceManager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/client/src/devices/DeviceManager.ts

📝 Walkthrough

Walkthrough

Device managers now gate applying and persisting device preferences on browser permission state, only applying or storing preferences when the browser permission observable reports 'granted'. Tests were added/updated to cover granted vs non‑granted permission scenarios.

Changes

Cohort / File(s) Summary
Core Device Manager Updates
packages/client/src/devices/CameraManager.ts, packages/client/src/devices/MicrophoneManager.ts, packages/client/src/devices/SpeakerManager.ts, packages/client/src/devices/DeviceManager.ts
Added observation/awaiting of browser permission state (browserPermissionState$ / getAudioBrowserPermission(...).asStateObservable() / firstValueFrom). Applying persisted preferences and writing persisted preferences now only proceed when permission state is 'granted'. Adjusted persistence subscription conditions to skip writes when permission not granted or when a track-end disable occurred.
Tests: device permission scenarios
packages/client/src/devices/__tests__/CameraManager.test.ts, packages/client/src/devices/__tests__/MicrophoneManager.test.ts, packages/client/src/devices/__tests__/SpeakerManager.test.ts, packages/client/src/devices/__tests__/DeviceManager.test.ts
Updated test setup to stub permission observable (default 'granted') and added tests verifying: preferences are applied/stored when permission is 'granted', not applied/stored when permission is 'prompt', and persisted preferences are not overwritten by track-end disable flows. Adjusted existing tests to assert permission-gated behavior.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App as "App / DeviceManager"
  participant Perm as "BrowserPermission\n(asStateObservable / firstValueFrom)"
  participant Devices as "Media Devices\n(select/enable)"
  participant Store as "Preference Store"

  App->>Perm: request current permission state
  alt permission === 'granted'
    Perm-->>App: 'granted'
    App->>Store: read persisted preferences
    Store-->>App: persisted prefs
    App->>Devices: apply persisted preferences (select/enable)
    Devices-->>App: device ready
    App->>Store: persist new selection (on changes)
  else permission != 'granted'
    Perm-->>App: 'prompt' / 'denied'
    App->>Devices: enable defaults (do not apply persisted prefs)
    Devices-->>App: device ready
    Note right of App: skip writing persisted prefs
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped to check the browser gate,
If granted, I stash each chosen mate.
If prompt or closed, I’ll wait and play—
Defaults today, prefs another day.
A tiny hop for settings, tidy and light. 🥕📷🎧

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: aligning device preference persistence with permission checks and track end event handling.
Description check ✅ Passed The description follows the required template structure with both 💡 Overview and 📝 Implementation notes sections, includes the ticket reference and docs link, and clearly explains the bug fix and implementation details.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch device-preferences-permission

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/client/src/devices/DeviceManager.ts`:
- Around line 92-103: The guard in the combineLatest callback currently uses the
broad boolean this.isTrackStoppedDueToTrackEnd which blocks all persistence
writes during the flag window; change the logic in the combineLatest
subscription (the callback that receives [selectedDevice, status,
browserPermissionState]) to stop ignoring writes only when the persisted state
truly matches a track-end stop rather than any change: remove the global
isTrackStoppedDueToTrackEnd check from the unconditional early return and
instead compare the current selectedDevice (from selectedDevice$) against the
priorSelectedDevice (store previous value in a local variable inside
DeviceManager) and only skip persistence when priorSelectedDevice indicates it
was stopped due to track end and the new selectedDevice is identical (i.e., no
user-initiated recovery); keep the browserPermissionState and status checks
as-is.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e0f4a120-0a8c-4beb-ba94-48bcd330bece

📥 Commits

Reviewing files that changed from the base of the PR and between 34ad675 and 887f398.

📒 Files selected for processing (8)
  • packages/client/src/devices/CameraManager.ts
  • packages/client/src/devices/DeviceManager.ts
  • packages/client/src/devices/MicrophoneManager.ts
  • packages/client/src/devices/SpeakerManager.ts
  • packages/client/src/devices/__tests__/CameraManager.test.ts
  • packages/client/src/devices/__tests__/DeviceManager.test.ts
  • packages/client/src/devices/__tests__/MicrophoneManager.test.ts
  • packages/client/src/devices/__tests__/SpeakerManager.test.ts

@jdimovska jdimovska merged commit b4ed7c2 into main Apr 10, 2026
20 of 21 checks passed
@jdimovska jdimovska deleted the device-preferences-permission branch April 10, 2026 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants