From 78c6c1bfcad60597d26afbc5290cb767ae65bb72 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 30 Aug 2026 22:13:55 +0200 Subject: [PATCH] fix(player): make Zone.js source zone candidates independently of Group projection Zone.js derived its "add speaker" candidate list from the `devices` prop, which is app.js's projected/collapsed device list. Once the stereo-pair projection (device_projection.go) started hiding a pair's non-master member from that list, it silently became impossible to add that physical device to an unrelated multiroom zone, even though the backend's HandleZoneAdd/HandleZoneRemove already operate on the raw device registry directly and never cared about pairing at all. Zone.js's own file wasn't touched by that change; its effective input just changed underneath it. Added GET /api/control/devices/{id}/zone/candidates, deliberately bypassing deviceViewSnapshot's projection and deliberately not excluding {id} itself -- which candidates to exclude is a caller concern (Zone.js already does this via the existing zoneIps set, which includes the zone master's own IP even when standalone, per models.ZoneInfo.IsStandalone). Zone and Group are separate, unrelated groupings and should stay that way. Co-Authored-By: Claude Sonnet 5 --- pkg/service/soundtouchweb/handler.go | 48 ++++++++++++ pkg/service/soundtouchweb/handler_test.go | 75 +++++++++++++++++++ pkg/service/soundtouchweb/mount.go | 1 + pkg/service/soundtouchweb/static/js/api.js | 1 + .../static/js/components/Zone.js | 20 +++-- 5 files changed, 139 insertions(+), 6 deletions(-) diff --git a/pkg/service/soundtouchweb/handler.go b/pkg/service/soundtouchweb/handler.go index 222e07c4..d6d29136 100644 --- a/pkg/service/soundtouchweb/handler.go +++ b/pkg/service/soundtouchweb/handler.go @@ -1099,6 +1099,54 @@ func (app *WebApp) HandleZoneLeave(w http.ResponseWriter, r *http.Request) { "Left zone") } +// HandleGetZoneCandidates returns every registered physical device, for the +// "add to zone" picker. Unlike HandleAPIDevices, this deliberately bypasses +// the stereo-pair projection (deviceViewSnapshot): Zone and Group are +// separate, unrelated groupings, and a device that's currently hidden as a +// stereo-pair member (see device_projection.go) must still be an +// independently addressable zone target, exactly as the backend +// HandleZoneAdd/HandleZoneRemove already treat it (both look devices up via +// the raw registry, unaffected by projection). +// +// Deliberately does not exclude the {id} device itself: which candidates to +// exclude (the page's own device, current zone members, ...) is a caller +// concern, not an inherent property of "what devices exist" -- excluding it +// here would make this endpoint silently unusable for any future caller +// that isn't Zone.js's current "add to my own zone" flow. +func (app *WebApp) HandleGetZoneCandidates(w http.ResponseWriter, r *http.Request) { + deviceID := chi.URLParam(r, "id") + + if _, exists := app.GetDevice(deviceID); !exists { + app.sendError(w, "Device not found", http.StatusNotFound) + return + } + + type zoneCandidate struct { + Info *models.DeviceInfo `json:"info,omitempty"` + } + + candidates := make(map[string]zoneCandidate) + + for _, entry := range app.DeviceSnapshot() { + if entry.Device == nil || entry.Device.DeviceInfo == nil { + continue + } + + candidates[entry.ID] = zoneCandidate{Info: entry.Device.DeviceInfo} + } + + w.Header().Set("Content-Type", "application/json") + + response := webtypes.APIResponse{ + Success: true, + Data: candidates, + } + + if err := json.NewEncoder(w).Encode(response); err != nil { + http.Error(w, "Failed to encode response", http.StatusInternalServerError) + } +} + // HandleDeviceRecents returns recently played items for a device. func (app *WebApp) HandleDeviceRecents(w http.ResponseWriter, r *http.Request) { deviceID := chi.URLParam(r, "id") diff --git a/pkg/service/soundtouchweb/handler_test.go b/pkg/service/soundtouchweb/handler_test.go index a556ead3..88ea9082 100644 --- a/pkg/service/soundtouchweb/handler_test.go +++ b/pkg/service/soundtouchweb/handler_test.go @@ -893,3 +893,78 @@ func TestHandleZoneLeave_UsesRemoveZoneSlave(t *testing.T) { t.Errorf("removeZoneSlave body should name the master, got: %s", masterBody) } } + +// TestHandleGetZoneCandidates_IncludesHiddenPairMemberAndSelf guards two +// things the stereo-pair projection (device_projection.go) must not affect: +// Zone and Group are separate, unrelated groupings, so (a) a stereo pair's +// hidden non-master member -- absent from the collapsed "devices" list -- +// must still be a valid zone-add candidate, matching how HandleZoneAdd +// already treats it (raw registry lookup, unaffected by projection); and +// (b) the endpoint itself does not exclude the requesting {id} device, +// since deciding what to exclude (this page's own device, current zone +// members, ...) is the caller's concern -- Zone.js already does this via +// the existing zoneIps set, which includes the master's own IP even for a +// standalone zone (see models.ZoneInfo.IsStandalone). +func TestHandleGetZoneCandidates_IncludesHiddenPairMemberAndSelf(t *testing.T) { + app := NewWebApp() + + group := testStereoGroup() + master := webtypes.NewDeviceConnection(nil, &models.DeviceInfo{DeviceID: "left-id", Name: "Living Room", IPAddress: "192.0.2.10"}) + master.SetStatus(&webtypes.DeviceStatus{IsConnected: true, Group: group}) + app.AddDevice("192.0.2.10", master) + + hiddenMember := webtypes.NewDeviceConnection(nil, &models.DeviceInfo{DeviceID: "right-id", Name: "Living Room", IPAddress: "192.0.2.11"}) + hiddenMember.SetStatus(&webtypes.DeviceStatus{IsConnected: true, Group: group}) + app.AddDevice("192.0.2.11", hiddenMember) + + standalone := webtypes.NewDeviceConnection(nil, &models.DeviceInfo{DeviceID: "kitchen-id", Name: "Kitchen", IPAddress: "192.0.2.12"}) + standalone.SetStatus(&webtypes.DeviceStatus{IsConnected: true}) + app.AddDevice("192.0.2.12", standalone) + + // Sanity check: the hidden member really is absent from the projected + // device list this test is guarding against leaking into. + projected := app.deviceViewSnapshot() + if _, visible := projected["192.0.2.11"]; visible { + t.Fatal("test setup: expected 192.0.2.11 to be hidden by the stereo-pair projection") + } + + req := httptest.NewRequest("GET", "/api/control/devices/192.0.2.10/zone/candidates", nil) + req = withChiParams(req, map[string]string{"id": "192.0.2.10"}) + w := httptest.NewRecorder() + + app.HandleGetZoneCandidates(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + + var response webtypes.APIResponse + if err := json.NewDecoder(w.Body).Decode(&response); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + + data, ok := response.Data.(map[string]interface{}) + if !ok { + t.Fatalf("expected data to be a map, got %T", response.Data) + } + + for _, ip := range []string{"192.0.2.10", "192.0.2.11", "192.0.2.12"} { + if _, ok := data[ip]; !ok { + t.Errorf("expected %s in zone candidates, got: %+v", ip, data) + } + } +} + +func TestHandleGetZoneCandidates_UnknownDeviceNotFound(t *testing.T) { + app := NewWebApp() + + req := httptest.NewRequest("GET", "/api/control/devices/192.0.2.99/zone/candidates", nil) + req = withChiParams(req, map[string]string{"id": "192.0.2.99"}) + w := httptest.NewRecorder() + + app.HandleGetZoneCandidates(w, req) + + if w.Code != http.StatusNotFound { + t.Fatalf("expected 404 for an unknown device, got %d: %s", w.Code, w.Body.String()) + } +} diff --git a/pkg/service/soundtouchweb/mount.go b/pkg/service/soundtouchweb/mount.go index cf0a5154..19b020fd 100644 --- a/pkg/service/soundtouchweb/mount.go +++ b/pkg/service/soundtouchweb/mount.go @@ -86,6 +86,7 @@ func (app *WebApp) MountWeb(r chi.Router, discoveryService *discovery.UnifiedDis r.Route("/zone", func(r chi.Router) { r.Get("/", app.HandleGetZone) + r.Get("/candidates", app.HandleGetZoneCandidates) r.Post("/add/{slaveId}", app.HandleZoneAdd) r.Post("/remove/{slaveId}", app.HandleZoneRemove) r.Post("/dissolve", app.HandleZoneDissolve) diff --git a/pkg/service/soundtouchweb/static/js/api.js b/pkg/service/soundtouchweb/static/js/api.js index 605c6ba4..f929663a 100644 --- a/pkg/service/soundtouchweb/static/js/api.js +++ b/pkg/service/soundtouchweb/static/js/api.js @@ -20,6 +20,7 @@ export const api = { power: (id) => req(`/api/control/devices/${id}/power`, { method: 'POST' }), recents: (id) => req(`/api/control/devices/${id}/recents`), zone: (id) => req(`/api/control/devices/${id}/zone`), + zoneCandidates: (id) => req(`/api/control/devices/${id}/zone/candidates`), zoneAdd: (masterId, slaveId) => req(`/api/control/devices/${masterId}/zone/add/${slaveId}`, { method: 'POST' }), zoneRemove: (masterId, slaveId) => req(`/api/control/devices/${masterId}/zone/remove/${slaveId}`, { method: 'POST' }), zoneDissolve: (id) => req(`/api/control/devices/${id}/zone/dissolve`, { method: 'POST' }), diff --git a/pkg/service/soundtouchweb/static/js/components/Zone.js b/pkg/service/soundtouchweb/static/js/components/Zone.js index c32dedf4..4529fb4c 100644 --- a/pkg/service/soundtouchweb/static/js/components/Zone.js +++ b/pkg/service/soundtouchweb/static/js/components/Zone.js @@ -7,13 +7,17 @@ const html = htm.bind(h); export function Zone({ deviceId, devices }) { const [zone, setZone] = useState(null); + const [candidates, setCandidates] = useState({}); const [loading, setLoading] = useState(true); const [showPicker, setShowPicker] = useState(false); function refresh() { - api.zone(deviceId).then(resp => { - if (resp.success) setZone(resp.data); - }).finally(() => setLoading(false)); + Promise.all([api.zone(deviceId), api.zoneCandidates(deviceId)]) + .then(([zoneResp, candidatesResp]) => { + if (zoneResp.success) setZone(zoneResp.data); + if (candidatesResp.success) setCandidates(candidatesResp.data || {}); + }) + .finally(() => setLoading(false)); } useEffect(() => { refresh(); }, [deviceId]); @@ -48,11 +52,15 @@ export function Zone({ deviceId, devices }) { if (!zone) return null; - // Devices not already in the zone are available to add + // Devices not already in the zone are available to add. Sourced from a + // dedicated candidates endpoint rather than the `devices` prop: Zone and + // Group are separate, unrelated groupings, so a stereo pair's hidden + // member -- absent from the collapsed device list -- is still a valid, + // independent zone-add target. const zoneIps = new Set([zone.masterIp, ...(zone.members || []).map(m => m.ip)].filter(Boolean)); - const available = Object.entries(devices || {}).filter(([ip]) => !zoneIps.has(ip)); + const available = Object.entries(candidates).filter(([ip]) => !zoneIps.has(ip)); - const deviceName = (ip) => devices[ip]?.info?.name || ip; + const deviceName = (ip) => devices[ip]?.info?.name || candidates[ip]?.info?.name || ip; return html`