feat(settings): system volume control with two-way OS sync#69
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bac4a6ac19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * persisted level instead of falsely reporting "unavailable". | ||
| */ | ||
| async function getVolume(): Promise<{ volume: number | null; available: boolean }> { | ||
| const status = await getSystemVolumeStatus(); |
There was a problem hiding this comment.
When a client connects while the fire-and-forget startup setSystemVolume(startupVolume) is still running, this live read can return the pre-write OS level. The startup .then later seeds volumeWatcher with the written value, so the next poll sees no diff and never corrects that client; the footer can then stay on the old level until the user changes volume again. Gate getVolume on the pending/serialized write or cache the startup result before answering the initial read.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in 2916034 — the startup mixer write was removed entirely. The backend now only reads the OS volume on start and adopts it (seed + persisted preference), so there is no pre-write level a concurrent client read could observe.
| const volumeWatcher = createVolumeWatcher({ | ||
| getStatus: getSystemVolumeStatus, | ||
| broadcast: status => broadcast('system:volumeChanged', status), | ||
| persist: v => void settings.set('audio.volume', v), |
There was a problem hiding this comment.
When the OS monitor emits several volume changes quickly, this fire-and-forget settings.set starts overlapping whole-file writes through JSONStorage.set; an older write can complete after a newer one and leave audio.volume stale on disk. Await or queue these persists so media-key/tray bursts cannot reorder the saved preference.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f864a98 — whole-file JSON writes are now chained per storage instance, so overlapping set() calls can no longer interleave truncate+write or finish out of order.
| const res = await setSystemVolume(pct); | ||
| // Seed the watcher with the value the mixer ACTUALLY ended on (res.volume, | ||
| // which under latest-wins may differ from pct) so its poll does not echo it. | ||
| volumeWatcher.remember({ volume: res.volume, available: res.available }); |
There was a problem hiding this comment.
When one connected client uses the +/- controls, system.setVolume updates only that client's local store and then remembers the new mixer state, so the watcher suppresses the subsequent poll/monitor echo instead of notifying other subscribed clients. Any other open window/tab keeps showing the old volume until a separate OS-side change happens; broadcast the resulting {volume, available} from this handler as well as remembering it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a6f9296 — system.setVolume now broadcasts the resulting {volume, available} to subscribers. The originating client ignores the level while its own adjustment is fresh, so it cannot fight in-progress input.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6f92963ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const amixer = await tryRun('amixer', ['get', 'Master']); | ||
| const pactl = amixer !== null && parseAlsaVolume(amixer) !== null ? null : await tryRun('pactl', ['get-sink-volume', '@DEFAULT_SINK@']); |
There was a problem hiding this comment.
On Linux systems that have both PulseAudio/PipeWire (pactl) and an ALSA Master control, these lines stop as soon as the ALSA read succeeds, while the monitor below subscribes to Pulse sink events. amixer(1) controls the ALSA soundcard mixer, whereas pactl(1) @DEFAULT_SINK@ commands target the default Pulse sink; in that setup tray/media-key changes update the Pulse sink but getVolume keeps returning the unrelated ALSA value, and the mirrored write path sends +/- changes to the wrong mixer unless amixer fails. Try pactl first and fall back to amixer only when no default sink is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — both the read and write paths now try the Pulse/PipeWire default sink first and fall back to ALSA Master only when no default sink answers, matching the pactl-based monitor.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 202a54e416
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // present; (re)spawn on crash or when a device reappears, stop otherwise. | ||
| if (volumeWanted && volumeWatcher.available() && !volumeMonitor) { | ||
| volumeMonitor = startVolumeMonitor( | ||
| status => volumeWatcher.ingest(status), |
There was a problem hiding this comment.
When a client changes the volume while the instant monitor is running, this callback sends monitor readings straight into volumeWatcher.ingest(), bypassing the isMixerWriteBusy() guard that only poll() uses. On Windows the 150 ms monitor, and on Linux pactl subscribe, can observe the mixer while our own write is still in flight or settling, so an old/intermediate value can be persisted and broadcast as an external change despite the serializer's settle window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5866fde — the busy guard moved into the watcher's ingest(), so monitor pushes observed while a write is in flight or settling are dropped for both the Windows timer monitor and the pactl subscriber.
| // (a second window/tab) would never hear about it — tell them directly. | ||
| // The originating client ignores the level while its own adjustment is | ||
| // fresh, so this cannot fight the user's in-progress input. | ||
| if (res.success) broadcast('system:volumeChanged', { volume: res.volume, available: res.available }); |
There was a problem hiding this comment.
When a device disappears during a volume change, setSystemVolume() returns success: false, available: false; this code still seeds the watcher with that unavailable state but then skips the broadcast. Because the next poll sees the same remembered {available:false} value, it will suppress the event too, so other subscribed windows can keep showing the old volume/device-available state indefinitely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ffb5ba4 — the write result is now broadcast unconditionally, so a device vanishing mid-write delivers available:false to all subscribers instead of being suppressed forever.
| // Availability (device plug/unplug) always applies; the level is skipped | ||
| // while the user is mid-adjustment so a stale poll cannot fight their input. | ||
| volumeAvailable.set(data.available); | ||
| if (Date.now() - lastLocalVolumeChange > 1000 && data.available && data.volume !== null) volume.set(data.volume); |
There was a problem hiding this comment.
When another window or the OS changes the volume within one second of this client pressing +/- , this condition drops that system:volumeChanged level permanently rather than deferring it. The backend remembers its own write and suppresses later identical polls, and the system.setVolume response does not include the final volume, so this client can remain stuck showing its local in-progress value until some future volume change happens.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ab6fa13 — events inside the guard window are deferred (latest wins) and replayed once the window expires; a newer local edit discards the deferred level since the local write supersedes it.
| const pactl = await tryRun('pactl', ['get-sink-volume', '@DEFAULT_SINK@']); | ||
| const amixer = pactl !== null && parseAlsaVolume(pactl) !== null ? null : await tryRun('amixer', ['get', 'Master']); | ||
| return classifyMixerReadings([pactl, amixer]); |
There was a problem hiding this comment.
On Linux, tryRun() returns null for any pactl/amixer failure, including a timeout or other transient command error, and this all-null path is passed into classifyMixerReadings() as no-device. That contradicts the getSystemVolumeStatus() contract that transient helper failures should be indeterminate, and it can incorrectly broadcast/persist available:false for users whose audio device is still present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e8d0ae9 — tryRun rethrows timeout kills so they classify as transient error (indeterminate) instead of no-device; a missing binary or non-zero exit (e.g. pactl 'No such entity') stays a definitive no-device signal.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Adds system audio volume control that keeps the app and the OS volume in sync both ways.
osascript, Linuxamixerwith apactlfallback)settings.footerWidgets.volumeUnavailable(EN "Audio device not available", CS "Zvukové zařízení není k dispozici")