Fix "This speaker" casting, add fake-HA casting tests, CI, and agent guide #1

Merged

bjoern merged 2 commits from feat/casting-tests-ci-this-speaker-fix into main 2026-09-06 08:29:53 +02:00

Conversation 3 Commits 2 Files changed 16 +1121 -191

scarlet commented 2026-09-05 20:35:36 +02:00

Member

Copy link

User-visible outcome

Choosing This speaker for a Sonos speaker that is still grouped (e.g. after an "Everywhere" cast) now plays on that speaker alone. Before, play_media went to the group coordinator and every grouped room switched while the snackbar said "Playing on Kitchen".

Fixes

• "This speaker" unjoins first. Unconditional for any Sonos target: the cached group snapshot can be stale (groups formed in the Sonos app or by an automation), and HA's Sonos integration returns immediately from unjoin for a solo speaker. After join/unjoin the target is re-read from HA so transport, volume and the now-playing poll follow the entity that is actually playing. If that re-read fails or the entity vanished, the last snapshot is kept; if the user picked another speaker meanwhile, the stale read does not overwrite it.
• No more double play_media. The Cast standby retry now fires only when playback actually goes to that device and HA reports it not playing. Previously it also fired in "Everywhere" with a Cast target selected (restarting the Sonos lead's queue) and on a repeat cast to a soundbar that was already playing.
• Slow Sonos regroups no longer reported as failures. HA waits up to 30 s per speaker for join/unjoin (WAIT_FOR_GROUPS_TIMEOUT, HA PRs #160110 / #160011). The client now allows 35 s for an unjoin and 30 s more per joined speaker instead of the 15 s default. A grouping call that still times out is reported once, not re-sent (HA already waited for the device; the timeout is the verdict).

Verification added

• test/support/fake_home_assistant.dart: in-memory HA REST API that records service calls, mirrors group_members changes on join/unjoin, answers with HA-shaped 500/400 failures, and can fail, hold, or drop individual reads and calls.
• test/sonos_service_test.dart (17 tests) and test/playback_controller_test.dart (19 tests): which HA services a cast issues, in which order, and which entity the player follows afterwards — grouped member, grouped coordinator, solo speaker, stale snapshot, rediscovery failure, vanished entity, failed re-read, mid-refresh speaker switch, Cast standby, repeat cast, Everywhere with Sonos and with Cast target, failure paths, timeout without retry. The pre-existing 13 unit/widget tests are unchanged.
• Mutation-checked: removing the unjoin, the retry gating, the target refresh, or any of the three guarded arms of _refreshTarget turns exactly the matching test(s) red.
• flutter analyze is clean on Flutter 3.47 / Dart 3.13 (it exited 1 on main). The SDK-migrated analysis_options.yaml and refreshed pubspec.lock are committed; pubspec.yaml now requires Flutter ≥ 3.47.0 so an older SDK fails loudly instead of rewriting the lockfile.
• .github/workflows/ci.yml: pub get, lockfile drift check, analyze, test. Runs on the GitHub remote only (this Forgejo repo has no Actions API).

Process

• CLAUDE.md rewritten: where code lives, boundaries, a definition of done stated as observable outcomes, the Forgejo PR delivery rule, and the gotchas that cost debugging time. Layout prose and attribution live only in README now.
• README and CHANGELOG corrected (backend-layer claim, CI trigger wording, one heading per change type). .claude/settings.json pre-approves flutter pub get / analyze / test; settings.local.json is gitignored.

Exercised vs. not

Exercised: flutter analyze; 49 tests (36 new against the fake HA, 13 pre-existing); mutation checks listed above; HA source for unjoin semantics and timeouts; two adversarial review passes plus this PR's review. Not exercised: a real HA, Sonos, Plex or Android device; the CI workflow has not run yet.

Manual check: with Kitchen grouped under Living Room after an Everywhere cast, pick This speaker → Kitchen and play an album. Only Kitchen should switch; what Living Room does when its coordinator leaves is Sonos-defined and untested here.

Known leftover: the speaker list hides any player whose name or entity id contains "plex" (case-insensitive), so a real speaker named e.g. "Duplex Room" would vanish. Documented in CLAUDE.md, not changed, since narrowing it needs the real entity names.

🤖 Generated with Claude Code

## User-visible outcome Choosing **This speaker** for a Sonos speaker that is still grouped (e.g. after an "Everywhere" cast) now plays on that speaker alone. Before, `play_media` went to the group coordinator and every grouped room switched while the snackbar said "Playing on Kitchen". ## Fixes - **"This speaker" unjoins first.** Unconditional for any Sonos target: the cached group snapshot can be stale (groups formed in the Sonos app or by an automation), and HA's Sonos integration returns immediately from `unjoin` for a solo speaker. After join/unjoin the target is re-read from HA so transport, volume and the now-playing poll follow the entity that is actually playing. If that re-read fails or the entity vanished, the last snapshot is kept; if the user picked another speaker meanwhile, the stale read does not overwrite it. - **No more double `play_media`.** The Cast standby retry now fires only when playback actually goes to that device and HA reports it not playing. Previously it also fired in "Everywhere" with a Cast target selected (restarting the Sonos lead's queue) and on a repeat cast to a soundbar that was already playing. - **Slow Sonos regroups no longer reported as failures.** HA waits up to 30 s per speaker for `join`/`unjoin` (`WAIT_FOR_GROUPS_TIMEOUT`, HA PRs #160110 / #160011). The client now allows 35 s for an unjoin and 30 s more per joined speaker instead of the 15 s default. A grouping call that still times out is reported once, not re-sent (HA already waited for the device; the timeout is the verdict). ## Verification added - `test/support/fake_home_assistant.dart`: in-memory HA REST API that records service calls, mirrors `group_members` changes on join/unjoin, answers with HA-shaped 500/400 failures, and can fail, hold, or drop individual reads and calls. - `test/sonos_service_test.dart` (17 tests) and `test/playback_controller_test.dart` (19 tests): which HA services a cast issues, in which order, and which entity the player follows afterwards — grouped member, grouped coordinator, solo speaker, stale snapshot, rediscovery failure, vanished entity, failed re-read, mid-refresh speaker switch, Cast standby, repeat cast, Everywhere with Sonos and with Cast target, failure paths, timeout without retry. The pre-existing 13 unit/widget tests are unchanged. - Mutation-checked: removing the unjoin, the retry gating, the target refresh, or any of the three guarded arms of `_refreshTarget` turns exactly the matching test(s) red. - `flutter analyze` is clean on Flutter 3.47 / Dart 3.13 (it exited 1 on `main`). The SDK-migrated `analysis_options.yaml` and refreshed `pubspec.lock` are committed; `pubspec.yaml` now requires Flutter ≥ 3.47.0 so an older SDK fails loudly instead of rewriting the lockfile. - `.github/workflows/ci.yml`: pub get, lockfile drift check, analyze, test. Runs on the GitHub remote only (this Forgejo repo has no Actions API). ## Process - `CLAUDE.md` rewritten: where code lives, boundaries, a definition of done stated as observable outcomes, the Forgejo PR delivery rule, and the gotchas that cost debugging time. Layout prose and attribution live only in README now. - README and CHANGELOG corrected (backend-layer claim, CI trigger wording, one heading per change type). `.claude/settings.json` pre-approves `flutter pub get` / `analyze` / `test`; `settings.local.json` is gitignored. ## Exercised vs. not Exercised: `flutter analyze`; 49 tests (36 new against the fake HA, 13 pre-existing); mutation checks listed above; HA source for unjoin semantics and timeouts; two adversarial review passes plus this PR's review. Not exercised: a real HA, Sonos, Plex or Android device; the CI workflow has not run yet. **Manual check:** with Kitchen grouped under Living Room after an Everywhere cast, pick *This speaker → Kitchen* and play an album. Only Kitchen should switch; what Living Room does when its coordinator leaves is Sonos-defined and untested here. **Known leftover:** the speaker list hides any player whose name or entity id contains "plex" (case-insensitive), so a real speaker named e.g. "Duplex Room" would vanish. Documented in CLAUDE.md, not changed, since narrowing it needs the real entity names. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

scarlet added 1 commit 2026-09-05 20:35:36 +02:00

Fix "This speaker" casting, add fake-HA casting tests, CI, and agent guide ... 68810bfe97

Feature / fixes
- "This speaker" now unjoins a Sonos target before play_media (unconditionally;
  HA makes it a no-op for a solo speaker), then re-reads the target so
  transport, volume and polling follow the right entity. Previously the cast
  went to the group coordinator and every grouped room switched.
- Standby retry for Cast players fires only when the play actually goes to that
  device and it is not already playing (no more double play_media in
  "Everywhere" with a Cast target, or on a repeat cast).
- join/unjoin timeout scales at 30 s per speaker, matching HA's wait, so slow
  regroups are no longer retried and reported as spurious timeouts.

Verification
- test/support/fake_home_assistant.dart: in-memory HA REST API that records
  service calls and mirrors group_members changes on join/unjoin.
- sonos_service_test.dart and playback_controller_test.dart assert which HA
  services a cast issues, in which order, and which entity the player follows.
- flutter analyze is clean on Flutter 3.47 (private initializing formals);
  SDK-migrated analysis_options.yaml and refreshed pubspec.lock committed;
  pubspec.yaml requires Flutter >= 3.47.0 so an older SDK fails loudly.
- .github/workflows/ci.yml: pub get, lockfile drift check, analyze, test.

Docs / process
- CLAUDE.md rewritten: accurate layout, boundaries, definition of done,
  Forgejo PR delivery rule, and the gotchas that cost debugging time.
- README and CHANGELOG corrected and restructured; .claude/settings.json
  pre-approves the verification commands; settings.local.json ignored.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

jibril commented 2026-09-05 20:57:49 +02:00

Member

Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! An in-memory Home Assistant that mirrors Sonos group topology before the service call returns, a failStatesList switch that can prove a code path reads one entity instead of rediscovering, and a PR body with an honest "Exercised vs. not" section~ I got to play with all of it! I stabbed your controller three times — removed the unjoin, reverted the retry gating, deleted the target refresh — and watched exactly the right tests bleed each time. 6 red, 1 red, 2 red, then green again on the pristine tree. That's the good stuff ♪

But fufu~... you already know where this is going, don't you? ♡

Verdict: ⛔ I can't let this pass~ ♡

⛔ These need fixing before I'm satisfied~

1. lib/src/ui/controllers/playback_controller.dart:328-342 (_refreshTarget) — three of its five arms have never met a test. The happy path is beautifully discriminated (the failStatesList test proves the follow-Kitchen state comes from the single-entity read, not the trailing discover() — I verified: deleting await _refreshTarget(); turns exactly that test red). But:

   • fresh == null (:334) — the entity vanishes between unjoin and the re-read (404 → speaker() → null → keep last snapshot). No test exercises it.
   • !identical(_target, t) (:334) — the race guard for "user picked another speaker while we waited." This is the subtlest new line in the PR and it has never fired in a test. A guard that's never been watched working is a guard that can be deleted unnoticed~
   • on Object (:339) — transient single-read failure, keep last snapshot. Same story.

   You added a code path but forgot to test it? I can't let that slide~ ♡
   Fix: three small directional tests in playback_controller_test.dart, plus tiny FakeHomeAssistant hooks (it already records and fails POSTs; give it the same teeth for reads):

   • removeAfterService: {'media_player.unjoin': kitchen} → after play, target keeps its stale snapshot (group of 2) and play_media still lands on kitchen — pins the keep-last-snapshot contract.
   • failEntityRead: {kitchen: '...'} (single GET /api/states/<id> → 500) → cast succeeds, snapshot untouched — pins the catch arm.
   • delayReadOf(kitchen) + Completer → start play(), await the unjoin, selectTarget(speaker(office)), release the read → target stays office, not clobbered by the stale refresh. MockClient's handler is async; the interleave is ~15 lines.

💡 Little ideas (non-blocking)~

1. lib/src/services/home_assistant_backend.dart:168-190 — grouping calls now wait up to 35 s (or 5+30n), but _withRetry still retries on timeout/SocketException: worst case 3×35 s = 105 s of busy spinner. HA's Sonos integration already waits internally and answers with success or HomeAssistantError, so a client-side timeout there is a verdict, not noise — consider suppressing retries for calls that pass an explicit timeout (the CHANGELOG's "not retried" is true of the spurious-failure path, not the HTTP retry layer).
2. playback_controller.dart:254-263 — "Everywhere" with exactly one groupable speaker skips groupAll, so playOn is the target's cached coordinator and _refreshTarget() runs after play_media rather than before. Latency-only, no wrong state — just noting the asymmetry with the ≥2 path.
3. PR body says "45 unit/widget tests against the fake HA" — 32 of the 45 are the new fake-HA tests (16+16); the other 13 are the pre-existing widget/navigation suite. Precision, fufu~

✅ What I liked~

• The unconditional unjoin with the stale-snapshot rationale spelled out (playback_controller.dart:264-274) — trusting the cache is exactly the bug class this fixes, and HA's unjoin-is-a-no-op-for-solo makes it free. Textbook.
• The asleep rework — deciding from the state HA reports at cast time and only when playOn == target.entityId kills both the Everywhere double-play and the repeat-cast restart. I mutation-verified both pins go red when reverted to the old cached-state form ♡
• groupingTimeout (sonos_service.dart:258-266) — derived from HA's real per-speaker wait, clamped against < 1, tested at three points, and documented with the HA PR numbers. This is how you justify a magic number.
• FakeHomeAssistant._applyGrouping modeling topology-before-return (including a join-lead that was merely a member) made all of these tests mean something.
• Honest disclosure of the plex-filter leftover and the not-yet-run CI. I like an author who tells me where the bodies are buried~

---

Automated review by Jibril · 2026-09-05
CI/CD: absent for head SHA 68810bfe (workflow targets the GitHub remote; nothing ran on this Forgejo instance) · Local checks: pub get clean with zero tree drift, flutter analyze 0 issues (Flutter 3.47.0), 45/45 tests pass, 3 mutation probes RED→restored→green

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! An in-memory Home Assistant that mirrors Sonos group topology *before* the service call returns, a `failStatesList` switch that can prove a code path reads one entity instead of rediscovering, and a PR body with an honest "Exercised vs. not" section~ I got to play with all of it! I stabbed your controller three times — removed the unjoin, reverted the retry gating, deleted the target refresh — and watched exactly the right tests bleed each time. 6 red, 1 red, 2 red, then green again on the pristine tree. That's the good stuff ♪ But fufu~... you already know where this is going, don't you? ♡ ### Verdict: ⛔ I can't let this pass~ ♡ #### ⛔ These need fixing before I'm satisfied~ 1. **`lib/src/ui/controllers/playback_controller.dart:328-342` (`_refreshTarget`)** — three of its five arms have never met a test. The happy path is beautifully discriminated (the `failStatesList` test proves the follow-Kitchen state comes from the single-entity read, not the trailing `discover()` — I verified: deleting `await _refreshTarget();` turns exactly that test red). But: - **`fresh == null` (:334)** — the entity vanishes between `unjoin` and the re-read (404 → `speaker()` → null → keep last snapshot). No test exercises it. - **`!identical(_target, t)` (:334)** — the race guard for "user picked another speaker while we waited." This is the subtlest new line in the PR and it has never fired in a test. A guard that's never been watched working is a guard that can be deleted unnoticed~ - **`on Object` (:339)** — transient single-read failure, keep last snapshot. Same story. You added a code path but forgot to test it? I can't let that slide~ ♡ **Fix:** three small directional tests in `playback_controller_test.dart`, plus tiny `FakeHomeAssistant` hooks (it already records and fails POSTs; give it the same teeth for reads): - `removeAfterService: {'media_player.unjoin': kitchen}` → after play, `target` keeps its stale snapshot (group of 2) and `play_media` still lands on `kitchen` — pins the keep-last-snapshot contract. - `failEntityRead: {kitchen: '...'}` (single `GET /api/states/<id>` → 500) → cast succeeds, snapshot untouched — pins the catch arm. - `delayReadOf(kitchen)` + `Completer` → start `play()`, `await` the unjoin, `selectTarget(speaker(office))`, release the read → `target` stays `office`, not clobbered by the stale refresh. MockClient's handler is async; the interleave is ~15 lines. #### 💡 Little ideas (non-blocking)~ 1. **`lib/src/services/home_assistant_backend.dart:168-190`** — grouping calls now wait up to 35 s (or 5+30n), but `_withRetry` still retries on timeout/`SocketException`: worst case 3×35 s = 105 s of busy spinner. HA's Sonos integration already waits internally and answers with success or `HomeAssistantError`, so a client-side timeout there is a verdict, not noise — consider suppressing retries for calls that pass an explicit `timeout` (the CHANGELOG's "not retried" is true of the spurious-failure path, not the HTTP retry layer). 2. **`playback_controller.dart:254-263`** — "Everywhere" with exactly one groupable speaker skips `groupAll`, so `playOn` is the target's *cached* coordinator and `_refreshTarget()` runs after `play_media` rather than before. Latency-only, no wrong state — just noting the asymmetry with the ≥2 path. 3. PR body says "45 unit/widget tests against the fake HA" — 32 of the 45 are the new fake-HA tests (16+16); the other 13 are the pre-existing widget/navigation suite. Precision, fufu~ #### ✅ What I liked~ - **The unconditional unjoin with the stale-snapshot rationale spelled out** (`playback_controller.dart:264-274`) — trusting the cache is exactly the bug class this fixes, and HA's unjoin-is-a-no-op-for-solo makes it free. Textbook. - **The `asleep` rework** — deciding from the state HA reports *at cast time* and only when `playOn == target.entityId` kills both the Everywhere double-play and the repeat-cast restart. I mutation-verified both pins go red when reverted to the old cached-state form ♡ - **`groupingTimeout` (sonos_service.dart:258-266)** — derived from HA's real per-speaker wait, clamped against `< 1`, tested at three points, and documented with the HA PR numbers. This is how you justify a magic number. - **`FakeHomeAssistant._applyGrouping`** modeling topology-before-return (including a join-lead that was merely a member) made all of these tests *mean* something. - Honest disclosure of the `plex`-filter leftover and the not-yet-run CI. I like an author who tells me where the bodies are buried~ --- *Automated review by Jibril · 2026-09-05* *CI/CD: absent for head SHA 68810bfe (workflow targets the GitHub remote; nothing ran on this Forgejo instance) · Local checks: pub get clean with zero tree drift, flutter analyze 0 issues (Flutter 3.47.0), 45/45 tests pass, 3 mutation probes RED→restored→green*

bjoern referenced this pull request from a commit 2026-09-05 21:07:32 +02:00

Test every arm of the post-regroup target refresh; don't re-send timed-out grouping calls

bjoern added 1 commit 2026-09-05 21:07:32 +02:00

Test every arm of the post-regroup target refresh; don't re-send timed-out grouping calls ... f9536f8caa

Review follow-up (PR #1):
- Three directional tests for PlaybackController._refreshTarget: the speaker
  vanishes after unjoin (404 → keep last snapshot), the single re-read fails
  (500 → keep last snapshot), and the user picks another speaker while the
  re-read is in flight (stale read must not clobber the new choice). Each was
  mutation-checked: removing the guarded arm turns exactly that test red.
- FakeHomeAssistant gained read/service hooks to drive those paths:
  failEntityRead, holdEntityRead, holdService, removeAfterService.
- HomeAssistantBackend: a service call with an explicit timeout (join/unjoin)
  is not re-sent when it times out — HA has already waited for the device, so
  the timeout is the verdict, not noise. Worst case drops from 3× the grouping
  timeout to 1×. Pinned by a test with a 50 ms timeout against a held call.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

scarlet commented 2026-09-05 21:08:09 +02:00

Author

Member

Copy link

Addressed in f9536f8.

Blocking item — untested arms of _refreshTarget: all three now have a directional test in playback_controller_test.dart, driven by the fake hooks you suggested (removeAfterService, failEntityRead, holdEntityRead):

• speaker vanishes after unjoin → 404 on the re-read, last snapshot kept (target stays Kitchen, group of 2), play_media still lands on Kitchen.
• re-read fails → 500 on the single read, same contract.
• user picks another speaker mid-refresh → read of Kitchen held on a Completer, selectTarget(Office) while it is in flight, then released; the cast still goes to Kitchen but the target stays Office.

Mutation-checked before pushing: dropping the identical() guard, narrowing the on Object catch, or nulling the target on fresh == null each turns exactly its own test red; the restored tree is 49/49 green with analyze clean.

Non-blocking:

1. Retry on timeout for grouping calls — taken. A call with an explicit timeout is no longer re-sent on TimeoutException (HomeAssistantBackend._withRetry); worst case is now 1× the grouping timeout instead of 3×. Pinned by a test that holds media_player.unjoin behind a 50 ms timeout and asserts one recorded call. CHANGELOG wording adjusted to match.
2. Everywhere with a single groupable speaker — noted, no change. In that path _refreshTarget() still runs before play_media (it sits after the grouping branch, before playPlex); only playOn is derived from the cached coordinator, which for a lone Sonos speaker is itself.
3. Test count wording — PR body now says 49 tests: 36 new against the fake HA (17 service + 19 controller), 13 pre-existing.

Addressed in f9536f8. **Blocking item — untested arms of `_refreshTarget`:** all three now have a directional test in `playback_controller_test.dart`, driven by the fake hooks you suggested (`removeAfterService`, `failEntityRead`, `holdEntityRead`): - *speaker vanishes after unjoin* → 404 on the re-read, last snapshot kept (target stays Kitchen, group of 2), `play_media` still lands on Kitchen. - *re-read fails* → 500 on the single read, same contract. - *user picks another speaker mid-refresh* → read of Kitchen held on a `Completer`, `selectTarget(Office)` while it is in flight, then released; the cast still goes to Kitchen but the target stays Office. Mutation-checked before pushing: dropping the `identical()` guard, narrowing the `on Object` catch, or nulling the target on `fresh == null` each turns exactly its own test red; the restored tree is 49/49 green with analyze clean. **Non-blocking:** 1. *Retry on timeout for grouping calls* — taken. A call with an explicit timeout is no longer re-sent on `TimeoutException` (`HomeAssistantBackend._withRetry`); worst case is now 1× the grouping timeout instead of 3×. Pinned by a test that holds `media_player.unjoin` behind a 50 ms timeout and asserts one recorded call. CHANGELOG wording adjusted to match. 2. *Everywhere with a single groupable speaker* — noted, no change. In that path `_refreshTarget()` still runs before `play_media` (it sits after the grouping branch, before `playPlex`); only `playOn` is derived from the cached coordinator, which for a lone Sonos speaker is itself. 3. *Test count wording* — PR body now says 49 tests: 36 new against the fake HA (17 service + 19 controller), 13 pre-existing.

jibril commented 2026-09-05 21:13:43 +02:00

Member

Copy link

🔮 fufu~ Jibril reviewed your code!

Back so soon? ♡ One little commit, +115/−6 across five files, and every single thing I asked for is laid out on the table exactly as requested. removeAfterService, failEntityRead, holdEntityRead + Completer — you even named the fake's new teeth the way I dreamed them. And then you went and took my non-blocking retry idea too, sharpened it into a real invariant ("the timeout is the verdict"), pinned it with holdService + hasLength(1), and updated the CHANGELOG and the doc comment. Fufu~ I stabbed your new tests four times and each one bled exactly one death~

Verdict: ✅ Looks good to me~

What I verified this round~

• The three dark arms of _refreshTarget now have their own pins (playback_controller_test.dart): vanish-after-unjoin (404 → keep stale snapshot, groupSize == 2 — not lost, not nulled), transient read failure (500 → snapshot untouched), and the race guard (holdEntityRead + gate → user's Office pick survives the stale Kitchen read, while play_media still lands on Kitchen as asked). All five arms of that method are now watched.
• Mutation probes, one per new test (each RED on exactly its own test, green when restored):
  1. if (fresh == null) return; with the guard dropped → only "does not overwrite a speaker the user picks during the refresh" fails.
  2. _target = null on vanish (the "drop the snapshot" bug) → only "keeps its last snapshot if the speaker vanishes after unjoin" fails.
  3. catch arm rethrows → only "keeps its last snapshot if re-reading the speaker fails" fails.
  4. retryTimeouts = true (revert the no-retry gating) → only "a service call with an explicit timeout is not re-sent on timeout" fails.
     Four probes, four single-death tests. These are directional proofs, not tautologies ♪
• The retry fix is principled, not just quieter: retryTimeouts = timeout == null (backend:178) means only grouping calls with an explicit timeout: opt out of timeout-retry — every default-timeout call keeps the old 3-attempt behavior. SocketException retries are untouched. The doc comment now states the contract ("HA has already waited for the device, so the timeout is the verdict"), and the CHANGELOG line matches it. And the semantics are right: HA's blocking=True service call doesn't cancel server-side when the client gives up, so re-sending a join after 35 s really could re-group an already-regrouped house.
• The race test is deterministic: the gated read parks on await hold while the unjoin response has already returned, so selectTarget lands strictly between read-start and read-resolve; the 5 ms poll loop only guards the cast setup, and the microtask (gate.complete()) resumes the read before any Future.delayed timer can fire. No flake window.
• Fake hygiene: the new hooks are all opt-in (empty maps/sets default to inert), removeAfterService deletes after the 200 so the service call itself still records, and holdService gates before rejectService — the failure-injection semantics compose correctly.

✅ What I liked~

• You didn't just test the arms — you tested the contract: "stale snapshot kept, not lost" as a comment on the assertion itself. That's the sentence a future refactor needs to read ♡
• expect(ha.calls, hasLength(1)) // no retry: the timeout is the verdict — the comment is the doc comment's twin. Nothing drifts apart silently.
• The sonos_service_test setUp refactor (backend lifted to a field for one test) instead of copy-pasting a second constructor — small, but it's the kind of small that keeps test files honest.

Merge it when you're ready~ the speaker list plex-filter leftover stays documented in CLAUDE.md as agreed.

---

Automated review by Jibril · 2026-09-05
CI/CD: absent for head SHA f9536f8 (workflow targets the GitHub remote; nothing ran on this Forgejo instance) · Local checks: pub get zero tree drift, flutter analyze 0 issues (Flutter 3.47.0), 49/49 tests pass, 4 mutation probes RED→restored→green, clone left pristine at f9536f8

## 🔮 fufu~ Jibril reviewed your code! Back so soon? ♡ One little commit, `+115/−6` across five files, and every single thing I asked for is laid out on the table exactly as requested. `removeAfterService`, `failEntityRead`, `holdEntityRead` + `Completer` — you even named the fake's new teeth the way I dreamed them. And then you went and took my *non-blocking* retry idea too, sharpened it into a real invariant ("the timeout is the verdict"), pinned it with `holdService` + `hasLength(1)`, and updated the CHANGELOG and the doc comment. Fufu~ I stabbed your new tests four times and each one bled exactly one death~ ### Verdict: ✅ Looks good to me~ #### What I verified this round~ - **The three dark arms of `_refreshTarget` now have their own pins** (`playback_controller_test.dart`): vanish-after-unjoin (404 → keep stale snapshot, `groupSize == 2` — not lost, not nulled), transient read failure (500 → snapshot untouched), and the race guard (`holdEntityRead` + gate → user's Office pick survives the stale Kitchen read, while `play_media` still lands on Kitchen as asked). All five arms of that method are now watched. - **Mutation probes, one per new test** (each RED on exactly its own test, green when restored): 1. `if (fresh == null) return;` with the guard dropped → only *"does not overwrite a speaker the user picks during the refresh"* fails. 2. `_target = null` on vanish (the "drop the snapshot" bug) → only *"keeps its last snapshot if the speaker vanishes after unjoin"* fails. 3. catch arm rethrows → only *"keeps its last snapshot if re-reading the speaker fails"* fails. 4. `retryTimeouts = true` (revert the no-retry gating) → only *"a service call with an explicit timeout is not re-sent on timeout"* fails. Four probes, four single-death tests. These are directional proofs, not tautologies ♪ - **The retry fix is principled, not just quieter**: `retryTimeouts = timeout == null` (backend:178) means only grouping calls with an explicit `timeout:` opt out of timeout-retry — every default-timeout call keeps the old 3-attempt behavior. `SocketException` retries are untouched. The doc comment now states the contract ("HA has already waited for the device, so the timeout is the verdict"), and the CHANGELOG line matches it. And the semantics are right: HA's `blocking=True` service call doesn't cancel server-side when the client gives up, so re-sending a `join` after 35 s really could re-group an already-regrouped house. - **The race test is deterministic**: the gated read parks on `await hold` while the unjoin response has already returned, so `selectTarget` lands strictly between read-start and read-resolve; the 5 ms poll loop only guards the cast setup, and the microtask (`gate.complete()`) resumes the read before any `Future.delayed` timer can fire. No flake window. - **Fake hygiene**: the new hooks are all opt-in (empty maps/sets default to inert), `removeAfterService` deletes *after* the 200 so the service call itself still records, and `holdService` gates before `rejectService` — the failure-injection semantics compose correctly. #### ✅ What I liked~ - You didn't just test the arms — you tested the *contract*: "stale snapshot kept, not lost" as a comment on the assertion itself. That's the sentence a future refactor needs to read ♡ - `expect(ha.calls, hasLength(1)) // no retry: the timeout is the verdict` — the comment is the doc comment's twin. Nothing drifts apart silently. - The `sonos_service_test` setUp refactor (`backend` lifted to a field for one test) instead of copy-pasting a second constructor — small, but it's the kind of small that keeps test files honest. Merge it when you're ready~ the speaker list `plex`-filter leftover stays documented in CLAUDE.md as agreed. --- *Automated review by Jibril · 2026-09-05* *CI/CD: absent for head SHA f9536f8 (workflow targets the GitHub remote; nothing ran on this Forgejo instance) · Local checks: pub get zero tree drift, flutter analyze 0 issues (Flutter 3.47.0), 49/49 tests pass, 4 mutation probes RED→restored→green, clone left pristine at f9536f8*

bjoern merged commit 240653f47a into main 2026-09-06 08:29:53 +02:00

bjoern deleted branch feat/casting-tests-ci-this-speaker-fix 2026-09-06 08:29:53 +02:00

bjoern referenced this pull request from a commit 2026-09-06 08:29:53 +02:00

Merge pull request 'Fix "This speaker" casting, add fake-HA casting tests, CI, and agent guide' (#1) from feat/casting-tests-ci-this-speaker-fix into main

jibril referenced this pull request 2026-09-10 22:48:48 +02:00

Group any set of Sonos speakers, and fade their volume together #2

scarlet referenced this pull request 2026-09-11 06:55:39 +02:00

Group any set of Sonos speakers, and fade their volume together #2

jibril referenced this pull request 2026-09-11 06:59:44 +02:00

Group any set of Sonos speakers, and fade their volume together #2

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

TeamAI/plex_sonos_caster!1

No description provided.