From b861c11d375eba8e400d13270de89f45d69d82cc Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Sun, 7 Jun 2026 10:22:36 +0200 Subject: [PATCH] feat(web): remove devices from the player UI (refs #451) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The merge of soundtouch-web into soundtouch-service was asymmetric: manual device *adds* propagated to the player UI (HandleAddManualDevice notifies, the hook re-seeds + broadcasts), but *removals* did not. The datastore-removal handler never notified, and the web registry's sync only ever added entries — its map was append-only, so a removed device lingered in the player UI until restart. This adds the missing removal path: - DELETE /api/control/devices/{id} (HandleDeleteDevice). The registry is keyed by host/IP; the datastore by device ID (MAC), so the handler resolves one to the other via the connection's DeviceInfo, cascades to the datastore through a new RemoveDeviceHook (embedded build only), prunes the in-memory entry, and broadcasts the updated list. - WebApp.RemoveDevice prunes the registry and stops the per-device goroutines (status poller + WebSocket reconnect loop) via a new done-channel + Close() on DeviceConnection — previously both ran for the life of the process. - Server.RemoveDeviceByID extracts the cross-account lookup + remove from HandleRemoveDevice and now fires notifyDevicesChanged, so the admin Devices tab removal also propagates to the player UI. - Player UI: a quiet per-card Remove control (visible on hover), a confirm dialog, optimistic prune, and a note that a still-online device may reappear after the next discovery scan (honest v1 — no ignore-list). Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/soundtouch-service/main.go | 7 ++ .../testdata/router_routes.txt | 1 + .../handlers/handlers_device_remove_test.go | 72 +++++++++++++++++ pkg/service/handlers/handlers_setup.go | 47 ++++++----- pkg/service/soundtouchweb/discovery.go | 9 ++- pkg/service/soundtouchweb/handler.go | 79 ++++++++++++++++++- pkg/service/soundtouchweb/mount.go | 1 + pkg/service/soundtouchweb/registry_test.go | 75 ++++++++++++++++++ pkg/service/soundtouchweb/static/css/app.css | 29 ++++++- pkg/service/soundtouchweb/static/js/api.js | 1 + pkg/service/soundtouchweb/static/js/app.js | 20 +++++ .../static/js/components/DeviceList.js | 19 +++-- pkg/service/soundtouchweb/websocket.go | 32 +++++++- pkg/service/soundtouchweb/webtypes/types.go | 31 ++++++++ 14 files changed, 393 insertions(+), 30 deletions(-) create mode 100644 pkg/service/handlers/handlers_device_remove_test.go diff --git a/cmd/soundtouch-service/main.go b/cmd/soundtouch-service/main.go index 9090aff..10cf7ae 100644 --- a/cmd/soundtouch-service/main.go +++ b/cmd/soundtouch-service/main.go @@ -1125,6 +1125,13 @@ func newEmbeddedWebApp(server *handlers.Server, serverURL string, ds *datastore. // UI "discover" runs the service's sweep, not a second mDNS stack. webApp.TriggerDiscovery = server.DiscoverDevices + // A removal from the player UI cascades to the datastore (the single + // source of truth), so the device does not reappear on the next re-sync. + webApp.RemoveDeviceHook = func(deviceID string) error { + _, err := server.RemoveDeviceByID(deviceID) + return err + } + // Keep the UI registry live as the service discovers or devices are added. server.SetDevicesChangedHook(func() { webApp.SeedExtraDevices() diff --git a/cmd/soundtouch-service/testdata/router_routes.txt b/cmd/soundtouch-service/testdata/router_routes.txt index b8227cf..af7f022 100644 --- a/cmd/soundtouch-service/testdata/router_routes.txt +++ b/cmd/soundtouch-service/testdata/router_routes.txt @@ -4,6 +4,7 @@ DELETE /accounts/{account}/devices/{device} handlers.( DELETE /accounts/{account}/group handlers.(*Server).HandleUnsupported-fm DELETE /accounts/{account}/group/ handlers.(*Server).HandleUnsupported-fm DELETE /accounts/{account}/group/{groupId} handlers.(*Server).HandleUnsupported-fm +DELETE /api/control/devices/{id}/ soundtouchweb.(*WebApp).HandleDeleteDevice-fm DELETE /api/setup/devices/{deviceId} handlers.(*Server).HandleRemoveDevice-fm DELETE /api/setup/dns-discoveries handlers.(*Server).HandleClearDNSDiscoveries-fm DELETE /api/setup/interactions/sessions handlers.(*Server).HandleCleanupSessions-fm diff --git a/pkg/service/handlers/handlers_device_remove_test.go b/pkg/service/handlers/handlers_device_remove_test.go new file mode 100644 index 0000000..6eca2d2 --- /dev/null +++ b/pkg/service/handlers/handlers_device_remove_test.go @@ -0,0 +1,72 @@ +package handlers + +import ( + "testing" + + "github.com/gesellix/bose-soundtouch/pkg/models" + "github.com/gesellix/bose-soundtouch/pkg/service/datastore" +) + +// TestRemoveDeviceByID verifies the extracted removal helper deletes the +// device from the datastore, fires the devices-changed hook (so the +// embedded web UI re-syncs), and reports not-found without firing the +// hook when no matching device exists. +func TestRemoveDeviceByID(t *testing.T) { + ds := datastore.NewDataStore(t.TempDir()) + server := NewServer(ds, nil, "http://127.0.0.1:8000", false, false, false) + + const ( + account = "1000001" + deviceID = "DEVICEID01" + ) + + if err := ds.SaveDeviceInfo(account, deviceID, &models.ServiceDeviceInfo{ + DeviceID: deviceID, + Name: "Test Speaker", + IPAddress: "192.0.2.10", + }); err != nil { + t.Fatalf("SaveDeviceInfo: %v", err) + } + + var hookFired int + + server.SetDevicesChangedHook(func() { hookFired++ }) + + found, err := server.RemoveDeviceByID(deviceID) + if err != nil { + t.Fatalf("RemoveDeviceByID: %v", err) + } + + if !found { + t.Fatal("RemoveDeviceByID reported the device as not found") + } + + if hookFired != 1 { + t.Errorf("devices-changed hook fired %d times; want 1", hookFired) + } + + devices, err := ds.ListAllDevices() + if err != nil { + t.Fatalf("ListAllDevices: %v", err) + } + + for i := range devices { + if devices[i].DeviceID == deviceID { + t.Error("device still present in datastore after removal") + } + } + + // A second removal finds nothing and must not fire the hook again. + found, err = server.RemoveDeviceByID(deviceID) + if err != nil { + t.Fatalf("RemoveDeviceByID (second call): %v", err) + } + + if found { + t.Error("RemoveDeviceByID reported a removed device as found") + } + + if hookFired != 1 { + t.Errorf("devices-changed hook fired %d times after no-op removal; want 1", hookFired) + } +} diff --git a/pkg/service/handlers/handlers_setup.go b/pkg/service/handlers/handlers_setup.go index 1076f03..f36f400 100644 --- a/pkg/service/handlers/handlers_setup.go +++ b/pkg/service/handlers/handlers_setup.go @@ -101,6 +101,34 @@ func (s *Server) HandleGetDiscoveryStatus(w http.ResponseWriter, _ *http.Request } } +// RemoveDeviceByID removes the device with the given device ID (MAC) from +// the datastore, searching across all accounts. It returns whether a +// matching device was found. On a successful removal it notifies +// observers (e.g. the embedded web UI) so they re-sync — the symmetric +// counterpart to the notify in HandleAddManualDevice. +func (s *Server) RemoveDeviceByID(deviceID string) (bool, error) { + devices, err := s.ds.ListAllDevices() + if err != nil { + return false, err + } + + for i := range devices { + if devices[i].DeviceID == deviceID { + if err := s.ds.RemoveDevice(devices[i].AccountID, devices[i].DeviceID); err != nil { + return false, err + } + + // Let any observer (e.g. the embedded web UI) re-sync from the + // datastore so the removal propagates to the player UI. + s.notifyDevicesChanged() + + return true, nil + } + } + + return false, nil +} + // HandleRemoveDevice removes a device from the datastore. func (s *Server) HandleRemoveDevice(w http.ResponseWriter, r *http.Request) { deviceId := chi.URLParam(r, "deviceId") @@ -109,29 +137,12 @@ func (s *Server) HandleRemoveDevice(w http.ResponseWriter, r *http.Request) { return } - // Find which account this device belongs to. - devices, err := s.ds.ListAllDevices() + found, err := s.RemoveDeviceByID(deviceId) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return } - var found bool - - for i := range devices { - if devices[i].DeviceID == deviceId { - err = s.ds.RemoveDevice(devices[i].AccountID, devices[i].DeviceID) - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - - found = true - - break - } - } - if !found { http.Error(w, "Device not found", http.StatusNotFound) return diff --git a/pkg/service/soundtouchweb/discovery.go b/pkg/service/soundtouchweb/discovery.go index 1da64f1..4d27c3d 100644 --- a/pkg/service/soundtouchweb/discovery.go +++ b/pkg/service/soundtouchweb/discovery.go @@ -78,8 +78,13 @@ func (app *WebApp) AddDeviceByHost(host string, port int, source string) { ticker := time.NewTicker(30 * time.Second) defer ticker.Stop() - for range ticker.C { - app.UpdateDeviceStatus(host, conn) + for { + select { + case <-ticker.C: + app.UpdateDeviceStatus(host, conn) + case <-conn.Done(): + return + } } }() diff --git a/pkg/service/soundtouchweb/handler.go b/pkg/service/soundtouchweb/handler.go index 4eb269d..12df157 100644 --- a/pkg/service/soundtouchweb/handler.go +++ b/pkg/service/soundtouchweb/handler.go @@ -63,6 +63,13 @@ type WebApp struct { // soundtouch-web leaves it nil and runs its own sweep. TriggerDiscovery func(ctx context.Context) + // RemoveDeviceHook, when set, removes a device from the backing store by + // its device ID (MAC). The embedded build wires it to the service's + // datastore removal so a removal from the player UI also clears the + // persisted device; standalone soundtouch-web leaves it nil (no store, so + // removal only prunes the in-memory registry). + RemoveDeviceHook func(deviceID string) error + discoveryStatus atomic.Value // stores *webtypes.DiscoveryStatus } @@ -108,8 +115,9 @@ func (app *WebApp) GetDevice(id string) (*webtypes.DeviceConnection, bool) { // DeviceSnapshot returns a list of (id, *DeviceConnection) pairs taken // under a single read lock. Callers can iterate the result without // holding any registry lock. Devices added or removed after the call -// are not reflected; the pointers themselves remain valid because -// nothing deletes from the underlying map today. +// are not reflected. A pointer captured here stays valid even if the +// device is later removed (RemoveDevice only detaches it from the map +// and stops its goroutines), so iterating a stale snapshot is safe. func (app *WebApp) DeviceSnapshot() []DeviceEntry { app.devicesMu.RLock() defer app.devicesMu.RUnlock() @@ -165,6 +173,27 @@ func (app *WebApp) TouchDevice(id string) bool { return true } +// RemoveDevice removes the device registered under id and stops its +// background goroutines (status poller + WebSocket reconnect loop) via +// conn.Close. Returns true if id was present. Close runs outside the +// registry lock because it performs network I/O (WebSocket disconnect). +func (app *WebApp) RemoveDevice(id string) bool { + app.devicesMu.Lock() + + conn, ok := app.devices[id] + if ok { + delete(app.devices, id) + } + + app.devicesMu.Unlock() + + if ok { + conn.Close() + } + + return ok +} + // HandleAPIDevices returns all devices as JSON func (app *WebApp) HandleAPIDevices(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -228,6 +257,52 @@ func (app *WebApp) HandleAPIDevice(w http.ResponseWriter, r *http.Request) { } } +// HandleDeleteDevice removes a device from the registry and, in the +// embedded build, from the service datastore. The registry is keyed by +// host/IP (the {id} URL param); the datastore is keyed by device ID +// (MAC), so we resolve one to the other via the connection's DeviceInfo +// before cascading. A device still live on the network is re-discovered +// on the next sweep — removal is "remove now", not a permanent ban. +func (app *WebApp) HandleDeleteDevice(w http.ResponseWriter, r *http.Request) { + host := chi.URLParam(r, "id") + if host == "" { + app.sendError(w, "Device ID required", http.StatusBadRequest) + return + } + + conn, exists := app.GetDevice(host) + if !exists { + app.sendError(w, "Device not found", http.StatusNotFound) + return + } + + // Cascade to the backing store (embedded build only). Standalone + // soundtouch-web has no datastore and leaves the hook nil, so removal + // only prunes the in-memory registry below. + if app.RemoveDeviceHook != nil { + deviceID := "" + if conn.DeviceInfo != nil { + deviceID = conn.DeviceInfo.DeviceID + } + + if err := app.RemoveDeviceHook(deviceID); err != nil { + log.Printf("Failed to remove device %s from store: %v", sanitizeLog(host), err) + app.sendError(w, "Failed to remove device from store", http.StatusBadGateway) + + return + } + } + + app.RemoveDevice(host) + app.BroadcastDeviceList() + + w.Header().Set("Content-Type", "application/json") + + if err := json.NewEncoder(w).Encode(webtypes.APIResponse{Success: true}); err != nil { + http.Error(w, "Failed to encode response", http.StatusInternalServerError) + } +} + // HandleAPIControl handles device control commands func (app *WebApp) HandleAPIControl(w http.ResponseWriter, r *http.Request) { deviceID := chi.URLParam(r, "id") diff --git a/pkg/service/soundtouchweb/mount.go b/pkg/service/soundtouchweb/mount.go index aaf966b..222705e 100644 --- a/pkg/service/soundtouchweb/mount.go +++ b/pkg/service/soundtouchweb/mount.go @@ -63,6 +63,7 @@ func (app *WebApp) MountWeb(r chi.Router, discoveryService *discovery.UnifiedDis r.Route("/{id}", func(r chi.Router) { r.Get("/", app.HandleAPIDevice) + r.Delete("/", app.HandleDeleteDevice) r.Post("/key/{key}", app.HandleDeviceKey) r.Post("/volume/{volume}", app.HandleDirectVolumeControl) r.Post("/power", app.HandleDevicePower) diff --git a/pkg/service/soundtouchweb/registry_test.go b/pkg/service/soundtouchweb/registry_test.go index e014c6e..e70a169 100644 --- a/pkg/service/soundtouchweb/registry_test.go +++ b/pkg/service/soundtouchweb/registry_test.go @@ -117,6 +117,81 @@ func TestDeviceSnapshotAndCount(t *testing.T) { } } +func TestRemoveDevice(t *testing.T) { + app := NewWebApp() + + if app.RemoveDevice("missing") { + t.Error("RemoveDevice returned true for unknown id") + } + + conn := newRegistryDevice("first") + app.AddDevice("host-1", conn) + + if !app.RemoveDevice("host-1") { + t.Fatal("RemoveDevice returned false for known id") + } + + if _, ok := app.GetDevice("host-1"); ok { + t.Error("device still present after RemoveDevice") + } + + if got := app.DeviceCount(); got != 0 { + t.Errorf("DeviceCount after removal = %d; want 0", got) + } + + // Removing the same id again is a no-op. + if app.RemoveDevice("host-1") { + t.Error("RemoveDevice returned true on second removal") + } + + // The connection's done channel must be closed so its background + // goroutines (status poll, WebSocket reconnect) stop. + select { + case <-conn.Done(): + default: + t.Error("RemoveDevice did not close the connection's Done channel") + } +} + +// TestRemoveDeviceConcurrent runs adds and removes of the same ids from +// many goroutines under `go test -race` to confirm the registry stays +// race-free when removal is in the mix. +func TestRemoveDeviceConcurrent(t *testing.T) { + app := NewWebApp() + + const ( + workers = 16 + opsPerWorker = 200 + ) + + var wg sync.WaitGroup + + wg.Add(workers * 2) + + for w := 0; w < workers; w++ { + go func(worker int) { + defer wg.Done() + + for i := 0; i < opsPerWorker; i++ { + id := fmt.Sprintf("w%d-%d", worker, i) + app.AddDevice(id, newRegistryDevice(id)) + } + }(w) + } + + for w := 0; w < workers; w++ { + go func(worker int) { + defer wg.Done() + + for i := 0; i < opsPerWorker; i++ { + app.RemoveDevice(fmt.Sprintf("w%d-%d", worker, i)) + } + }(w) + } + + wg.Wait() +} + // TestRegistryConcurrent exercises the registry from many goroutines // at once. Before the introduction of devicesMu this would either // panic with "fatal error: concurrent map read and map write" or be diff --git a/pkg/service/soundtouchweb/static/css/app.css b/pkg/service/soundtouchweb/static/css/app.css index 6c8f673..894595c 100644 --- a/pkg/service/soundtouchweb/static/css/app.css +++ b/pkg/service/soundtouchweb/static/css/app.css @@ -312,8 +312,35 @@ img { display: block; max-width: 100%; } } .device-card:hover { box-shadow: 0 4px 12px rgba(0,0,0,.12); transform: translateY(-1px); } -.device-header { display: flex; align-items: center; justify-content: space-between; margin-bottom: .25rem; } +.device-header { display: flex; align-items: center; justify-content: space-between; margin-bottom: .25rem; gap: .5rem; } .device-name { font-weight: 600; font-size: .95rem; } +.device-header-right { display: flex; align-items: center; gap: .5rem; flex-shrink: 0; } + +/* Quiet remove affordance: invisible until the card is hovered, then dim, + warming to red only on its own hover. Keeps the grid relaxed. */ +.device-remove { + border: 0; + background: none; + padding: 0; + width: 1.1rem; + height: 1.1rem; + line-height: 1; + font-size: .8rem; + color: var(--text-dim); + cursor: pointer; + opacity: 0; + transition: opacity .15s, color .15s; +} +.device-card:hover .device-remove { opacity: .55; } +.device-remove:hover { opacity: 1; color: var(--offline); } +.device-remove:focus-visible { opacity: 1; outline: 2px solid var(--offline); outline-offset: 2px; } + +.device-list-note { + margin: .9rem .15rem 0; + font-size: .78rem; + color: var(--text-dim); + line-height: 1.4; +} .device-type { font-size: .8rem; color: var(--text-dim); margin-bottom: .5rem; display: flex; gap: .4rem; flex-wrap: wrap; } .device-ip { color: var(--text); font-family: monospace; font-weight: 500; } diff --git a/pkg/service/soundtouchweb/static/js/api.js b/pkg/service/soundtouchweb/static/js/api.js index 12bccd4..e89d6b2 100644 --- a/pkg/service/soundtouchweb/static/js/api.js +++ b/pkg/service/soundtouchweb/static/js/api.js @@ -8,6 +8,7 @@ async function req(url, opts = {}) { export const api = { devices: () => req('/api/control/devices'), device: (id) => req(`/api/control/devices/${id}`), + removeDevice: (id) => req(`/api/control/devices/${id}`, { method: 'DELETE' }), discover: () => req('/api/control/discover', { method: 'POST' }), key: (id, key) => req(`/api/control/devices/${id}/key/${key}`, { method: 'POST' }), volume: (id, level) => req(`/api/control/devices/${id}/volume/${level}`, { method: 'POST' }), diff --git a/pkg/service/soundtouchweb/static/js/app.js b/pkg/service/soundtouchweb/static/js/app.js index cd1a517..6474fb5 100644 --- a/pkg/service/soundtouchweb/static/js/app.js +++ b/pkg/service/soundtouchweb/static/js/app.js @@ -148,6 +148,25 @@ function App() { await api.discover(); } + async function removeDevice(id) { + const name = devices[id]?.info?.name || id; + if (!confirm(`Remove "${name}"?\n\nThis clears it from AfterTouch. A device still online may reappear after the next discovery scan.`)) { + return; + } + // Optimistically drop it; the server's devices broadcast reconciles. + setDevices(prev => { + const next = { ...prev }; + delete next[id]; + return next; + }); + try { + const resp = await api.removeDevice(id); + showToast(resp?.success ? `Removed "${name}"` : (resp?.error || 'Failed to remove device')); + } catch (err) { + showToast('Failed to remove device'); + } + } + return html`