diff --git a/.pi/skills/yellowjacket-dev/SKILL.md b/.pi/skills/yellowjacket-dev/SKILL.md index c3b738f..16686c7 100644 --- a/.pi/skills/yellowjacket-dev/SKILL.md +++ b/.pi/skills/yellowjacket-dev/SKILL.md @@ -195,6 +195,12 @@ Two rules about climbing: - **Do not write an e2e spec first.** Drive the flow by hand, then promote it with `/e2e`. Specs written blind assert on selectors that do not exist. +- **Not every view has a nav item.** Since #25 the destinations are + configurable, Autotag is hidden by default and Downloads is absent + until a download client exists — so `getByTestId('nav-')` waits + 30 s for a locator that will never resolve. `navigateTo(page, view)` + (`e2e/support/fixtures.ts`) dispatches the app's own `navigate` event. + Click the nav item when the *nav* is what the spec is about. Before a commit, the gate is `make lint`, `make test`, `make ui-test`, `make bindings-check`, `make css-check` and — from `frontend/` — diff --git a/.planning/NOTES.md b/.planning/NOTES.md index 5230157..c7d3adf 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -3696,3 +3696,39 @@ as a tidy-up, a change nothing on a desktop renders differently. Related: a width-gated decision **is** testable at both tiers, which is why #61's phone mini player is a `matchMedia` stub in the component test and needs nothing special. + +## A default that is an *absent* key survives an existing seed (2026-08-19) + +The skill warns that a seed freezes every default it has already +persisted, so changing one in `backend/config` is invisible against an +existing `YJ_HOME` while CI, which seeds by running the app, tests the +new one. That warning is about defaults stored as *values*. + +#25's Autotag-hidden default is stored as the **absence of a key**: +`GeneralConfig.ViewVisibility` is a map, an id it does not mention takes +`backend/config.Views`' answer, and only what the user changed is ever +written. So a seed built before the feature existed showed the new +default immediately — verified against `.dev/seeds/default.tar`, whose +`config.toml` has no `[General.ViewVisibility]` table at all, and whose +sidebar came up without Autotag on the first launch of the new binary. +After toggling it on and off again the file carries exactly one line, +`autotag = false`. + +The general form is worth keeping: **a default expressed as a zero value +needs a re-seed to observe; a default expressed as an absent key does +not**, and it needs no migration for existing installs either. It is the +same property that makes removing a view later free (an unknown key is +dropped on load), which is what the `#25 → #27` ordering on #73 rests on. + +## A spec cannot assume a destination has a nav item (2026-08-19) + +Since #25, `getByTestId('nav-')` is not a reliable way to reach a +view: Autotag is hidden by default and Downloads is absent without a +download client, so four existing specs failed on a 30 s timeout waiting +for a locator that will never resolve. `navigateTo(page, view)` in +`e2e/support/fixtures.ts` dispatches the app's own `navigate` event +instead, which is what every nav item, card and detail view dispatches — +so it is the mechanism and not a test-only door. + +Use the nav item when the *nav* is the subject, and `navigateTo` when +the view is. diff --git a/CLAUDE.md b/CLAUDE.md index c0e7826..4664818 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1032,6 +1032,71 @@ whole way through — so it was green on the broken build. Same trap as `layout-overflow.spec.ts` and `page-header`: a spec named for the behaviour, measuring the plumbing. +**Which destinations exist is configuration, and hiding one takes away +the nav item and nothing else.** Eleven sidebar entries is more than +most libraries need (#25), so each is toggleable from Settings → +Navigation, Autotag is off until asked for, and Downloads is absent +until there is a client to download with — a destination for a feature +that cannot work is worse than none. `navigate` still resolves a hidden +view, which is not a nicety: detail views navigate into these and the +launch page is one of them. Nothing needed a special case for the +highlight either, because the paragraph above moved that onto +`active-view-store`: the sidebar asks `isActive(id)` per *rendered* +item, so a hidden view lights nothing exactly as a detail view does. + +Five things about it are load-bearing. + +**The stored shape is a map keyed by view id, and an absent key means +that view's own default** (`backend/config.Views`). That is what makes +this need no migration in either direction, and it is the polarity rule +`AllowMeteredCatalogDownload` states: the zero value is the intended +answer. A `HiddenViews []string` cannot express "Autotag off by +default" at all — its zero value is *hide nothing* — and a struct with +a boolean per view turns a view that later stops existing into stored +garbage. Here an unknown key is dropped on load and a view added later +gets its own default rather than being invisible or forcibly visible. +It is also what makes #73's `#25 → #27` order safe rather than +backwards: when Jobs folds into Settings, `jobs = true` in somebody's +config is a key nothing asks about. + +**Two states the user could not get out of are refused, in the config +and not in the checkbox.** Settings is never hideable and the launch +page is not hideable while it is the launch page. `config.toml` is +hand-editable, so a disabled checkbox is the affordance and +`SetViewVisible` is the rule — an app that can be locked out of its own +Settings by a typo in TOML is a support problem nobody can debug +remotely. On *load* the launch page is instead un-hidden rather than +refused: there is nobody to tell, and the honest reading of "my launch +page is Autotag" is that this user wants Autotag, not that their launch +page should be silently reset to something they did not choose. + +**Downloads is gated at the nav and not in the config**, on +`downloadStore.available`, so switching it on in Settings still means +what it says once a client exists and the tab appears without a restart +(#37's rule). `available` is false until the providers have loaded, +which makes the item *appear* on a fresh launch rather than appearing +and then vanishing. + +**The tab bar honours the toggles too, and the reason is local rather +than a general rule about phones.** `PHONE_COLUMN_IDS` is the precedent +for "what a phone shows is a different question", and it would apply — +except that `bottom-nav`'s "More" opens the *same* ``, +which filters, so an unfiltered bar would contradict its own drawer one +tap away. Which four tabs is still plan 016's committed subset; this +only removes from it, and "More" is never filtered because it is how +everything else stays reachable. + +**The list of destinations is `services/view-meta.ts`**, on +`shortcut-meta.ts`'s pattern, because #25 gave it a second reader: +Settings renders a toggle per view and needs the same labels in the +same order. Which views exist and what an unconfigured install shows is +Go's (`backend/config.Views`, which `DefaultPage`'s validation reads +too, so the launchable set is not a second list); how they are *drawn* +is the frontend's, beside the rest of the icon vocabulary. The binding +returns the **resolved** map for every view, so the frontend holds no +copy of the defaults — which would be the copy that shipped in the +binary rather than the one being edited. + **A primary view is cached, not unmounted.** `index.ts` keeps every primary view in the DOM and toggles a `.view-hidden` class, because that is what preserves `scrollTop` across navigation — so diff --git a/backend/config/config.go b/backend/config/config.go index 2f6430e..3e5b7b0 100644 --- a/backend/config/config.go +++ b/backend/config/config.go @@ -544,7 +544,7 @@ func (c *Config) SetDefaultPage(page string) error { c.General.ApplyDefaults() } - c.General.DefaultPage = DefaultPage(page) + c.General.DefaultPage = View(page) if err := c.General.Validate(); err != nil { return fmt.Errorf( @@ -666,6 +666,81 @@ func (c *Config) SetAllowMeteredCatalogDownload(allow bool) error { return nil } +// GetViewVisibility reports which primary views the sidebar should +// show, answered for every known view rather than only the ones the +// config mentions -- so the frontend filters on a value and never has +// to hold a second copy of the defaults. +func (c *Config) GetViewVisibility() map[string]bool { + if c.General == nil { + general := &GeneralConfig{} + general.ApplyDefaults() + + return general.ResolvedViewVisibility() + } + + return c.General.ResolvedViewVisibility() +} + +// SetViewVisible shows or hides one primary view. +// +// Two refusals, both about a state the user cannot get out of from the +// UI they would be left with: Settings is never hideable, and the +// launch page is never hideable while it is the launch page (change it +// first). Hiding a view does not make it unreachable -- `navigate` +// still resolves it, which detail views depend on -- it only takes the +// nav item away. +func (c *Config) SetViewVisible(view string, visible bool) error { + spec, known := LookupView(view) + if !known { + return fmt.Errorf("%w: %q", errUnknownView, view) + } + + if c.General == nil { + c.General = &GeneralConfig{} + c.General.ApplyDefaults() + } + + if !visible { + if !spec.Hideable { + return fmt.Errorf("%w: %q", errViewNotHideable, view) + } + + if spec.ID == c.General.DefaultPage { + return fmt.Errorf("%w: %q", errViewIsLaunchPage, view) + } + } + + if c.General.ViewVisibility == nil { + c.General.ViewVisibility = make(map[string]bool, len(Views)) + } + + c.General.ViewVisibility[view] = visible + + if err := c.General.Validate(); err != nil { + return fmt.Errorf("invalid view visibility: %w", err) + } + + if err := c.Save(); err != nil { + return fmt.Errorf("could not save config: %w", err) + } + + events.Emit( + c.ctx, + events.GeneralConfigChanged, + map[string]any{ + "ViewVisibility": c.General.ResolvedViewVisibility(), + }, + ) + + c.logger.Info( + "view visibility updated", + "view", view, + "visible", visible, + ) + + 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 895b391..750ec65 100644 --- a/backend/config/general.go +++ b/backend/config/general.go @@ -5,27 +5,13 @@ import ( "fmt" ) -// DefaultPage identifies which view the app opens to on launch. -type DefaultPage string - -// Valid DefaultPage values, matching the frontend's top-level route ids. -const ( - DefaultPageHome DefaultPage = "home" - DefaultPageTracks DefaultPage = "tracks" - DefaultPageAlbums DefaultPage = "albums" - DefaultPageArtists DefaultPage = "artists" - DefaultPageGenres DefaultPage = "genres" - DefaultPagePlaylists DefaultPage = "playlists" - DefaultPageExplore DefaultPage = "explore" - DefaultPageDownloads DefaultPage = "downloads" - DefaultPageAutotag DefaultPage = "autotag" - DefaultPageJobs DefaultPage = "jobs" -) - // DefaultDefaultPage is the launch page for a fresh install. -const DefaultDefaultPage = DefaultPageHome +const DefaultDefaultPage = ViewHome -var errUnknownDefaultPage = errors.New("unknown default page") +var ( + errUnknownDefaultPage = errors.New("unknown default page") + errViewCannotLaunch = errors.New("view cannot be the launch page") +) // QueueFallback identifies what plays, if anything, once the queue // runs out with nothing left to auto-advance to. @@ -46,8 +32,22 @@ var errUnknownQueueFallback = errors.New("unknown queue fallback") // GeneralConfig holds general application preferences that don't // belong to a more specific subsystem. type GeneralConfig struct { - DefaultPage DefaultPage `toml:"DefaultPage"` + DefaultPage View `toml:"DefaultPage"` QueueFallback QueueFallback `toml:"QueueFallback"` + // ViewVisibility says which sidebar destinations are shown, keyed by + // view id. + // + // **An absent key means that view's own default** (`Views`), and that + // is the whole reason this is a map rather than a `HiddenViews + // []string` or a struct of booleans. A list's zero value is "hide + // nothing", which cannot express Autotag being off by default without + // a migration; a struct field for a view that later stops existing is + // stored garbage somebody has to deprecate. Here a view added later + // gets its own default rather than being invisible or forcibly + // visible, an unknown key is dropped on load, and no install needs + // migrating in either direction. Same polarity rule as + // AllowMeteredCatalogDownload: the zero value is the intended answer. + ViewVisibility map[string]bool `toml:"ViewVisibility"` // 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, @@ -71,15 +71,17 @@ func (c *GeneralConfig) ApplyDefaults() { func (c *GeneralConfig) Validate() error { c.ApplyDefaults() - switch c.DefaultPage { - case DefaultPageHome, DefaultPageTracks, DefaultPageAlbums, DefaultPageArtists, - DefaultPageGenres, DefaultPagePlaylists, DefaultPageExplore, DefaultPageDownloads, - DefaultPageAutotag, DefaultPageJobs: - // Valid. - default: + spec, known := LookupView(string(c.DefaultPage)) + if !known { return fmt.Errorf("%w: %q", errUnknownDefaultPage, c.DefaultPage) } + if !spec.CanLaunch { + return fmt.Errorf("%w: %q", errViewCannotLaunch, c.DefaultPage) + } + + c.normalizeViewVisibility() + switch c.QueueFallback { case QueueFallbackStop, QueueFallbackFavorites, QueueFallbackDynamicMix: // Valid. @@ -89,3 +91,51 @@ func (c *GeneralConfig) Validate() error { return nil } + +// normalizeViewVisibility drops what the stored map may not say, and +// repairs the one invariant the shell depends on. +// +// Three things are dropped or forced, and all three are reachable only +// from a hand-edited config or from a version that knew different +// views: an unknown id (a view removed since, e.g. when #27 folds Jobs +// into Settings) says nothing to anybody; a view that is not Hideable +// cannot be false; and **the launch page is always visible**, because +// otherwise an install lands on a page with no nav item pointing at it. +// +// That last one is a *repair* here and an *error* at the setter +// (SetViewVisible), deliberately. On load there is nobody to tell and +// the honest reading of "my launch page is Autotag" is that this user +// wants Autotag, so it is un-hidden rather than the launch page being +// silently reset to something they did not choose. At the setter the +// user is right there and can act, so it refuses and says why. +func (c *GeneralConfig) normalizeViewVisibility() { + for id := range c.ViewVisibility { + spec, known := LookupView(id) + if !known || !spec.Hideable { + delete(c.ViewVisibility, id) + } + } + + if visible, ok := c.ViewVisibility[string(c.DefaultPage)]; ok && !visible { + c.ViewVisibility[string(c.DefaultPage)] = true + } +} + +// ResolvedViewVisibility answers for every known view, so no caller has +// to know the defaults -- the frontend included, which is why the +// binding returns this rather than the stored map. +func (c *GeneralConfig) ResolvedViewVisibility() map[string]bool { + resolved := make(map[string]bool, len(Views)) + + for _, v := range Views { + visible := v.VisibleByDefault + + if stored, ok := c.ViewVisibility[string(v.ID)]; ok && v.Hideable { + visible = stored + } + + resolved[string(v.ID)] = visible + } + + return resolved +} diff --git a/backend/config/views.go b/backend/config/views.go new file mode 100644 index 0000000..87eb47b --- /dev/null +++ b/backend/config/views.go @@ -0,0 +1,89 @@ +package config + +import "errors" + +var ( + errUnknownView = errors.New("unknown view") + errViewNotHideable = errors.New("view cannot be hidden") + errViewIsLaunchPage = errors.New("view is the launch page") +) + +// View identifies one of the shell's primary destinations -- the +// things the sidebar lists and `index.ts` knows as `VIEW_TAGS`. +type View string + +// The primary views, in no particular order: the sidebar owns the order +// it draws them in, because that is presentation. +const ( + ViewHome View = "home" + ViewPlaylists View = "playlists" + ViewArtists View = "artists" + ViewGenres View = "genres" + ViewAlbums View = "albums" + ViewTracks View = "tracks" + ViewExplore View = "explore" + ViewDownloads View = "downloads" + ViewAutotag View = "autotag" + ViewJobs View = "jobs" + ViewSettings View = "settings" +) + +// ViewSpec is what the backend knows about a destination. The label and +// the icon are deliberately absent: those are presentation, they live +// beside the rest of the app's icon vocabulary in +// `frontend/src/utils/icon-language.ts`, and a Go copy of them would be +// a second thing to keep in step for nothing. +type ViewSpec struct { + // ID is the view name the frontend navigates by. + ID View + // VisibleByDefault is what an install gets when the config says + // nothing about this view -- which is every install until somebody + // changes it, and every view added after this one shipped. + VisibleByDefault bool + // Hideable is false for Settings alone. It is a property of the + // view rather than a check in the setter because `config.toml` is + // hand-editable, and an app that can be locked out of its own + // Settings by a typo is a support problem nobody can debug + // remotely. + Hideable bool + // CanLaunch reports whether the view may be the launch page. + // Settings is the only one that may not, which is the shape the + // DefaultPage enum already had. + CanLaunch bool +} + +// Views is the one list of primary destinations, in the order Settings +// offers them. +// +// It is the single source for three things that used to be written down +// separately: which views exist, which of them may be the launch page +// (`DefaultPage`'s validation reads it), and what an unconfigured +// install shows. +// +// Autotag is the one view hidden by default: it rewrites tags on disk, +// which is not what most libraries want on day one, and #25 asks for it +// to be turned on deliberately. +var Views = []ViewSpec{ + {ID: ViewHome, VisibleByDefault: true, Hideable: true, CanLaunch: true}, + {ID: ViewPlaylists, VisibleByDefault: true, Hideable: true, CanLaunch: true}, + {ID: ViewArtists, VisibleByDefault: true, Hideable: true, CanLaunch: true}, + {ID: ViewGenres, VisibleByDefault: true, Hideable: true, CanLaunch: true}, + {ID: ViewAlbums, VisibleByDefault: true, Hideable: true, CanLaunch: true}, + {ID: ViewTracks, VisibleByDefault: true, Hideable: true, CanLaunch: true}, + {ID: ViewExplore, VisibleByDefault: true, Hideable: true, CanLaunch: true}, + {ID: ViewDownloads, VisibleByDefault: true, Hideable: true, CanLaunch: true}, + {ID: ViewAutotag, VisibleByDefault: false, Hideable: true, CanLaunch: true}, + {ID: ViewJobs, VisibleByDefault: true, Hideable: true, CanLaunch: true}, + {ID: ViewSettings, VisibleByDefault: true, Hideable: false, CanLaunch: false}, +} + +// LookupView returns the spec for a view id. +func LookupView(id string) (ViewSpec, bool) { + for _, v := range Views { + if string(v.ID) == id { + return v, true + } + } + + return ViewSpec{}, false +} diff --git a/backend/config/views_test.go b/backend/config/views_test.go new file mode 100644 index 0000000..f14b14f --- /dev/null +++ b/backend/config/views_test.go @@ -0,0 +1,255 @@ +package config + +import ( + "errors" + "log/slog" + "path/filepath" + "testing" +) + +// newViewTestConfig builds a Config backed by a temp file, which is all +// SetViewVisible needs: it saves and emits, and the emit is a no-op +// without a running app. +func newViewTestConfig(t *testing.T) *Config { + t.Helper() + + c := &Config{ + logger: slog.Default(), + filePath: filepath.Join(t.TempDir(), "config.toml"), + } + + // Load a file that is not there: that is what marks the config + // loaded, without which Save refuses on the *second* write. + if err := c.Load(); err != nil { + t.Fatalf("Load() error: %v", err) + } + + return c +} + +// A view the config says nothing about takes its own default, which is +// what makes this need no migration in either direction: an existing +// install gets Autotag hidden without a key, and a view added later +// gets its own answer rather than the list's. +func TestViewVisibilityDefaults(t *testing.T) { + t.Parallel() + + general := &GeneralConfig{} + general.ApplyDefaults() + + resolved := general.ResolvedViewVisibility() + + if len(resolved) != len(Views) { + t.Fatalf("resolved %d views, want %d", len(resolved), len(Views)) + } + + if resolved[string(ViewAutotag)] { + t.Error("autotag should be hidden by default") + } + + for _, v := range Views { + if v.ID == ViewAutotag { + continue + } + + if !resolved[string(v.ID)] { + t.Errorf("%s should be visible by default", v.ID) + } + } +} + +// A stored answer wins over the default, in both directions -- turning +// Autotag on is the whole user-facing point. +func TestViewVisibilityStoredWins(t *testing.T) { + t.Parallel() + + general := &GeneralConfig{ + ViewVisibility: map[string]bool{ + string(ViewAutotag): true, + string(ViewJobs): false, + }, + } + general.ApplyDefaults() + + resolved := general.ResolvedViewVisibility() + + if !resolved[string(ViewAutotag)] { + t.Error("autotag was switched on and should be visible") + } + + if resolved[string(ViewJobs)] { + t.Error("jobs was switched off and should be hidden") + } +} + +// A key for a view that no longer exists is discarded rather than +// migrated. This is the property the #25-before-#27 ordering rests on: +// when Jobs folds into Settings, `jobs = true` in somebody's config is +// a key nothing asks about, not a cleanup task. +func TestValidateDropsUnknownAndUnhideableViews(t *testing.T) { + t.Parallel() + + general := &GeneralConfig{ + ViewVisibility: map[string]bool{ + "a-view-that-was-removed": true, + string(ViewSettings): false, + string(ViewAutotag): true, + }, + } + + if err := general.Validate(); err != nil { + t.Fatalf("Validate() error: %v", err) + } + + if _, ok := general.ViewVisibility["a-view-that-was-removed"]; ok { + t.Error("an unknown view id should be dropped on load") + } + + if _, ok := general.ViewVisibility[string(ViewSettings)]; ok { + t.Error("settings is not hideable and should not be stored") + } + + if !general.ResolvedViewVisibility()[string(ViewSettings)] { + t.Error("settings must resolve visible whatever the file said") + } +} + +// On load there is nobody to tell, so a launch page hidden by a +// hand-edited file is un-hidden rather than the launch page being +// reset to something the user did not choose. +func TestValidateRevealsAHiddenLaunchPage(t *testing.T) { + t.Parallel() + + general := &GeneralConfig{ + DefaultPage: ViewAutotag, + ViewVisibility: map[string]bool{ + string(ViewAutotag): false, + }, + } + + if err := general.Validate(); err != nil { + t.Fatalf("Validate() error: %v", err) + } + + if !general.ResolvedViewVisibility()[string(ViewAutotag)] { + t.Error("the launch page must be visible") + } +} + +// Settings may not be the launch page, which is the shape the old +// DefaultPage enum had and is now read off the same table. +func TestValidateRejectsAnUnlaunchablePage(t *testing.T) { + t.Parallel() + + general := &GeneralConfig{DefaultPage: ViewSettings} + + err := general.Validate() + if !errors.Is(err, errViewCannotLaunch) { + t.Fatalf("Validate() error = %v, want errViewCannotLaunch", err) + } +} + +// At the setter the user is present and can act, so the two states +// they could not get out of are refused rather than repaired. +func TestSetViewVisibleRefusals(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + view string + visible bool + want error + }{ + {"settings is never hideable", string(ViewSettings), false, errViewNotHideable}, + {"the launch page is not hideable", string(ViewHome), false, errViewIsLaunchPage}, + {"an unknown view is not a setting", "nonsense", false, errUnknownView}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + c := newViewTestConfig(t) + + err := c.SetViewVisible(tt.view, tt.visible) + if !errors.Is(err, tt.want) { + t.Fatalf("SetViewVisible() error = %v, want %v", err, tt.want) + } + }) + } +} + +// Showing a view is never refused, including Settings and the launch +// page -- there is no state to be stuck in. +func TestSetViewVisibleShowsAnything(t *testing.T) { + t.Parallel() + + c := newViewTestConfig(t) + + for _, v := range Views { + if err := c.SetViewVisible(string(v.ID), true); err != nil { + t.Fatalf("SetViewVisible(%q, true) error: %v", v.ID, err) + } + } + + if !c.GetViewVisibility()[string(ViewAutotag)] { + t.Error("autotag was switched on and should be visible") + } +} + +// The stored map survives a save/load round trip, which is what a +// map-valued TOML key is worth checking for. +func TestViewVisibilityRoundTrips(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "config.toml") + + original := &Config{logger: slog.Default(), filePath: path} + if err := original.Load(); err != nil { + t.Fatalf("Load() error: %v", err) + } + + if err := original.SetViewVisible(string(ViewAutotag), true); err != nil { + t.Fatalf("SetViewVisible() error: %v", err) + } + + if err := original.SetViewVisible(string(ViewJobs), false); err != nil { + t.Fatalf("SetViewVisible() error: %v", err) + } + + loaded := &Config{logger: slog.Default(), filePath: path} + if err := loaded.Load(); err != nil { + t.Fatalf("Load() error: %v", err) + } + + resolved := loaded.GetViewVisibility() + + if !resolved[string(ViewAutotag)] { + t.Error("autotag should have loaded as visible") + } + + if resolved[string(ViewJobs)] { + t.Error("jobs should have loaded as hidden") + } +} + +// Every view the shell can launch into is a view the sidebar can show, +// or an install could land on a page with no nav item and no setting +// pointing at it. +func TestEveryLaunchableViewIsAView(t *testing.T) { + t.Parallel() + + for _, v := range Views { + if !v.CanLaunch { + continue + } + + if !v.Hideable { + continue + } + + if _, ok := LookupView(string(v.ID)); !ok { + t.Errorf("%s is launchable but not a known view", v.ID) + } + } +} diff --git a/e2e/specs/page-header.spec.ts b/e2e/specs/page-header.spec.ts index faf16ed..71ce42a 100644 --- a/e2e/specs/page-header.spec.ts +++ b/e2e/specs/page-header.spec.ts @@ -1,4 +1,4 @@ -import { test, expect } from '../support/fixtures.js'; +import { test, expect, navigateTo } from '../support/fixtures.js'; /** * H-19: Playlists, Downloads, Jobs, Settings and Home had a page @@ -58,8 +58,12 @@ const TAGS: Record = { test.describe('every primary view says what it is', () => { test('each one has the shared header, with a heading', async ({ app }) => { + // By event rather than by nav item: a destination is not + // guaranteed to have one any more (#25 — Downloads is absent + // without a download client), and every one of these is still a + // primary view with a header, which is what this spec is about. for (const [view, heading, hasCount] of VIEWS) { - await app.getByTestId(`nav-${view}`).click(); + await navigateTo(app, view); await expect(app.getByTestId('main-content')).toHaveAttribute( 'data-active-view', view, diff --git a/e2e/specs/settings-reach.spec.ts b/e2e/specs/settings-reach.spec.ts index 9e9273b..1ddddea 100644 --- a/e2e/specs/settings-reach.spec.ts +++ b/e2e/specs/settings-reach.spec.ts @@ -1,4 +1,4 @@ -import { test, expect } from '../support/fixtures.js'; +import { test, expect, navigateTo } from '../support/fixtures.js'; /** * Plan 007 phase 5: a11y.1 and a11y.2, frozen against the real app. @@ -56,7 +56,9 @@ test.describe('Settings is reachable without a mouse', () => { test.describe("Downloads' tabs are tabs", () => { test('arrow keys move the selection and swap the panel', async ({ app }) => { - await app.getByTestId('nav-downloads').click(); + // By event, not by nav item: with no download client configured + // there is no Downloads destination to click (#25). + await navigateTo(app, 'downloads'); const view = app.locator('downloads-view'); const requests = view.getByRole('tab', { name: 'Requests' }); diff --git a/e2e/specs/view-lifecycle.spec.ts b/e2e/specs/view-lifecycle.spec.ts index 7bd9d08..d42e764 100644 --- a/e2e/specs/view-lifecycle.spec.ts +++ b/e2e/specs/view-lifecycle.spec.ts @@ -4,6 +4,7 @@ import { eventNames, resetEvents, waitForEvent, + navigateTo, } from '../support/fixtures.js'; /** @@ -39,7 +40,9 @@ test.describe('view lifecycle', () => { test('a keypress on Settings does not reach the Autotag queue', async ({ app, }) => { - await app.getByTestId('nav-autotag').click(); + // By event, not by nav item: Autotag is hidden by default (#25) + // and a hidden view is still reachable. + await navigateTo(app, 'autotag'); await expect(app.getByTestId('main-content')).toHaveAttribute( 'data-active-view', 'autotag', @@ -83,7 +86,9 @@ test.describe('view lifecycle', () => { // The other half of the same bug (H-2): two document keydown handlers // with no arbitration meant `s` on this page skipped the album *and* // toggled shuffle. As a panel binding it can only mean one thing. - await app.getByTestId('nav-autotag').click(); + // By event, not by nav item: Autotag is hidden by default (#25) + // and a hidden view is still reachable. + await navigateTo(app, 'autotag'); await expect .poll(() => pendingCount(app)) .toMatch(/^Pending \(\d+\)$/); diff --git a/e2e/specs/view-visibility.spec.ts b/e2e/specs/view-visibility.spec.ts new file mode 100644 index 0000000..9a5b7e4 --- /dev/null +++ b/e2e/specs/view-visibility.spec.ts @@ -0,0 +1,116 @@ +import { test, expect, navigateTo } from '../support/fixtures.js'; + +/** + * Which destinations the navigation offers (#25). + * + * Eleven sidebar entries is more than most libraries need, so they are + * individually toggleable from Settings, Autotag is off until asked for + * and Downloads is absent until there is a client to download with. + * + * **The assertions are about the navigation, not about the setting.** + * "The config was saved" is the plumbing, and the two most recent bugs + * in this area — #69 and #72 — both shipped green under specs that + * measured exactly that. What a person sees is whether the item is in + * the accessibility tree, and whether the view is still reachable when + * it is not. + * + * This runs against the seeded app, whose config is defaults and whose + * download client list is empty, so the initial state below is what a + * fresh install looks like. + */ +type Page = import('@playwright/test').Page; + +const navItem = (page: Page, label: string) => + page.getByRole('button', { name: label, exact: true }); + +/** The Navigation section's checkbox for a destination. */ +const viewToggle = (page: Page, label: string) => + page.getByRole('checkbox', { name: `Show ${label} in the navigation` }); + +async function openNavigationSettings(page: Page): Promise { + await page.getByTestId('nav-settings').click(); + + const section = page.locator( + 'config-page config-section[heading="Navigation"] .header', + ); + + await expect(section).toBeVisible(); + + if ((await section.getAttribute('aria-expanded')) === 'false') { + await section.click(); + } + + await expect(section).toHaveAttribute('aria-expanded', 'true'); +} + +test.describe('configurable destinations', () => { + test('Autotag is off by default and Downloads needs a client', async ({ + app, + }) => { + await expect(app.getByTestId('nav-home')).toBeVisible(); + + await expect(app.getByTestId('nav-autotag')).toHaveCount(0); + await expect(app.getByTestId('nav-downloads')).toHaveCount(0); + }); + + /** + * Hiding takes the item away and nothing else. Detail views navigate + * into these and the launch page is one of them, so a destination + * with no nav item still has to open. + */ + test('a hidden destination is still reachable', async ({ app }) => { + await navigateTo(app, 'autotag'); + + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'autotag', + ); + + // And nothing is falsely lit while standing on it -- the same rule + // a detail view follows, with no special case for either. + await expect(navItem(app, 'Home')).toHaveAttribute('aria-current', 'false'); + }); + + test('switching Autotag on adds it to the sidebar', async ({ app }) => { + await openNavigationSettings(app); + + await viewToggle(app, 'Autotag').check(); + + await expect(app.getByTestId('nav-autotag')).toBeVisible(); + + // Clicking it is the point of having it. + await app.getByTestId('nav-autotag').click(); + await expect(app.getByTestId('main-content')).toHaveAttribute( + 'data-active-view', + 'autotag', + ); + + // Put it back, or the next spec against this app sees a library + // this one changed. + await openNavigationSettings(app); + await viewToggle(app, 'Autotag').uncheck(); + await expect(app.getByTestId('nav-autotag')).toHaveCount(0); + }); + + /** + * Settings has no toggle at all, rather than a toggle that refuses: + * a user who hides it cannot get back to unhide it. The backend + * refuses it too, because `config.toml` is hand-editable. + */ + test('Settings cannot be switched off', async ({ app }) => { + await openNavigationSettings(app); + + await expect(viewToggle(app, 'Settings')).toBeDisabled(); + await expect(app.getByTestId('nav-settings')).toBeVisible(); + }); + + /** + * The launch page is refused while it is the launch page, which is a + * state the user can leave by changing the launch page above it. + */ + test('the launch page cannot be switched off', async ({ app }) => { + await openNavigationSettings(app); + + await expect(viewToggle(app, 'Home')).toBeDisabled(); + }); +}); diff --git a/e2e/support/fixtures.ts b/e2e/support/fixtures.ts index db33658..72a0327 100644 --- a/e2e/support/fixtures.ts +++ b/e2e/support/fixtures.ts @@ -111,6 +111,35 @@ export async function bindingCalls(page: Page): Promise { return calls.map(nameOf); } +/** + * Go to a view without going through the navigation. + * + * `navigate` is the event the shell listens for and every nav item, card + * and detail view dispatches, so this is the app's own mechanism rather + * than a test-only door. It exists because a destination is not + * guaranteed to have a nav item any more (#25): Autotag is hidden until + * the user asks for it and Downloads until a client exists, and a spec + * about what a *view* does should not also be asserting that the + * sidebar offers it. + */ +export async function navigateTo(page: Page, view: string): Promise { + await page.evaluate( + (v) => + void document.dispatchEvent( + new CustomEvent('navigate', { + detail: { view: v }, + bubbles: true, + composed: true, + }), + ), + view, + ); + + await page + .getByTestId('main-content') + .waitFor({ state: 'attached' }); +} + /** Thin client for the dev-only /__test/ surface (backend/testctl). */ export class TestCtl { constructor(private readonly baseURL: string) {} diff --git a/frontend/bindings/yellowjacket/backend/config/config.ts b/frontend/bindings/yellowjacket/backend/config/config.ts index ddd06ac..169ae09 100644 --- a/frontend/bindings/yellowjacket/backend/config/config.ts +++ b/frontend/bindings/yellowjacket/backend/config/config.ts @@ -112,6 +112,16 @@ export function GetTrackListColumns(): $CancellablePromise { + return $Call.ByID(2798108026); +} + /** * Load reads and parses the config file from disk. */ @@ -247,6 +257,20 @@ export function SetTrackListColumns(columns: tracklist$0.Column[] | null): $Canc return $Call.ByID(4226159685, columns); } +/** + * SetViewVisible shows or hides one primary view. + * + * Two refusals, both about a state the user cannot get out of from the + * UI they would be left with: Settings is never hideable, and the + * launch page is never hideable while it is the launch page (change it + * first). Hiding a view does not make it unreachable -- `navigate` + * still resolves it, which detail views depend on -- it only takes the + * nav item away. + */ +export function SetViewVisible(view: string, visible: boolean): $CancellablePromise { + return $Call.ByID(1751982648, view, visible); +} + /** * Validate returns errors if there is a breaking issue with the config. */ diff --git a/frontend/src/components/bottom-nav/bottom-nav.ts b/frontend/src/components/bottom-nav/bottom-nav.ts index 24f6d7c..c029664 100644 --- a/frontend/src/components/bottom-nav/bottom-nav.ts +++ b/frontend/src/components/bottom-nav/bottom-nav.ts @@ -8,6 +8,7 @@ import '../sidebar/app-sidebar.js'; import { nameDialog } from '@utils/name-dialog'; import { ICON_PLAYLIST } from '@utils/icon-language'; import { ActiveViewController } from '@store/controllers/active-view-controller'; +import { ViewVisibilityController } from '@store/controllers/view-visibility-controller'; type View = 'home' | 'albums' | 'tracks' | 'playlists'; @@ -129,6 +130,22 @@ export class BottomNav extends LitElement { */ private activeCtrl = new ActiveViewController(this); + /** + * The tab bar honours the sidebar's toggles (#25), and the reason is + * inside this component rather than a general rule about phones. + * `PHONE_COLUMN_IDS` is the precedent for "what a phone shows is a + * different question", and it would apply here too -- except that + * "More" opens the *same* ``, which filters. An + * unfiltered bar would therefore contradict its own drawer, one tap + * apart, and a destination the user switched off is off wherever it + * is offered. + * + * Which four tabs remains plan 016's committed subset; this only + * removes from it. Hiding all four leaves "More", which is always + * present and reaches everything. + */ + private visibilityCtrl = new ViewVisibilityController(this); + /** * Whether the drawer has been asked for. * @@ -211,7 +228,9 @@ export class BottomNav extends LitElement { return html`