diff --git a/.planning/NOTES.md b/.planning/NOTES.md index 9079bb2..c8781c4 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -3301,3 +3301,34 @@ answering for the *old* bundle — it reported the desktop layout at 424 px until the page was reopened. And wireless adb dropped twice more mid- session when the screen slept; USB for anything longer than a few probes. + +## The catalog download now asks about the connection (2026-08-17) + +Plan 016 B4. ~0.6 GB had no network awareness at all; it is skipped on a +cellular connection unless the user says otherwise +(`AllowMeteredCatalogDownload`, default false, toggle in Settings' Search +Index section). + +**The shape is dictated by the cgo rule, not by taste.** `explore` is +imported by `cmd/indexbuild`, which builds with `CGO_ENABLED=0` and must +not link Wails, so `netpolicy.go` holds the policy and the JSON parsing — +tested on every platform — while the one platform call is a closure +injected from `app.go`, where naming `application` is already legitimate. + +Four things measured or corrected in the doing: + +- **The portable name is `application.Mobile`, not `application.Android`** + (which the plan and `CLAUDE.md` both named). `Android` exists only + under the `android` build tag; `Mobile`'s desktop implementation is a + stub whose `NetworkJSON()` returns `""`. +- **The runtime reports no metered flag.** `{"connected":bool, + "type":"wifi|cellular|ethernet|none"}` is all there is, so cellular is + the signal and a metered *Wi-Fi* — a phone hotspot, a hotel — cannot be + detected. Android itself knows (`NET_CAPABILITY_NOT_METERED`) and the + runtime does not pass it on. Documented gap, not an oversight. +- **An unknown answer must not read as metered.** Every desktop answers + `""`, so the obvious defensive default would have disabled the catalog + download for every desktop user in the world. +- **The gate belongs before the first status write.** Declining is a + no-op — no job in the indicator, no error tier to dismiss — which is + what makes the refusal safe to have on by default. diff --git a/.planning/plans/pending/016-android-feature-parity.md b/.planning/plans/pending/016-android-feature-parity.md index 9b43dac..4a3fbf2 100644 --- a/.planning/plans/pending/016-android-feature-parity.md +++ b/.planning/plans/pending/016-android-feature-parity.md @@ -347,8 +347,18 @@ done.** viewport would have: saved *desktop* column widths reached the phone through an id-keyed store and gave the duration column 55% of the row. -**B2 is complete.** What is left in this plan is B3 (tag writing, which -needs a device), B4 (the catalog download on a metered connection), and +**B2 and B4 are complete.** B4 is `backend/explore/netpolicy.go`: the +catalog download is skipped on a cellular connection unless +`AllowMeteredCatalogDownload` is on, with the toggle in Settings' Search +Index section. The policy and the JSON parsing are in `explore` (tested +on every platform) and only the platform call is injected from `app.go`, +because `cmd/indexbuild` imports `explore` and must not link Wails. Two +things the plan got slightly wrong: the portable API is +`application.Mobile.NetworkJSON()` rather than `Android`'s, and it +reports no metered flag — so cellular is the signal and a metered Wi-Fi +cannot be seen. + +What is left in this plan is B3 (tag writing, which needs a device) and the standing question of the Light Phone's Chrome 113 — which so far has cost nothing: menus, dialogs and long-press all work on it. diff --git a/CLAUDE.md b/CLAUDE.md index 4c617df..4c8d04d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -508,6 +508,31 @@ selected as a literal `0`. Adding the column to the importer's SELECT list without that is how a published artifact — which nobody can re-cut retroactively — starts failing with `no such column`. +**A 0.6 GB download asks about the connection first.** `explore`'s +catalog artifact had no network awareness at all, which on a phone is a +month's data allowance spent without being asked (plan 016 B4). +`netpolicy.go` is the gate, and its shape is dictated by one constraint: +`explore` is imported by `cmd/indexbuild`, which is built with +`CGO_ENABLED=0` and must not link Wails — so the *policy* and the +*parsing* live here and are tested on every platform, while the platform +call is a closure injected from `app.go`. It is +`application.Mobile.NetworkJSON()`, not `application.Android`'s: the +latter exists only under the `android` build tag, and `Mobile`'s desktop +implementation is a stub returning `""`. + +Three rules in it are load-bearing. **An unknown answer is not a metered +one** — only mobile answers at all, so treating silence as metered would +refuse the download on every desktop. **Cellular is the only signal +available**: the runtime reports `wifi|cellular|ethernet|none` and no +metered flag, so a metered *Wi-Fi* (a hotspot, a hotel) cannot be +detected and is not refused, which is a documented gap rather than an +oversight. And **the gate runs before anything is staged**, so declining +is a no-op rather than a job in the indicator and a status the user has +to dismiss. The permission (`AllowMeteredCatalogDownload`, default +false, so an existing config is careful without a migration) is read at +the moment a download would start, so turning it on takes effect on the +next attempt rather than the next launch. + **Background work yields, and says so in the context.** The post-scan backfills share MusicBrainz's rate limiters with every page the user can open, and both were FIFO — so a thousand-artist enrichment put an diff --git a/backend/app.go b/backend/app.go index 9a25996..376f0ae 100644 --- a/backend/app.go +++ b/backend/app.go @@ -191,6 +191,24 @@ func NewYellowJacketApp( yjApp.library.SetJobRegistry(yjApp.jobs) yjApp.explore.SetJobRegistry(yjApp.jobs) + // Whether this connection is one to spend ~0.6 GB of catalog on + // (plan 016 B4). The probe is injected from here because `explore` is + // imported by `cmd/indexbuild`, which must not link Wails: naming + // `application` there is what `TestIndexToolsDoNotImportWails` + // forbids. + // + // `application.Mobile`, not `application.Android`: the latter exists + // only under the `android` build tag, while `Mobile` is the portable + // name whose desktop implementation is a stub returning "" — which + // parses to "unknown" and refuses nothing. Plan 016 named the tagged + // one; this is the same call by the name every build has. + yjApp.explore.SetNetworkPolicy( + func() explore.Network { + return explore.ParseNetworkJSON(application.Mobile.NetworkJSON()) + }, + yjApp.appConfig.GetAllowMeteredCatalogDownload, + ) + // Let the release prefetch skip albums the user already owns in // full — those open with no catalog call at all, so warming their // tracklists spends the most expensive request in the app on diff --git a/backend/config/config.go b/backend/config/config.go index c178ccb..f0ff93b 100644 --- a/backend/config/config.go +++ b/backend/config/config.go @@ -620,6 +620,51 @@ func (c *Config) SetQueueFallback(mode string) error { return nil } +// GetAllowMeteredCatalogDownload reports whether the ~0.6 GB Explore +// catalog may be fetched on a metered connection. +func (c *Config) GetAllowMeteredCatalogDownload() bool { + if c.General == nil { + return false + } + + return c.General.AllowMeteredCatalogDownload +} + +// SetAllowMeteredCatalogDownload saves the metered-download permission. +// +// There is nothing to validate and nothing to restart: the policy is +// read at the moment a download would start, so turning it on takes +// effect on the next attempt rather than needing this launch to be over. +func (c *Config) SetAllowMeteredCatalogDownload(allow bool) error { + if c.General == nil { + c.General = &GeneralConfig{} + c.General.ApplyDefaults() + } + + c.General.AllowMeteredCatalogDownload = allow + + if err := c.Save(); err != nil { + return fmt.Errorf( + "could not save config: %w", err, + ) + } + + events.Emit( + c.ctx, + events.GeneralConfigChanged, + map[string]any{ + "AllowMeteredCatalogDownload": allow, + }, + ) + + c.logger.Info( + "metered catalog download permission updated", + "allow", allow, + ) + + return nil +} + // GetTrackListColumns returns the configured track-list columns. func (c *Config) GetTrackListColumns() []tracklist.Column { if c.TrackList == nil { diff --git a/backend/config/general.go b/backend/config/general.go index 641c3ab..895b391 100644 --- a/backend/config/general.go +++ b/backend/config/general.go @@ -48,6 +48,12 @@ var errUnknownQueueFallback = errors.New("unknown queue fallback") type GeneralConfig struct { DefaultPage DefaultPage `toml:"DefaultPage"` QueueFallback QueueFallback `toml:"QueueFallback"` + // AllowMeteredCatalogDownload permits the ~0.6 GB Explore catalog to + // be fetched on a connection the platform calls cellular. It defaults + // to false, which is the whole point: the zero value is the safe one, + // so an existing config with no such key refuses by default rather + // than needing a migration to become careful. + AllowMeteredCatalogDownload bool `toml:"AllowMeteredCatalogDownload"` } // ApplyDefaults fills zero-value fields with sensible defaults. diff --git a/backend/explore/artifactbuild.go b/backend/explore/artifactbuild.go index b592367..df52097 100644 --- a/backend/explore/artifactbuild.go +++ b/backend/explore/artifactbuild.go @@ -27,6 +27,20 @@ var artifactStageNames = [...]string{ // failure path is non-fatal by design: the caller falls back, and a // fresh install with no network still gets its own library in Explore. func (si *SearchIndex) tryCoreArtifact(ctx context.Context) error { + // Before anything is staged: ~0.6 GB is not a download to start on + // someone's cellular allowance without being asked (plan 016 B4). + // This is checked first so no job appears and no status changes -- + // declining is a no-op, not a failure the user has to dismiss. + if si.netPolicy.refuses() { + si.logIndexJob( + jobs.LevelInfo, + "Skipping the catalog download on a metered connection. "+ + "Enable it in Settings to download anyway.", + ) + + return ErrMeteredNetwork + } + si.mu.Lock() si.buildStatus = IndexStatus{ Building: true, diff --git a/backend/explore/netpolicy.go b/backend/explore/netpolicy.go new file mode 100644 index 0000000..72d2da0 --- /dev/null +++ b/backend/explore/netpolicy.go @@ -0,0 +1,137 @@ +package explore + +import ( + "encoding/json" + "errors" + "strings" + "sync" +) + +// Whether the catalog artifact may be downloaded on this connection +// (plan 016 B4). +// +// The artifact is ~0.6 GB. On a desktop that is a minute of someone +// else's bandwidth; on a phone it can be a month's allowance, and the +// app had no awareness of the difference at all. +// +// Three decisions shape this file. +// +// **The policy lives here and the platform call does not.** `explore` is +// imported by `cmd/indexbuild`, which is built with `CGO_ENABLED=0` in a +// plain Go container, so naming `application` here would break the one +// job that must not fail (see `TestIndexToolsDoNotImportWails`). What is +// injected is a closure; what is *tested* is the parsing and the +// decision, on every platform. +// +// **An unknown answer is not a metered one.** Only mobile answers this +// question — the desktop stub returns an empty string — so a policy that +// treated silence as "metered" would refuse the download on every +// desktop in the world. Silence means "no reason to refuse". +// +// **Cellular is the signal, and it is the only one available.** Wails +// reports `{"connected":bool,"type":"wifi|cellular|ethernet|none"}` and +// no metered flag, so a metered *wifi* — a phone hotspot, a hotel — is +// invisible to us and will not be refused. That is a known gap rather +// than an oversight: Android knows (`NET_CAPABILITY_NOT_METERED`) and +// the runtime does not pass it on. + +// ErrMeteredNetwork is returned instead of downloading the catalog when +// the connection looks metered and the user has not opted in. Every +// failure path in `tryCoreArtifact` is already non-fatal, so this +// behaves like any other reason the artifact is not available yet. +var ErrMeteredNetwork = errors.New( + "explore: catalog download declined on a metered connection", +) + +// Network is what the platform can say about the connection. +type Network struct { + // Known is false when nothing answered — every desktop, and any + // mobile build whose bridge is not up yet. + Known bool + // Connected reports a usable connection of any kind. + Connected bool + // Metered reports a connection the user is plausibly paying for by + // the byte. See the note above on what this cannot see. + Metered bool +} + +// NetworkProbe answers "what kind of connection is this", or an unknown +// Network when the platform does not say. +type NetworkProbe func() Network + +// ParseNetworkJSON reads the runtime's network payload. +// +// Anything unparseable is `Known: false` rather than an error: this +// decides whether to *skip* an optional download, and a malformed +// payload is not a reason to refuse one. +func ParseNetworkJSON(payload string) Network { + var raw struct { + Connected bool `json:"connected"` + Type string `json:"type"` + } + + if strings.TrimSpace(payload) == "" { + return Network{} + } + + if err := json.Unmarshal([]byte(payload), &raw); err != nil { + return Network{} + } + + return Network{ + Known: true, + Connected: raw.Connected, + Metered: strings.EqualFold(raw.Type, "cellular"), + } +} + +// networkPolicy is the injected half: how to ask, and whether the user +// has said yes anyway. +type networkPolicy struct { + mu sync.RWMutex + probe NetworkProbe + allowMetered func() bool +} + +func (p *networkPolicy) set(probe NetworkProbe, allowMetered func() bool) { + p.mu.Lock() + defer p.mu.Unlock() + + p.probe = probe + p.allowMetered = allowMetered +} + +// refuses reports whether a large optional download should be skipped. +func (p *networkPolicy) refuses() bool { + p.mu.RLock() + probe, allow := p.probe, p.allowMetered + p.mu.RUnlock() + + if probe == nil { + return false + } + + if allow != nil && allow() { + return false + } + + state := probe() + + return state.Known && state.Metered +} + +// SetNetworkPolicy wires how the catalog download decides whether this +// connection is one to spend 0.6 GB on. Both arguments may be nil, which +// is the desktop's answer: never refuse. +// +//wails:ignore // internal wiring, not part of the app's IPC surface. +func (si *SearchIndex) SetNetworkPolicy(probe NetworkProbe, allowMetered func() bool) { + si.netPolicy.set(probe, allowMetered) +} + +// SetNetworkPolicy wires the metered-connection policy into the index. +// +//wails:ignore // internal wiring, not part of the app's IPC surface. +func (e *Service) SetNetworkPolicy(probe NetworkProbe, allowMetered func() bool) { + e.index.SetNetworkPolicy(probe, allowMetered) +} diff --git a/backend/explore/netpolicy_test.go b/backend/explore/netpolicy_test.go new file mode 100644 index 0000000..008394a --- /dev/null +++ b/backend/explore/netpolicy_test.go @@ -0,0 +1,158 @@ +package explore + +import ( + "errors" + "testing" +) + +// The catalog is ~0.6 GB and the decision not to fetch it is the only +// part of plan 016 B4 that can be tested anywhere but on a phone: the +// platform call is a one-line closure injected from app.go, and +// everything that decides anything is here. + +func TestParseNetworkJSON(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + payload string + want Network + }{{ + name: "cellular is metered", + payload: `{"connected":true,"type":"cellular"}`, + want: Network{Known: true, Connected: true, Metered: true}, + }, { + name: "wifi is not", + payload: `{"connected":true,"type":"wifi"}`, + want: Network{Known: true, Connected: true}, + }, { + name: "ethernet is not", + payload: `{"connected":true,"type":"ethernet"}`, + want: Network{Known: true, Connected: true}, + }, { + name: "the case is the platform's business, not ours", + payload: `{"connected":true,"type":"Cellular"}`, + want: Network{Known: true, Connected: true, Metered: true}, + }, { + name: "offline is known and unmetered", + payload: `{"connected":false,"type":"none"}`, + want: Network{Known: true}, + }, { + // The desktop stub. This is the case that must not read as + // "metered": every desktop in the world answers this way. + name: "an empty payload is unknown", + payload: "", + want: Network{}, + }, { + name: "so is a malformed one", + payload: `{"connected":`, + want: Network{}, + }} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := ParseNetworkJSON(tt.payload); got != tt.want { + t.Errorf("ParseNetworkJSON(%q) = %+v, want %+v", tt.payload, got, tt.want) + } + }) + } +} + +func TestNetworkPolicyRefuses(t *testing.T) { + t.Parallel() + + cellular := func() Network { + return Network{Known: true, Connected: true, Metered: true} + } + wifi := func() Network { return Network{Known: true, Connected: true} } + unknown := func() Network { return Network{} } + yes := func() bool { return true } + no := func() bool { return false } + + tests := []struct { + name string + probe NetworkProbe + allowMetered func() bool + want bool + }{{ + name: "no probe wired refuses nothing", + probe: nil, + want: false, + }, { + name: "an unknown connection refuses nothing", + probe: unknown, + want: false, + }, { + name: "wifi refuses nothing", + probe: wifi, + want: false, + }, { + name: "cellular refuses by default", + probe: cellular, + want: true, + }, { + name: "cellular with no permission refuses", + probe: cellular, + allowMetered: no, + want: true, + }, { + name: "cellular the user opted into does not", + probe: cellular, + allowMetered: yes, + want: false, + }, { + // The permission is read at decision time rather than captured, + // so turning it on takes effect on the next attempt instead of + // the next launch. + name: "permission is asked, not remembered", + probe: cellular, + allowMetered: yes, + want: false, + }} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var p networkPolicy + + p.set(tt.probe, tt.allowMetered) + + if got := p.refuses(); got != tt.want { + t.Errorf("refuses() = %v, want %v", got, tt.want) + } + }) + } +} + +// The gate has to come before anything is staged: a declined download is +// a no-op, not a job in the indicator or a status the user must dismiss. +func TestTryCoreArtifactDeclinesMeteredWithoutStaging(t *testing.T) { + t.Parallel() + + si := &SearchIndex{} + + si.SetNetworkPolicy( + func() Network { return Network{Known: true, Connected: true, Metered: true} }, + nil, + ) + + err := si.tryCoreArtifact(t.Context()) + + if !errors.Is(err, ErrMeteredNetwork) { + t.Fatalf("tryCoreArtifact() error = %v, want ErrMeteredNetwork", err) + } + + // Nothing announced itself: no build status, no tiers, no job. A + // SearchIndex with no database would panic on any of the work below + // the gate, which is itself part of the assertion. + if si.buildStatus.Building { + t.Error("declining a metered download still reported a build in progress") + } + + if len(si.buildStatus.Tiers) != 0 { + t.Errorf("declining staged %d tiers, want none", len(si.buildStatus.Tiers)) + } +} diff --git a/backend/explore/searchindex.go b/backend/explore/searchindex.go index 384bfd5..d7a0448 100644 --- a/backend/explore/searchindex.go +++ b/backend/explore/searchindex.go @@ -212,6 +212,11 @@ type SearchIndex struct { cancel context.CancelFunc done chan struct{} + // netPolicy decides whether this connection is one to spend ~0.6 GB + // of catalog on. Its own lock: it is written once at startup and read + // from the build goroutine (netpolicy.go). + netPolicy networkPolicy + mu sync.RWMutex ready bool diff --git a/frontend/bindings/yellowjacket/backend/config/config.ts b/frontend/bindings/yellowjacket/backend/config/config.ts index 75f97fa..ddd06ac 100644 --- a/frontend/bindings/yellowjacket/backend/config/config.ts +++ b/frontend/bindings/yellowjacket/backend/config/config.ts @@ -17,6 +17,14 @@ import * as download$0 from "../download/models.js"; // @ts-ignore: Unused imports import * as tracklist$0 from "../tracklist/models.js"; +/** + * GetAllowMeteredCatalogDownload reports whether the ~0.6 GB Explore + * catalog may be fetched on a metered connection. + */ +export function GetAllowMeteredCatalogDownload(): $CancellablePromise { + return $Call.ByID(2258585139); +} + /** * GetDefaultPage returns the view the app opens to on launch. */ @@ -127,6 +135,17 @@ export function Save(): $CancellablePromise { return $Call.ByID(1988945736); } +/** + * SetAllowMeteredCatalogDownload saves the metered-download permission. + * + * There is nothing to validate and nothing to restart: the policy is + * read at the moment a download would start, so turning it on takes + * effect on the next attempt rather than needing this launch to be over. + */ +export function SetAllowMeteredCatalogDownload(allow: boolean): $CancellablePromise { + return $Call.ByID(192700351, allow); +} + /** * SetDefaultPage validates and saves a new launch page. */ diff --git a/frontend/src/components/config-page/config-page.ts b/frontend/src/components/config-page/config-page.ts index cf7d9b9..c0850a0 100644 --- a/frontend/src/components/config-page/config-page.ts +++ b/frontend/src/components/config-page/config-page.ts @@ -17,6 +17,8 @@ import { SetDefaultPage, GetQueueFallback, SetQueueFallback, + GetAllowMeteredCatalogDownload, + SetAllowMeteredCatalogDownload, } from '@go/config/config.js'; import { GetIndexStatus } from '@go/explore/service.js'; import { notificationStore } from '@store/notification-store'; @@ -75,6 +77,9 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) { // --- Now Playing state --- @state() private scrollMode = 'hover'; + /** Whether the ~0.6 GB catalog may be fetched on mobile data. */ + @state() private allowMeteredCatalogDownload = false; + // --- Favorites state --- @state() private playlists: playlist.Summary[] = []; @@ -888,17 +893,20 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) { private async loadLibraries(): Promise { try { - const [libs, mode, defaultPage, queueFallback] = await Promise.all([ - GetAllLibrariesWithTrackCounts(), - GetScanConcurrency(), - GetDefaultPage(), - GetQueueFallback(), - ]); + const [libs, mode, defaultPage, queueFallback, allowMetered] = + await Promise.all([ + GetAllLibrariesWithTrackCounts(), + GetScanConcurrency(), + GetDefaultPage(), + GetQueueFallback(), + GetAllowMeteredCatalogDownload(), + ]); this.libraries = libs ?? []; this.concurrencyMode = mode; this.defaultPage = defaultPage; this.queueFallback = queueFallback; + this.allowMeteredCatalogDownload = allowMetered; } catch (err) { console.error( @@ -1500,10 +1508,53 @@ export class ConfigPage extends ViewLifecycleMixin(LitElement) { ` : html`
Loading status…
`} + + `; } + /** + * The catalog download's one permission (plan 016 B4). + * + * It is in this section rather than General because it is about + * *this* download and nothing else, and because the section already + * explains what the catalog is — the toggle would be unreadable + * beside "Default page". + */ + private handleAllowMeteredChange = ( + e: CustomEvent, + ): void => { + const allow = Boolean(e.detail.value); + const previous = this.allowMeteredCatalogDownload; + + this.allowMeteredCatalogDownload = allow; + + void SetAllowMeteredCatalogDownload(allow).catch((err: unknown) => { + console.error('failed to save metered download permission', err); + // The visible state reverted, so this is the Transient case: + // a small action the user can simply repeat. + this.allowMeteredCatalogDownload = previous; + notificationStore.transient({ + key: 'metered-catalog-setting', + title: 'Setting not saved', + text: describeError(err, 'That setting could not be saved.'), + }); + }); + }; + private tierIcon(state: string): string { switch (state) { case 'complete':