Adding urls.validate() to setAllBoseURLsViaTelnet made a rejected URL come
back through resyncBoseURLsAfterXML as "could not re-sync boseurls over
telnet", which reads as the device being unreachable and sends the user to
look at port 17000.
applyURLOverrides lets the XML migration accept URLs the telnet validator now
refuses, a query string for instance, so this is reachable: the XML write
succeeds with that value while the runtime re-sync is silently skipped. A
reboot does reconcile from the XML, so the outcome is fine; the diagnosis was
not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A rejected or unrecognised command response aborted the sequence with
"read back and reconcile all four URL fields before rebooting", leaving the
user to do by hand what the service can do in one read-only command.
That advice also assumes the write failed, which the reply shape does not
prove. A telnet console is a shared stream and firmware echoes vary: an
interleaved log line, a normalised URL, or a banner arriving late all produce
a response the parser does not recognise, for a write that landed. This is the
same class as the earlier parser reporting HTTP 500 when all four writes had
succeeded, just narrower.
The abort itself is kept, so no further write is sent and a rejected sequence
cannot spread. Before returning, `getpdo CurrentSystemConfiguration` now runs
and the reported error carries what the device actually holds. Nothing is
claimed to have succeeded on ambiguous evidence; the user simply gets the
evidence.
The two tests asserting that nothing at all follows a rejection now assert the
property that matters, that no further command CHANGES the device, since a
read-only read-back does not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
telnetRevertAvailable compared the live URLs against canonicalBoseTelnetURLs
and reported "revertable" on any difference. That set is one original variant,
not the only one: this repo's own model of a factory speaker
(pkg/service/testing/fakespeaker) uses stats.bose.com and bmxservice.bose.com
where the canonical set has events.api.bosecm.com and content.api.bose.io.
Feeding that fixture to the gate returned true, so the web UI offered
"Restore Bose URLs via Telnet" on a pristine speaker. Pressing it rewrites the
device's genuine factory URLs and commits them through envswitch, the layer
that wins on the next reboot. The speaker keeps working, since both host sets
point at the shut-down Bose cloud, but the record of what that device's URLs
actually were is gone, and telnet migration takes no backup to recover it from.
The question the gate should answer is "has this been changed away from a
factory configuration", not "does it differ from our canonical set". It now
compares each field against the values observed on unmigrated speakers,
normalising case and a trailing slash so firmware echoing does not decide it.
Only observed values are listed. Other Bose hostnames appear in the DNS
interception lists and in DNS recordings, but those capture hosts a speaker
resolves at runtime rather than the configured value of these four fields, and
a wrong entry would hide the revert from someone who needs it.
The existing test could not catch this: it asserts against the same canonical
constants the code was comparing with.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Expose telnet-only URL restore through the CLI, setup API, and web UI. Validate command-safe URLs, serialize per-speaker mutations, and verify runtime readback while reporting partial-state failures.
The readiness verdict was only reachable by attempting the migration, so the
UI's pre-flight panel could show every check green and then fail at Apply
with a 409. The user commits to the operation before learning it will be
refused.
GetMigrationSummary now runs the same read-only check and reports it:
data_ready_error carries the reason migration would be refused, and
data_ready_warnings carries the advisory ones. Nothing is enforced here, and
MigrateSpeaker still runs the check itself, so this cannot let a refused
migration through.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The GetExactDeviceInfo error was discarded and every failure reported as
"DeviceInfo.xml is not persisted", so a malformed file or an I/O error was
diagnosed as a missing sync and the user was told to run Data Sync, which is
the wrong remedy for either.
Include the cause, as the neighbouring branches already do.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mapPresetsToFullResponse omits a preset whose source is absent from the
account's configured sources and cannot be synthesised. The readiness check
then found the speaker holding a slot /full does not, refused migration, and
attached its default action: "Run Data Sync for this device and retry
migration".
Syncing cannot add a missing music service source, so the user looped with no
override and no path forward.
compareMigrationPresets now reports whether the speaker's own view holds a
slot the rendered account lacks, which is the signature of that deliberate
omission, and that case gets an action naming the real remedy: re-link or
repopulate the source. Every other mismatch keeps the sync advice, which is
still right for a stale snapshot.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Clearing a slot through the Marge API leaves a zero-value entry in the list
(RemovePreset assigns models.ServicePreset{}), which savePresetsNoLock
persists as <preset id=""> with no filtering. Reading it back gives an empty
slot, while mapPresetsToFullResponse drops it from the rendered /full.
The comparison then refused migration with either "contains a preset without
a slot" or a count mismatch, for a datastore that was otherwise perfectly in
sync, and Data Sync could not fix it because nothing was actually wrong.
An empty slot carries no identity to compare, so skip it on both sides.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Migration was refused whenever the rendered account held more than one
device. One account holding every speaker in the household is the normal Bose
arrangement, so this blocked most setups, and there was no override: the
check runs unconditionally at the top of MigrateSpeaker.
It also misfired on genuinely single-speaker setups.
handleDiscoveredDeviceFallback writes a second device directory keyed by the
host address under the same account whenever /info momentarily fails, and its
cleanup only runs when d.SerialNo is set, which discovery never populates. A
stale entry left behind by a DHCP lease change then blocked migration
permanently.
The evidence does not support a hard block either. Issue #614 concluded the
shared-account preset wipe is empirical rather than a proven mechanism, with
the root cause still open, and the troubleshooting entry added there is
labelled a workaround. The guide's own remediation was unreachable in normal
use, since discovery re-adds the other devices.
The check now reports it as a warning, naming the device count, carried into
the migration log the UI already shows alongside its other "Warning:" lines.
The provable checks (persisted snapshot present and valid, presets equal
across snapshot, live /presets and rendered /full) still refuse.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The readiness check refused when the live /info carried no
margeAccountUUID, which is exactly the state a factory-reset speaker is in.
That made the documented onboarding impossible. The admin UI migrates first
and pairs afterwards (see "Pairing runs after the URL flip" in the setup
page), and MIGRATION-GUIDE.md step 4 tells the user to Generate an account ID
on a factory-reset device. Both now hit a 409 before pairing can run. The
suggested remedy could not help either: SyncDeviceData files an account-less
device under "default", which never matches an empty live account, so the
user had no way forward at all.
An unpaired speaker has no account data to preserve, so there is nothing for
this check to compare and nothing to lose. Skip it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The player reloaded itself five seconds after its status socket closed. That
cannot work while the service is down, because the document is served by that
same service: the tab left a working UI for the browser's error page, lost
whatever it held (a pending source command, the selected device, scroll
position) and stayed there until reloaded by hand.
The socket now reconnects with exponential backoff, from 1s to 15s, and a
banner says the connection was lost. The page stays usable and recovers on
its own when the service returns, with no interaction.
This is only safe because of the epoch added alongside the source-selection
work. A restarted service publishes revisions from 0 again, and revisions are
only comparable within one epoch; without it a reconnected socket would
deliver a sequence the browser rejects forever, leaving the page silently
frozen. Reloading was presumably how that was avoided before.
The regression test drives the outage through a TCP proxy it can take down
and bring back at the same address. Simulating this needs both refusing new
connections and severing established ones: a server that stops accepting
leaves an open WebSocket running, and Chrome's offline emulation does not
close it either, so neither reproduces a service that went away. Against the
old behaviour the test fails with "Inspected target navigated or closed",
which is the reload destroying the page.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TUNEIN was routed like RADIO_BROWSER on the strength of
stations.ResolveContentItem handling them identically, without a hardware
check. Resuming it from Recents has since been confirmed to play the station
as intended, so the comment no longer presents it as inferred.
ALEXA cannot be tested on the hardware available, so it is not a pending
question but a standing decision: leave it unlisted rather than guess at its
behaviour and risk breaking a source that works today. isStubNowPlaying
covers it, reporting a failure if a bare select does strand it.
Comments and documentation only.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SOURCE-SELECTION.md listed TUNEIN, RADIO_BROWSER and STORED_MUSIC among the
sources selectable with a source and account alone, and showed SelectTuneIn
as an example. That is the trap: a speaker answers 200 for such a select,
parks on a stub now-playing, and carries on playing whatever it was playing,
so callers checking /now_playing see the source they asked for while the
audio is something else.
Splits the list into sources a bare select works for and sources that need a
ContentItem with a Location, replaces the SelectTuneIn example with
SelectContentItem, and carries the same warning into the doc comments on
SelectSource and SelectTuneIn, where a developer reaching for them will
actually see it. Also points the player's README at the new reference.
No behaviour change; comments and documentation only.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A STORED_MUSIC entry names a media server, not something to play: its
sourceAccount is a server UDN, and there is one entry per server. Selecting
it identifies no track or container, so browsing is the only meaningful
action.
Clicking one now opens the Library. It never resumes from Recents either:
even a resumable album there is a worse guess than showing the user what is
on the server. The clicked server is not carried over, matching how the other
browser pages let you pick a device rather than inheriting one.
Servers that are offline never reach this path anyway: the speaker reports
them status="UNAVAILABLE" and the source list only renders READY entries.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Clicking it played the AfterTouch notification ding. That was the newest
Recents entry for the source, and resuming the newest entry is what the
provider path does.
The source is not like the other two. AfterTouch plays its own one-shot audio
through it: TTS and the ding both go out over /custom/v1/playback/. So its
Recents mix notifications with stations, and the newest entry is as likely to
be a ding as anything worth replaying. On the test speaker the only
LOCAL_INTERNET_RADIO recent WAS the ding.
The announcement proxy path is distinguishable from Play URL's
BuildOrionLocation, so filtering was possible, but the same path also carries
URLs played through the CLI, and any future feature that injects audio would
have to remember to stay out of it. Opening Play URL unconditionally, which
is the page that emits content for this source, does not depend on being able
to classify what happens to be in Recents.
RADIO_BROWSER and TUNEIN keep resuming: nothing writes one-shot audio to
them, and on the test speaker their recents held real stations.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PROVIDER_SOURCES covers the sources known to produce the stub now-playing.
It cannot cover the ones we have not tested, ALEXA among them, and a source
list is whatever the speaker chooses to advertise.
So the readback now refuses to confirm the stub itself, wherever it comes
from: a now-playing that names the source we asked for but reports no
location, no play status, and an item name echoing the source is reported as
a failure rather than a success. That shape is what a speaker returns for a
select it accepted but cannot act on.
All three conditions are required together. A physical input reports no
location and no item name of its own yet is genuinely playing, so any one
condition alone would reject real selections; a test covers exactly that
case, confirming with PlayStatus set and no location.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Confirmed on real hardware: selecting it bare produces the byte-identical
stub RADIO_BROWSER produced, with the source name echoed as the item name,
empty type and location, isPresetable false, and PlayStatus empty, while the
previously playing stream carries on. Nothing about the speaker's audio
changes; only /now_playing does.
Its fallback browser is Play URL, which is the page in this app that emits
that source: HandlePlayURL builds a ContentItem with
Source "LOCAL_INTERNET_RADIO".
ALEXA is advertised READY as well but stays unlisted: whether a bare select
resumes anything for it is still unverified.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A speaker advertises RADIO_BROWSER and TUNEIN in /sources with
status="READY", so the player rendered them as ordinary source buttons and
issued the same bare /select it uses for AUX or SPOTIFY: source and account,
no ContentItem.
They are not selectable inputs. Playing one needs a station ContentItem
carrying a Location, which is what stations.ResolveContentItem builds and
what HandlePlayRadioBrowser sends. Given a bare select the speaker accepts
the command and parks on a stub now-playing instead, observed on real
hardware:
<nowPlaying source="RADIO_BROWSER" sourceAccount="">
<ContentItem source="RADIO_BROWSER" type="" location="" isPresetable="false">
<itemName>RADIO_BROWSER</itemName>
</ContentItem>
</nowPlaying>
Empty type, empty location, itemName echoing the source name, and no
playStatus, while the previous audio keeps playing. The speaker then reports
that stub indefinitely, so the player shows RadioBrowser while Spotify is
audible. Worse, the readback sees the source it asked for and confirms
"Source selected" for a command that produced a dead state.
Clicking such a source now resumes its most recent station, using the
Recents entry's own ContentItem, which is the real item the speaker was
given and carries the Location a bare select cannot supply. With nothing to
resume, or if the lookup fails, the click navigates to that provider's
browser rather than issuing a select known to strand the speaker.
Only RADIO_BROWSER and TUNEIN are treated this way. LOCAL_INTERNET_RADIO and
ALEXA are advertised READY too, but whether a bare select resumes anything
for them is unverified, so they keep today's behaviour.
api.playChecked mirrors api.selectSource: the command path needs a write
whose failure it can see, while api.play keeps its response-level behaviour
for the callers that already rely on it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Selecting a source cost up to ~21 speaker requests. Two multipliers, both
removed here.
A match on the first readback only marked the command provisional and left
the later deadlines running, so the happy path always spent all three. The
reason to keep watching is real: /select answers 200 even for a source the
speaker rejects seconds later, surfacing as a transition to an error source.
But the event stream already reports that transition as it happens, and the
effect watching nowPlayingUpdated already turns it into a failure. Polling
on top is re-asking a question we are subscribed to the answer of. The
remaining readbacks are now kept only when the readback itself reports no
live event stream, which is the case they are actually needed for.
Each readback also fetched the whole device, and HandleAPIDevice runs a full
UpdateDeviceStatus: six sequential speaker calls plus /getGroup on a
stereo-capable model, to answer one question, against a device the readback
may be checking on precisely because it is slow. GET
/devices/{id}/now-playing refreshes only /now_playing and returns the same
shape, under FieldNowPlaying's generation so it still orders against push
events and concurrent polls, and reporting to the health tracker like any
other HTTP round.
A confirmed selection on a speaker with a live event stream now costs one
speaker request instead of about twenty-one. A speaker whose events are not
arriving keeps the full three-readback window, at one request each.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A failed /sources read went through CompleteFieldPoll, so it consumed the
field generation. That let a newer failed read discard an older, still
in-flight read that had succeeded, and since the player renders
disabled=${sourcesStale}, one transient hiccup could disable every source
button until the next fully successful poll, up to 30s later, even though a
valid inventory had just arrived.
A failure carries no inventory, so there is nothing to order and no reason
to spend the generation on it. ApplySourcesRead now splits the two:
- a success is still fenced by generation, so two successful reads keep
their ordering and an older one cannot overwrite a newer one, and it
always clears the marker;
- a failure is counted instead, and only staleSourcesFailureThreshold in
a row marks the inventory unusable, matching how
offlineFailureThreshold already debounces connectivity in this file.
A genuinely unreachable speaker is therefore stale one poll cycle later
than before, and a single dropped read costs nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
When a `devices` snapshot lost the revision comparison, the merge kept the
newer status we already held but took the rest of the incoming entry. That
produced a self-inconsistent device: the server derives stereoPair from the
very status.Group the snapshot lost on, so a snapshot captured before a
pair was dissolved restored its projection alongside a status that had
already cleared the group. StereoPair.js then rendered a pair that no
longer exists, with an expectedGroupId pointing at a deleted group.
Keep the entry we hold instead. The cost is that info riding the same
snapshot waits for the next one, bounded at 5s by the periodic devices
frame and in practice much shorter, because whatever produced the newer
status also queued a device-list broadcast.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DeviceStatus.Revision is per-connection and restarts at 0. The browser
compares revisions to decide which frame wins, but nothing in the frame
said which revision sequence it belonged to.
So a device id backed by a fresh DeviceConnection published revisions
starting at 0 while an open tab still held a high revision for that id, and
the tab rejected every later frame for it: a status frozen until reload.
HandleDeleteDevice broadcasts after removal, which covers the ordinary
remove-then-rediscover path, but a discovery sweep re-adding the host
inside that window yields a snapshot that already contains the device at
revision 0, so no device-less snapshot is ever sent.
Every status now carries an Epoch identifying the connection that produced
it, stamped by both SetStatus and UpdateStatus. The browser compares epochs
first and only falls back to revisions within one epoch, so a newer
connection is accepted regardless of its revision and a frame still in
flight from the replaced connection is rejected regardless of its.
nextStatusEpoch is seeded from the wall clock and forced strictly
increasing, so epochs also keep rising across a service restart, where a
plain counter would restart at 0 and reintroduce the same problem. It is
in milliseconds because the browser compares it as a JSON number and a
nanosecond timestamp exceeds Number.MAX_SAFE_INTEGER.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A nowPlayingUpdated event is authoritative evidence that the speaker
switched, and the effect watching it promotes a pending command to
provisional-confirmed. The last readback then overwrote the command
wholesale with outcome: 'unverified', in both its no-match branch and its
catch, without looking at what the command had already become.
So a selection the speaker confirmed by push at ~1s was reported as
"Source selection unverified" when the 10s readback happened to fail or
returned a now-playing that had since moved on.
The readback window closing now settles such a command as confirmed rather
than retracting it; only a command still pending, which nothing ever
confirmed, becomes unverified. A failed command stays failed.
The push-event promotion also has to record confirmedRevision, which it
previously left unset: without it the later-authoritative-source check
would compare against undefined and the projection would never clear.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rebase derives NowPlayingRevision from the existing FieldNowPlaying
generation rather than a counter of its own. Nothing asserted that it
advances down both paths that write the field, a completed poll and a push
event, which is the property the player's readback loop depends on to tell
a fresh now-playing write from an unrelated field's merge.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Selecting a source posts once and then confirms by bounded readback. When
the POST itself failed, the write error was captured into active.writeError
and never surfaced: a rejected command reported nothing for the full 10s
readback window and then a bare "Source selection unverified".
Surfacing it needs a distinction the API layer did not make. checkedReq
collapsed every failure into one Error, but the two cases differ:
- 4xx: every 4xx on these endpoints is produced before AfterTouch calls
the speaker (unknown device, unparseable body, empty source, unknown
action), so the command provably never went out. Nothing can confirm
it; report the failure at once, with the server's reason.
- 5xx and transport errors: handleSourceControl reports a failed
Client.SelectSource through sendControlResponse, which maps any
speaker-call error to 500. A request that timed out after the speaker
already switched is indistinguishable from one it never received, so
the readbacks must keep running and the reason is carried into
whatever outcome they reach.
checkedReq now tags thrown errors with `definitive`, and only a definitive
refusal cancels the readbacks. Outcome text appends the reason when there
is one, so a firmware rejection also names the error source it saw.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
main already runs the player's static JS unit tests via `make
test-frontend` over pkg/service/soundtouchweb/frontend_test/*.test.mjs,
wired into .github/workflows/browser-tests.yml. That target was added by
PR #672 and this change predates it.
api.test.mjs was placed in static/js/ instead and run by a second `node
--test` invocation bolted onto test-browser, behind a new NODE variable.
That is a parallel test-wiring convention for one file, and it also mixes
a test into the directory that is served to browsers as static assets.
Move the file next to the other frontend tests and revert the Makefile
change; the existing target and CI job pick it up unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `devices` and `status_update` frames are now applied by revision, but
refreshDevices() still replaced the whole map with whatever the REST call
returned. A refresh issued before a socket update but answered after it
would put the older status back.
Route it through mergeDevicesSnapshot as well. Entries the snapshot omits
are still dropped, so device removal keeps working, and info/stereoPair
still come from the snapshot even when its status loses the comparison.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Removing the `ready.length === 0` early return meant a device that has not
been polled yet rendered a "Sources" heading with an empty list and a
"Source list unavailable" notice. That reports a problem where there is
none: having read no inventory is not the same as having one we distrust.
The section is hidden again when there is nothing to offer, and the
availability notice is now reserved for the case it was meant for, an
inventory we hold but refuse to act on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The source inventory was expired by a 30s read-time TTL evaluated inside
DeviceConnection.Status(). The status poll ticker is also exactly 30s
(discovery.go), so the two ran in lockstep and every source button went
disabled for the moment before each refresh.
Deriving staleness per read had a second cost: two reads at the same
Revision could disagree about sourcesStale, which is the only reason the
browser needed mergeDerivedStatus to carry that one bit across an
otherwise-rejected frame. Dropping the TTL removes that whole special
case, and with it a class of "equal revision, different derived state"
reasoning.
Sources are now stale exactly when the last /sources read failed, recorded
at merge time so every change advances Revision. An unreachable device
fails its polls, so the case the TTL was meant to cover is still covered.
The newer-failure-fences-older-success ordering is unchanged: the merge
still runs through CompleteFieldPoll(FieldSources, ...).
Drops SourcesReadAt, sourceCacheTTL, sourceCacheStatusAt, the Status()
projection and mergeDerivedStatus. The TTL-boundary tests go with them;
the fencing and recovery tests stay, and one of them no longer has to pin
its read times to the wall clock to avoid the TTL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Use a single POST command with bounded delayed readback, revision-order
REST and WebSocket state, and explicit pending, confirmed, unverified, or
failed outcomes. Mark stale source inventories unusable until a successful
refresh and preserve legacy API error handling outside this command path.
Rebased onto main. One conflict could not be resolved by picking a side:
this change introduced its own per-field fencing (fieldRevision,
deviceStatusFieldRevisions, MergeNowPlaying/MergeVolume/MergePresets/
MergeSources/MergeBass/MergeIsConnected) over the same six fields that
main's StatusField mechanism (BeginFieldPoll/CompleteFieldPoll/
ApplyFieldEvent, added by the #654/#668 stack) already orders. Landing
both would leave two independent generation counters guarding the same
state, which is the divergent-fencing bug class that stack already had to
fix three times. Resolved in favour of main's mechanism:
- the Merge* methods and their counters are dropped; the WebSocket
handlers and updateDeviceStatus use main's calls;
- NowPlayingRevision is derived from the existing FieldNowPlaying
generation via recordFieldRevision, not a second counter;
- SetStatus derives Revision from the stored status and supersedes all
field generations, so a replacement cannot reset the revision a
browser is ordering on;
- updateSourcesCache records a failed /sources read through
CompleteFieldPoll, keeping the newer-failure-fences-older-success
property the original had via MergeSourcesFailure;
- the three tests that called the dropped API were ported to it.
Everything else in this change is unmodified.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HandleGetZoneCandidates read entry.Device.DeviceInfo (the immutable
discovery-time snapshot) directly instead of Info(), missed when every
other handler in this file was migrated to the live-name accessor. A
speaker renamed after discovery kept showing its stale name in the
"add to my zone" picker indefinitely.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
refreshDevices() now throws on a failed /api/devices fetch (needed by
removeDeviceAndRefresh's success/failure contract), but StereoPair's
run() awaited it inside the same try that already notified success --
so a refresh/device-list-fetch hiccup after a successful create/rename/
dissolve rolled the UI back into an "operation failed" error state even
though the mutation itself succeeded. Isolate the post-success refresh
so its failure only surfaces as its own, separate notice.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
OnTransportState routed connect/disconnect through
applyConnectionStateEvent, which calls ApplyFieldEvent(FieldConnectivity,
...). That unconditionally bumps FieldConnectivity's applied generation,
so a transient WebSocket transport blip could invalidate a
concurrently-completing, genuinely successful HTTP poll's IsConnected
merge -- exactly the kind of cross-mechanism staleness bug the per-field
fencing was built to prevent. ObserveEventStreamTransport already derives
IsConnected (via applyConnectivityLocked) from the same transport signal,
so the extra call was redundant as well as unsafe.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tuneInSearchSection, TuneInSearchNext, and tuneInProfileNavItem each
implemented the same Station/PlayItem/Topic/Program/Profile dispatch,
but disagreed on what to do with anything else: the first two had no
default case and silently dropped the item entirely (hiding real
content with no trace it existed), while the third treated any
unrecognized type as directly playable with no indication that was a
guess.
Consolidated into one tuneInClassifyItem, used by all three, that
takes a middle path instead of picking one side: an unrecognized type
still gets a playback link (nothing is silently hidden), but its
Subtitle is marked ("Unrecognized type, may not play") so a playback
attempt that doesn't pan out reads as an unsupported content type
rather than a mystery broken link. The player UI (TuneInBrowser.js)
already renders Subtitle verbatim, so this is visible to users, not
just present in the API response.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
handlers_bmx_tunein.go's parseTuneInNavigatePath and stations.go's
navigateTuneIn were byte-for-byte identical (confirmed both already
independently implemented the same "profiles" parsing before PR #677;
that fix was applied consistently to both copies rather than
introducing new duplication). Removed the duplicate: HandleTuneInNavigate
now delegates to stations.Navigate(stations.ProviderTuneIn, wildcard),
the same path stations' own callers already use.
Added TestNavigateTuneIn_ProfilesPathDispatch, covering the single-
segment (current), legacy multi-segment, and empty-URI shapes -- this
package (and the handler package's now-removed copy) had zero test
coverage for this logic before.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TuneInSearch, TuneInSearchNext, and TuneInNavigateProfile each
duplicated the identical "Items" (v1.3)-then-"body" (legacy) fallback.
Extracted tuneInRawItems, used by all three. Pure refactor, no
behavior change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The navigate-500 fix replaced a working call to the existing
tuneInSearchSection helper with new hand-rolled container-handling
logic that silently dropped several things the original already did
correctly:
- An empty container (no "Type" field, only "ContainerType") fell
through to a default branch that treats any unrecognized type as
directly playable, turning the container's own non-playable GuideId
into a bogus playback link. Now skipped instead.
- The Pivots.More.Url "load more" pagination cursor was never read, so
a container with more children than fit on one page silently showed
only the first page. Extracted the cursor-link logic already in
tuneInSearchSection into a shared tuneInMoreCursorLink helper, used
by both.
- The legacy lowercase "children" key (tuneInSearchSection's own
fallback for "Children") was dropped entirely.
- The response's self link used "/v1/navigate/profile/" (singular);
no route dispatcher recognizes that, breaking re-navigation via the
link itself.
- base64.URLEncoding (padded) vs. RawURLEncoding (no padding), two
lines apart building the same kind of href -- decodeBase64URI
already tolerates both, which is why this never surfaced as a
decode failure. Standardized on RawURLEncoding, matching every
other encode site in the file.
- The "profiles" path case (duplicated pre-existing in both
handlers_bmx_tunein.go and stations.go) had no fallback for an empty
encoded URI, unlike the sibling "sub" case a few lines above. Both
copies now fall back the same way "sub" does.
Added TestTuneInNavigateProfileHandlesContainerShapes covering all
four tunein.go fixes against a fixture server modeling the real
two-fetch profile/contents shape.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Splitting mutatingGet into two functions during the main rebase
(mutatingGet keeps the existing result-unmarshaling form used by
/removeGroup; the new mutatingGetConfirmStatus validates a <status>
echo instead, for endpoints with no meaningful response body) left
~30 lines of one-shot-transport setup (clone, disable keep-alives,
CheckRedirect) duplicated between them. Extracted newOneShotHTTPClient
so a future fix to that mechanism can't be applied to one copy and
forgotten in the other.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same reasoning as the -service fix: NewWebApp's default generation-
lifecycle wiring (used by the standalone player, and by embedded
-service until SetStereoPairGenerationPersistence overrides it)
proactively pushed group cleanup/rename to an external Marge backend
the player doesn't own. A speaker's own firmware already self-reports
that create/rename/teardown to whatever Marge backend it's configured
with -- that's the entire reason HandleMargeAddGroup/HandleMargeModifyGroup/
HandleMargeDeleteGroup exist, they're only ever called by speakers.
Extracted the wiring into playerStereoPairGenerationPersistence
(mirroring cmd/soundtouch-service's own testable helper): cleanup and
rename are now no-ops, and preflight's read-only dangling-generation
check stays but its failure no longer blocks Create.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A speaker migrated at the DNS level keeps reporting its own MargeURL as
the literal Bose cloud hostname (e.g. https://streaming.bose.com) --
DNS migration only changes how that hostname resolves on the network,
never the device's own advertised URL. embeddedStereoPairGenerationPersistence's
isLocal() only matched against this service's own advertised server
URL, so it misclassified such a speaker as "external" and sent the
stereo-pair Create preflight's generation-conflict check out over the
real internet instead of checking the local datastore. Bose's cloud is
still live enough to answer (just not to authenticate us), so the
check failed with HTTP 401 and hard-blocked Create for an otherwise
perfectly normal DNS-migrated setup.
Added a DNS-hijack-aware check: if this service's own DNS hijack is
active and the reported MargeURL's host is one of the known redirected
Bose hostnames (discovery.InterceptedBoseHosts), treat it as local too.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Constrain device and playback metadata across narrow layouts while retaining complete values through tooltips and a touch-friendly details disclosure, including RAOP tracks.
