Replace airtunes lib by airplay-sender#2439
Open
bertrandda wants to merge 10 commits intoGladysAssistant:masterfrom
Open
Replace airtunes lib by airplay-sender#2439bertrandda wants to merge 10 commits intoGladysAssistant:masterfrom
bertrandda wants to merge 10 commits intoGladysAssistant:masterfrom
Conversation
📝 WalkthroughWalkthroughReplaced runtime Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as AirplayHandler
participant Sender as AirplaySender
participant FFmpeg as FFmpeg
participant Device as Device
Client->>Handler: setValue(audioValue)
Handler->>Sender: start({ host, airplay2:true, volume }, callback)
Sender-->>Handler: { event: "device", message: "ready" }
Handler->>FFmpeg: spawn(decoder -> PCM stdout)
FFmpeg-->>Handler: stdout chunk (PCM)
Handler->>Sender: sendPcm(chunk)
Sender->>Device: stream PCM
FFmpeg-->>Handler: stdout end
Handler->>Sender: schedule stop() (after 7s)
Handler->>Sender: stop()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/services/airplay/lib/airplay.setValue.js`:
- Around line 24-28: Replace the falsy-check fallback for the numeric optional
parameter so a legitimate 0 is preserved: change the use of logical OR on
options?.volume to nullish coalescing (use options?.volume ?? 70) where the
object with host/ipAddress/airplay2/volume is built in airplay.setValue.js (the
volume property assignment), ensuring other numeric optional parameters in the
same file follow the same pattern if present.
- Around line 31-61: The spawned ffmpeg process (decodeProcess created via
this.childProcess.spawn) lacks an 'error' event listener, so add
decodeProcess.on('error', ...) to handle spawn failures: log the error with
logger.error including the error object/message, and call sender.stop() (and any
necessary cleanup) to avoid an unhandled exception; ensure the handler
references decodeProcess and sender so the listener is tied to the correct
process and tears down playback on failure.
#4129 Bundle Size — 11.44MiB (0%).a560324(current) vs b3cfa5a master#4120(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
| Current #4129 |
Baseline #4120 |
|
|---|---|---|
 Initial JS |
6.42MiB |
6.42MiB |
| Initial CSS |
310.54KiB |
310.54KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
51 |
51 |
| Assets |
179 |
179 |
| Modules |
1643 |
1643 |
| Duplicate Modules |
21 |
21 |
| Duplicate Code |
0.94% |
0.94% |
| Packages |
136 |
136 |
| Duplicate Packages |
2 |
2 |
Bundle size by type no changes
| Current #4129 |
Baseline #4120 |
|
|---|---|---|
| JS |
8.32MiB |
8.32MiB |
| IMG |
2.68MiB |
2.68MiB |
| CSS |
328.39KiB |
328.39KiB |
| Fonts |
93.55KiB |
93.55KiB |
| Other |
18.82KiB |
18.82KiB |
| HTML |
13.58KiB |
13.58KiB |
Bundle analysis report Branch bertrandda:fix/change-airplay-li... Project dashboard
Generated by RelativeCI Documentation Report issue
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2439 +/- ##
=======================================
Coverage 98.81% 98.81%
=======================================
Files 1009 1009
Lines 17670 17683 +13
=======================================
+ Hits 17460 17473 +13
Misses 210 210 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/test/services/airplay/lib/airplay.setValue.test.js (1)
66-72: Redundantsinon.reset()calls.Having
sinon.reset()in bothbeforeEachandafterEachis unnecessary—one is sufficient to isolate tests.♻️ Suggested simplification
- beforeEach(() => { - sinon.reset(); - }); - afterEach(() => { sinon.reset(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/airplay/lib/airplay.setValue.test.js` around lines 66 - 72, The tests call sinon.reset() in both beforeEach and afterEach which is redundant; remove one of the calls so reset is performed only once per test cycle. Edit the test file to keep either beforeEach(() => sinon.reset()) or afterEach(() => sinon.reset()) and delete the duplicate block (references: beforeEach, afterEach, sinon.reset) to ensure proper isolation without duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/test/services/airplay/lib/airplay.setValue.test.js`:
- Around line 66-72: The tests call sinon.reset() in both beforeEach and
afterEach which is redundant; remove one of the calls so reset is performed only
once per test cycle. Edit the test file to keep either beforeEach(() =>
sinon.reset()) or afterEach(() => sinon.reset()) and delete the duplicate block
(references: beforeEach, afterEach, sinon.reset) to ensure proper isolation
without duplication.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/test/services/airplay/lib/airplay.setValue.test.js (1)
97-130: Consider asserting no PCM is sent on execvp failure.If the intended behavior is that streaming never starts when ffmpeg fails to spawn, a
notCalledassertion will lock in that contract.Proposed test tightening
await new Promise((resolve) => { setImmediate(resolve); }); sinon.assert.calledOnce(stopSender); + sinon.assert.notCalled(sendPcm);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/airplay/lib/airplay.setValue.test.js` around lines 97 - 130, The test should also assert that no PCM/audio sending is attempted when ffmpeg fails to spawn: after creating airplayHandlerWithExecvpError and calling setValue(device, ...), add a sinon assertion that the PCM sending stub (e.g., airplaySender.sendPCM or the specific stub used to stream audio in tests) was not called (sinon.assert.notCalled(sendPCM)). Keep the existing check that stopSender was called; place the notCalled check before or after the stopSender assertion to lock in the contract that streaming never starts on execvp failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/test/services/airplay/lib/airplay.setValue.test.js`:
- Around line 97-130: The test should also assert that no PCM/audio sending is
attempted when ffmpeg fails to spawn: after creating
airplayHandlerWithExecvpError and calling setValue(device, ...), add a sinon
assertion that the PCM sending stub (e.g., airplaySender.sendPCM or the specific
stub used to stream audio in tests) was not called
(sinon.assert.notCalled(sendPCM)). Keep the existing check that stopSender was
called; place the notCalled check before or after the stopSender assertion to
lock in the contract that streaming never starts on execvp failure.
There was a problem hiding this comment.
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 `@server/test/services/airplay/lib/airplay.setValue.test.js`:
- Around line 21-33: The mock childProcessMock.spawn currently invokes the
stdout 'end' callback (inside stdout.on) which triggers production
setTimeout(7000); remove the automatic invocation of the 'end' event in the mock
so only the 'data' emission occurs during the test—edit the
childProcessMock.spawn implementation (stdout.on handler) to stop calling cb()
for type === 'end' (or only call it when an explicit test helper flag is set),
leaving the 'data' emission intact; this prevents the real 7-second timer from
being scheduled while keeping assertions based on the 'data' event working.
---
Duplicate comments:
In `@server/services/airplay/lib/airplay.setValue.js`:
- Around line 23-28: The volume defaulting currently uses a logical OR which
treats 0 as falsy and overwrites mute (options?.volume || 70); change it to use
the nullish coalescing operator so only null/undefined fall back (replace the
volume expression passed into this.airplaySender with options?.volume ?? 70) to
preserve a valid 0 volume; locate the call to this.airplaySender in
airplay.setValue.js and update that volume expression accordingly.
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/test/services/airplay/lib/airplay.setValue.test.js (2)
74-101: Consider wrapping clock lifecycle in try/finally for test resilience.If an assertion fails before
clock.restore(), the fake timers remain active and could affect subsequent tests. Using try/finally ensures cleanup regardless of test outcome.♻️ Suggested pattern
it('should talk on speaker', async () => { airplayHandler.scanTimeout = 1; const devices = await airplayHandler.scan(); const device = devices[0]; const clock = sinon.useFakeTimers({ toFake: ['setTimeout'] }); - await airplayHandler.setValue(device, device.features[0], 'http://play-url.com'); - await new Promise((resolve) => { - setImmediate(resolve); - }); - clock.tick(7000); - sinon.assert.calledOnce(sendPcm); - sinon.assert.calledOnce(stopSender); - clock.restore(); + try { + await airplayHandler.setValue(device, device.features[0], 'http://play-url.com'); + await new Promise((resolve) => { + setImmediate(resolve); + }); + clock.tick(7000); + sinon.assert.calledOnce(sendPcm); + sinon.assert.calledOnce(stopSender); + } finally { + clock.restore(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/airplay/lib/airplay.setValue.test.js` around lines 74 - 101, Wrap the fake-timer lifecycle in each test in a try/finally so clock.restore() always runs: create the clock with sinon.useFakeTimers(...) before the awaited setValue call, put the assertions (sinon.assert.calledOnce(sendPcm) etc.) inside the try block, and call clock.restore() in the finally block; update both tests that use clock (the two it blocks invoking airplayHandler.setValue, sendPcm and stopSender) to ensure cleanup even if an assertion or await throws.
155-162: Consider simplifying the callback sequencing.The
async/awaitpattern insideprocess.nextTickworks, but theawaiton synchronous callbacks is redundant. Since callbacks execute immediately, a simpler approach would be clearer.♻️ Simplified pattern
const airplaySenderWithBufferEnd = (options, callback) => { const senderInstance = { sendPcm, stop: stopSender }; - process.nextTick(async () => { - await callback({ event: 'device', message: 'ready' }); - await callback({ event: 'buffer', message: 'end' }); + process.nextTick(() => { + callback({ event: 'device', message: 'ready' }); + callback({ event: 'buffer', message: 'end' }); }); return senderInstance; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/airplay/lib/airplay.setValue.test.js` around lines 155 - 162, The sequence inside airplaySenderWithBufferEnd uses async/await unnecessarily for synchronous callback calls; replace the async function passed to process.nextTick with a plain function that directly invokes callback twice in order (first with { event: 'device', message: 'ready' } then with { event: 'buffer', message: 'end' }) and return the same senderInstance; keep sendPcm and stop: stopSender intact and do not await the callback calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/test/services/airplay/lib/airplay.setValue.test.js`:
- Around line 74-101: Wrap the fake-timer lifecycle in each test in a
try/finally so clock.restore() always runs: create the clock with
sinon.useFakeTimers(...) before the awaited setValue call, put the assertions
(sinon.assert.calledOnce(sendPcm) etc.) inside the try block, and call
clock.restore() in the finally block; update both tests that use clock (the two
it blocks invoking airplayHandler.setValue, sendPcm and stopSender) to ensure
cleanup even if an assertion or await throws.
- Around line 155-162: The sequence inside airplaySenderWithBufferEnd uses
async/await unnecessarily for synchronous callback calls; replace the async
function passed to process.nextTick with a plain function that directly invokes
callback twice in order (first with { event: 'device', message: 'ready' } then
with { event: 'buffer', message: 'end' }) and return the same senderInstance;
keep sendPcm and stop: stopSender intact and do not await the callback calls.
Contributor
Pierre-Gilles
left a comment
Pierre-Gilles
left a comment
There was a problem hiding this comment.
Looks nice! Thanks for the PR! A small comment :)
| }, | ||
| async (event) => { | ||
| if (event.event === 'device' && event.message === 'ready') { | ||
| decodeProcess = this.childProcess.spawn('ffmpeg', [ |
Contributor
There was a problem hiding this comment.
Is there any risk that this process could run indefinitely?
Why not use execFile with a proper timeout, like we do in getImage.js for the RTSP camera integration?
Contributor
Author
There was a problem hiding this comment.
Maybe it can happen if there is a deconnection during url reading. I added a global timeout after 5 minutes (we can adjust, I don't know user's notification duration). In this case execFile is not possible, file is send in realtime using chunks to homepods, execFile needs to buffer all file in memory
bertrandda
force-pushed
the
fix/change-airplay-lib
branch
from
54f967b to
8c26bdd
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/test/services/airplay/lib/airplay.setValue.test.js (1)
80-82: Consider extracting thesetImmediatewait pattern to a helper.The pattern
await new Promise((resolve) => setImmediate(resolve))is repeated 7 times across tests. A helper function would improve readability and reduce duplication.♻️ Optional helper extraction
+const flushNextTick = () => new Promise((resolve) => setImmediate(resolve)); + describe('AirplayHandler.setValue', () => { // ... it('should talk on speaker', async () => { // ... - await new Promise((resolve) => { - setImmediate(resolve); - }); + await flushNextTick(); // ... });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/services/airplay/lib/airplay.setValue.test.js` around lines 80 - 82, The tests repeatedly use the pattern await new Promise((resolve) => setImmediate(resolve)); extract this into a small test helper (e.g., waitImmediate or flushImmediate) and replace the seven inline instances in server/test/services/airplay/lib/airplay.setValue.test.js with await waitImmediate(); create the helper in your test utils module (or a new file under server/test/helpers) exporting the function and import it at the top of airplay.setValue.test.js; ensure the helper simply returns new Promise(resolve => setImmediate(resolve)) so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/test/services/airplay/lib/airplay.setValue.test.js`:
- Around line 80-82: The tests repeatedly use the pattern await new
Promise((resolve) => setImmediate(resolve)); extract this into a small test
helper (e.g., waitImmediate or flushImmediate) and replace the seven inline
instances in server/test/services/airplay/lib/airplay.setValue.test.js with
await waitImmediate(); create the helper in your test utils module (or a new
file under server/test/helpers) exporting the function and import it at the top
of airplay.setValue.test.js; ensure the helper simply returns new
Promise(resolve => setImmediate(resolve)) so behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6a2921c-34da-4af8-b2b5-e0ba06302165
📒 Files selected for processing (1)
server/test/services/airplay/lib/airplay.setValue.test.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request check-list
To ensure your Pull Request can be accepted as fast as possible, make sure to review and check all of these items:
npm teston both front/server)npm run eslinton both front/server)npm run prettieron both front/server)NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
Description of change
Change airtunes2 in airplay integration by airplay-sender.
This new lib is modern, code is cleaner, ts only (airtunes used C++ binding). It will avoid failure on future Nodejs upgrades and avoid required lib build on installation.
Also update
bonjourlib.Summary by CodeRabbit
Dependencies
Improvements
Bug Fixes
Tests