From 96a40f7d885279d269c58d915e7168d1eb2b6f52 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Sun, 1 Nov 2026 13:40:25 -0500 Subject: [PATCH] feat(filter): label endpoint names id values for client-held trees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SSR embed (65f257f3) covers trees the server renders; one path still fell through to the flawed fallback — applying a SAVED filter from a cold bookshelf. That page was rendered before any filter existed (nothing embedded), and the fallback reused the suggest facet, whose rows are MEDIA-CONSTRAINED: "feed in [A,B,C] AND title contains Chapter" labeled only the feeds with a matching episode (the exact live repro from the original bug report). - GET /api/filter/labels?field=X&ids=a,b,c resolves names straight from the entity tables via the shared ResolveFieldLabels core (extracted from FilterTreeLabels): no media join, no tree constraint. RLS/visibility ride the request identity — a zero-episode subscribed feed resolves its title (pinned; the facet path structurally could not), an unsubscribed feed id stays unnamed (pinned), non-id fields are 400 (pinned), ids cap at 100. - Client: resolveIdLabels/resolveLabelsAsync use ONLY the endpoint; the quiet suggest-fetch mode is deleted (its only caller was the flawed fallback). The saved-filter apply flow resolves through the endpoint and rebuilds the chips bar when labels land — chips render raw ids only while the round trip is in flight, never stuck. - 188 web tests green; Go endpoint pins + filter/episode slices green; contract_pins documents the endpoint and the fallback removal. --- cmd/server/tests/podcast_test.go | 66 ++++++++++++++++ docs/contract_pins.md | 18 +++++ internal/handlers/media.go | 25 ++++++ internal/router/search.go | 4 + internal/services/filter_labels.go | 118 ++++++++++++++++++----------- web/src/bookshelf.ts | 9 +++ web/src/filter_builder.test.ts | 30 +++++++- web/src/filter_builder.ts | 82 ++++++++++++-------- 8 files changed, 274 insertions(+), 78 deletions(-) diff --git a/cmd/server/tests/podcast_test.go b/cmd/server/tests/podcast_test.go index 69d1db68..451eccc2 100644 --- a/cmd/server/tests/podcast_test.go +++ b/cmd/server/tests/podcast_test.go @@ -2415,3 +2415,69 @@ func TestSmartCollectionBornReversed(t *testing.T) { "static collections get no chain_order seed") }) } + +// ---- label endpoint: names id-kind values for client-held trees ---- + +// TestFilterLabelsEndpoint pins GET /api/filter/labels (2026-11-01): +// the client path that never touches a server render — applying a +// SAVED filter from a cold bookshelf — resolves names through ENTITY +// tables, not the media-constrained suggest facet. A zero-episode feed +// resolves its title (the facet path structurally could not); an +// unsubscribed feed id resolves to nothing (RLS); a non-id field is a +// 400. +func TestFilterLabelsEndpoint(t *testing.T) { + setup := setupTestServer(t) + regUUID := getRegularUserID(t, setup.DB) + regToken := setup.RegularToken + + lib, _ := createPodcastsLibrary(t, setup, "test LabelsEP Podcasts") + marker := fmt.Sprintf("labelsep%d", time.Now().UnixNano()%100000) + + feedA, err := setup.DB.CreatePodcastFeed(adminSeedCtx(t, setup), database.CreatePodcastFeedParams{ + LibraryID: lib.ID, + FeedUrl: "https://feed.invalid/" + marker + "-a.xml", + Title: "LabelsEP Feed A " + marker, + }) + require.NoError(t, err) + feedB, err := setup.DB.CreatePodcastFeed(adminSeedCtx(t, setup), database.CreatePodcastFeedParams{ + LibraryID: lib.ID, + FeedUrl: "https://feed.invalid/" + marker + "-b.xml", + Title: "LabelsEP Feed B " + marker, + }) + require.NoError(t, err) + // feedA is subscribed; feedB is NOT (RLS must hide it even by id). + subscribeRaw(t, setup, regUUID, feedA.ID) + + feedAID := uuid.UUID(feedA.ID.Bytes).String() + feedBID := uuid.UUID(feedB.ID.Bytes).String() + + t.Run("zero_episode_feed_resolves", func(t *testing.T) { + code, body := podcastAuthed(t, setup, regToken, "GET", + "/api/filter/labels?field=feed&ids="+feedAID, "") + require.Equal(t, http.StatusOK, code, "%s", string(body)) + assert.Contains(t, string(body), `"labels"`) + assert.Contains(t, string(body), "LabelsEP Feed A "+marker, + "a subscribed feed with ZERO episodes resolves its title (no media join)") + }) + + t.Run("comma_joined_ids_resolve_together", func(t *testing.T) { + code, body := podcastAuthed(t, setup, regToken, "GET", + "/api/filter/labels?field=feed&ids="+feedAID+","+feedBID, "") + require.Equal(t, http.StatusOK, code) + assert.Contains(t, string(body), "LabelsEP Feed A "+marker) + }) + + t.Run("unsubscribed_feed_stays_unnamed", func(t *testing.T) { + code, body := podcastAuthed(t, setup, regToken, "GET", + "/api/filter/labels?field=feed&ids="+feedBID, "") + require.Equal(t, http.StatusOK, code) + assert.NotContains(t, string(body), "LabelsEP Feed B "+marker, + "an unsubscribed feed id is invisible to resolution (RLS)") + }) + + t.Run("non_id_field_is_400", func(t *testing.T) { + code, body := podcastAuthed(t, setup, regToken, "GET", + "/api/filter/labels?field=title&ids=whatever", "") + assert.Equal(t, http.StatusBadRequest, code, "%s", string(body)) + }) +} diff --git a/docs/contract_pins.md b/docs/contract_pins.md index 21e52910..2c740a09 100644 --- a/docs/contract_pins.md +++ b/docs/contract_pins.md @@ -685,3 +685,21 @@ born-reversed chain/page pins replace the former native expectations pin from 81e54f89 was corrected while flipping: the API envelope's match list is NOT chain-ordered and never was — order pins live on the SSR page and the reader chain. + +## 2026-11-01 — label endpoint replaces the suggest-facet fallback + +`GET /api/filter/labels?field=X&ids=a,b,c` names id-kind filter values +from their source tables (entity rows only — no media join, no tree +constraint; visibility/RLS ride the request identity). Non-id fields +are a 400; ids truncate at the same 100 cap the SSR embed uses. + +The client's label resolution (`resolveIdLabels`/`resolveLabelsAsync`) +now uses ONLY this endpoint. The former fallback — the quiet suggest +fetch through the MEDIA-CONSTRAINED facet — is deleted: it labeled only +the ids whose media matched the tree's sibling rules (the live feed +repro), which mattered exactly where the SSR embed can't reach: +applying a SAVED filter from a cold bookshelf. The apply flow now +resolves through the endpoint and rebuilds the chips bar when labels +land. Pinned (TestFilterLabelsEndpoint): a zero-episode feed resolves +its title, comma-joined ids resolve together, an unsubscribed feed id +stays unnamed (RLS), non-id fields are 400. diff --git a/internal/handlers/media.go b/internal/handlers/media.go index 5ec9bc13..9b484d88 100644 --- a/internal/handlers/media.go +++ b/internal/handlers/media.go @@ -248,6 +248,31 @@ func NewMediaHandler(db *database.Queries, raw database.DBTX, libraryService *se // resolve id labels through it — 2026-11-01). func (h *MediaHandler) SearchService() *services.SearchService { return h.searchService } +// ResolveFilterLabels — GET /api/filter/labels?field=X&ids=a,b,c names +// id-kind filter values straight from their source tables: the client +// path that never touches a server render (applying a SAVED filter +// from a cold bookshelf) would otherwise fall back to the suggest +// facet, whose rows are media-constrained and label only the ids whose +// media matches the tree's sibling rules (2026-11-01 feed repro). +// Visibility/RLS ride the request identity, same as the SSR embed. +func (h *MediaHandler) ResolveFilterLabels(c *echo.Context) error { + user := c.Get("user").(database.Users) + field := c.QueryParam("field") + if !services.IsIDValuedField(field) { + return c.JSON(http.StatusBadRequest, map[string]string{"error": "field is not an id-valued filter field"}) + } + ids := services.ParseUUIDValues(strings.Split(c.QueryParam("ids"), ",")) + if len(ids) == 0 { + return c.JSON(http.StatusBadRequest, map[string]string{"error": "ids required"}) + } + if len(ids) > services.MaxResolveIDs { + ids = ids[:services.MaxResolveIDs] + } + labels := h.SearchService().ResolveFieldLabels( + c.Request().Context(), user.ID, field, ids) + return c.JSON(http.StatusOK, map[string]interface{}{"labels": labels}) +} + func (h *MediaHandler) ExecuteSearch(ctx context.Context, params services.SearchParams) ([]database.SearchMediaItemsUnifiedRow, int, error) { return h.searchService.ExecuteSearch(ctx, params) } diff --git a/internal/router/search.go b/internal/router/search.go index 0a0664fb..bbcc74c0 100644 --- a/internal/router/search.go +++ b/internal/router/search.go @@ -16,6 +16,10 @@ func registerSearchRoutes(cfg *Config) { // JWT middleware for protected routes jwtMiddleware := createJWTMiddleware(cfg) protected := e.Group("/api", jwtMiddleware) + // Filter label resolution (2026-11-01): names id-kind filter values + // from their source tables for trees the server didn't render (the + // cold saved-filter apply). Visibility/RLS ride the request identity. + protected.GET("/filter/labels", cfg.MediaHandler.ResolveFilterLabels) // Search endpoint - returns JSON or HTML based on HX-Request header protected.GET("/media-items/search", func(c *echo.Context) error { // Check if HTMX request (expects HTML) diff --git a/internal/services/filter_labels.go b/internal/services/filter_labels.go index 8b3a8744..9f9174c1 100644 --- a/internal/services/filter_labels.go +++ b/internal/services/filter_labels.go @@ -29,9 +29,25 @@ import ( // a legal pick, the resolver only names it. // ════════════════════════════════════════════════════════════════════ -// maxResolveIDs bounds one field's resolution query (picks accumulate -// per rule; menus cap far below this). -const maxResolveIDs = 100 +// MaxResolveIDs bounds one field's resolution query (picks accumulate +// per rule; menus cap far below this). The label endpoint truncates to +// the same cap. +const MaxResolveIDs = 100 + +// IsIDValuedField — whether the field's values are entity ids (the +// label endpoint's accepted universe: id-kind + a known source). +func IsIDValuedField(field string) bool { + f, ok := FilterFieldByKey(field) + if !ok { + return false + } + return valueKindFor(f.Kind, field) == VKId && f.Suggest != "" +} + +// ParseUUIDValues splits comma-joined picks into uuids, skipping junk. +func ParseUUIDValues(values []string) []uuid.UUID { + return parseUUIDValues(values) +} // FilterTreeLabels resolves every id-kind rule value in the tree to its // display name. Best-effort by design: a resolve failure degrades to @@ -64,7 +80,7 @@ func (s *SearchService) FilterTreeLabels(ctx context.Context, userID pgtype.UUID continue } for _, id := range parseUUIDValues(c.Values) { - if len(byField[c.Field]) >= maxResolveIDs { + if len(byField[c.Field]) >= MaxResolveIDs { return } byField[c.Field] = append(byField[c.Field], id) @@ -74,48 +90,62 @@ func (s *SearchService) FilterTreeLabels(ctx context.Context, userID pgtype.UUID walk(tree.Children) for field, ids := range byField { - f, _ := FilterFieldByKey(field) - idArgs := make([]uuid.UUID, len(ids)) - copy(idArgs, ids) - var sqlStr string - var args []any - switch f.Suggest { - case suggestLibrary: - // The facet's visibility shape, minus the media join: a - // hidden library is invisible even by id (family lens). - sqlStr = `SELECT l.id::text, l.name -FROM libraries l -LEFT JOIN library_visibility lv ON l.id = lv.library_id AND lv.user_id = $2 -WHERE COALESCE(lv.is_visible, true) = true AND l.id = ANY($1)` - args = []any{idArgs, userID} - case suggestFeed: - // RLS-governed (subscription + visibility predicate): an - // unsubscribed feed id resolves to nothing. - sqlStr = `SELECT id::text, title FROM podcast_feeds WHERE id = ANY($1)` - args = []any{idArgs} - case suggestCollection: - // RLS-governed (app_can_read_collection). - sqlStr = `SELECT id::text, name FROM collections WHERE id = ANY($1)` - args = []any{idArgs} - case suggestAddedBy: - sqlStr = `SELECT id::text, username FROM users WHERE id = ANY($1)` - args = []any{idArgs} - default: - continue + for k, v := range s.ResolveFieldLabels(ctx, userID, field, ids) { + labels[k] = v } - rows, err := s.raw.Query(ctx, sqlStr, args...) - if err != nil { - log.Printf("[filter-labels] resolve %q failed: %v", field, err) - continue + } + return labels +} + +// ResolveFieldLabels names one field's picked ids against its source +// table — the shared core of the SSR embed and the label endpoint. +// Unknown fields resolve to nothing (the caller validates). +func (s *SearchService) ResolveFieldLabels(ctx context.Context, userID pgtype.UUID, field string, ids []uuid.UUID) map[string]string { + labels := map[string]string{} + f, ok := FilterFieldByKey(field) + if !ok { + return labels + } + idArgs := make([]uuid.UUID, len(ids)) + copy(idArgs, ids) + var sqlStr string + var args []any + switch f.Suggest { + case suggestLibrary: + // The facet's visibility shape, minus the media join: a + // hidden library is invisible even by id (family lens). + sqlStr = `SELECT l.id::text, l.name +FROM libraries l +LEFT JOIN library_visibility lv ON l.id = lv.library_id AND lv.user_id = $2 +WHERE COALESCE(lv.is_visible, true) = true AND l.id = ANY($1)` + args = []any{idArgs, userID} + case suggestFeed: + // RLS-governed (subscription + visibility predicate): an + // unsubscribed feed id resolves to nothing. + sqlStr = `SELECT id::text, title FROM podcast_feeds WHERE id = ANY($1)` + args = []any{idArgs} + case suggestCollection: + // RLS-governed (app_can_read_collection). + sqlStr = `SELECT id::text, name FROM collections WHERE id = ANY($1)` + args = []any{idArgs} + case suggestAddedBy: + sqlStr = `SELECT id::text, username FROM users WHERE id = ANY($1)` + args = []any{idArgs} + default: + return labels + } + rows, err := s.raw.Query(ctx, sqlStr, args...) + if err != nil { + log.Printf("[filter-labels] resolve %q failed: %v", field, err) + return labels + } + defer rows.Close() + for rows.Next() { + var v, display string + if err := rows.Scan(&v, &display); err != nil { + break } - for rows.Next() { - var v, display string - if err := rows.Scan(&v, &display); err != nil { - break - } - labels[v] = display - } - rows.Close() + labels[v] = display } return labels } diff --git a/web/src/bookshelf.ts b/web/src/bookshelf.ts index 1f5b9e09..34989edf 100644 --- a/web/src/bookshelf.ts +++ b/web/src/bookshelf.ts @@ -287,6 +287,15 @@ Alpine.data("bookshelf", () => ({ // Rehydrate the rule-builder tree from the refilled form builderData()?.hydrateFromForm(); + // Cold-cache labels (2026-11-01): this page was rendered BEFORE + // the filter existed, so the SSR embed has no names for the saved + // filter's ids — resolve through the label endpoint, then rebuild + // the chips (without the refresh they'd show raw ids until the + // next grid event). + void builderData() + ?.resolveLabelsAsync() + .then(() => refreshFilterChips()); + // Trigger HTMX to apply the filter by submitting the form window.htmx.trigger(filterForm, "submit"); diff --git a/web/src/filter_builder.test.ts b/web/src/filter_builder.test.ts index de1b6eb8..d3a69dda 100644 --- a/web/src/filter_builder.test.ts +++ b/web/src/filter_builder.test.ts @@ -466,20 +466,44 @@ describe("id-kind value cells", () => { url = String(u); return { ok: true, - json: async () => ({ results: [{ value: "lib-9", display: "Ebooks", count: 4 }] }), + json: async () => ({ labels: { "lib-9": "Ebooks" } }), }; }), ); c.resolveIdLabels(); await vi.waitFor(() => expect(c.idLabels["lib-9"]).toBe("Ebooks")); - // the stored id rides as the typed lookup - expect(url).toContain("library=lib-9"); + // resolution rides the LABEL ENDPOINT (entity tables, no media + // constraints) — not the suggest facet + expect(url).toContain("/api/filter/labels"); + expect(url).toContain("field=library"); + expect(url).toContain("ids=lib-9"); expect(c.idChipsFor([0]).map((ch) => ch.label)).toEqual(["Ebooks"]); // label resolution never opens a menu expect(c.suggestionsOpen).toBe(""); vi.unstubAllGlobals(); }); + it("unlabeled ids from a PARTIAL response stay raw, labeled ones resolve", async () => { + // the server can only name what exists (invisible/unsubscribed ids + // return nothing) — the rest must keep their raw ids, not break + const c = filterBuilderData(); + c.filterTree = { + mode: "all", + children: [{ field: "library", op: "in", values: ["lib-1,lib-404"] }], + }; + vi.stubGlobal( + "fetch", + vi.fn(async () => ({ + ok: true, + json: async () => ({ labels: { "lib-1": "Comics" } }), + })), + ); + c.resolveIdLabels(); + await vi.waitFor(() => expect(c.idLabels["lib-1"]).toBe("Comics")); + expect(c.idChipsFor([0]).map((ch) => ch.label)).toEqual(["Comics", "lib-404"]); + vi.unstubAllGlobals(); + }); + it("opening the menu after a pick browses the WHOLE list (regression)", async () => { const c = filterBuilderData(); c.filterTree = { diff --git a/web/src/filter_builder.ts b/web/src/filter_builder.ts index bae6a041..920c219b 100644 --- a/web/src/filter_builder.ts +++ b/web/src/filter_builder.ts @@ -466,7 +466,7 @@ export function filterBuilderData() { // the tree's id-kind values to names at render time and embedded // the map here — ingest it BEFORE any tree lands, so the cells // show names from the very first paint and resolveIdLabels (the - // quiet fetch below) early-returns instead of hitting the server. + // label endpoint below) early-returns instead of hitting it. const labelsRaw = el?.dataset.idLabels; if (labelsRaw) { try { @@ -502,7 +502,7 @@ export function filterBuilderData() { this.hydrateFromForm(); } // Anything still unlabeled (a tree hydrated from a form the SSR - // didn't know about) resolves through the quiet fetch fallback. + // didn't know about) resolves through the label endpoint. this.resolveIdLabels(); }, // treeJSON is the APPLY wire for collection mounts: the page's @@ -725,28 +725,58 @@ export function filterBuilderData() { this.openMenu = ""; this._touch(); }, - // resolveIdLabels — a freshly ingested tree (page load, the Apply - // reload) carries id-kind values with no labels in sight; one QUIET - // fetch per id-kind rule fills idLabels so the cells render names, - // not uuids (2026-11-01: the library cell reverted to the id after - // Apply because labels only ever came from suggestion picks). + // resolveIdLabels — fire-and-forget wrapper (init/ingest paths). resolveIdLabels(): void { - const visit = (node: FilterRuleState | FilterGroupState, path: number[]): void => { + void this.resolveLabelsAsync(); + }, + + // resolveLabelsAsync — names every UNLABELED id-kind value in the + // tree through the label endpoint (entity tables, no media + // constraints). The SSR embed covers page renders; this covers + // trees the client holds that the server never saw — applying a + // SAVED filter from a cold bookshelf (2026-11-01: the old fallback + // reused the suggest facet, which labels only the ids whose media + // matches the tree's sibling rules). Resolves when labels have + // landed so callers can refresh id-labeled UI (the chips bar). + async resolveLabelsAsync(): Promise { + const byField: Record> = {}; + const visit = (node: FilterRuleState | FilterGroupState): void => { if (isRule(node)) { - const meta = FIELD_BY_KEY.get(node.field); - if (meta?.valueKind !== "id") return; + if (FIELD_BY_KEY.get(node.field)?.valueKind !== "id") return; const ids = node.values .join(",") .split(",") .filter((v) => v.trim() !== ""); if (ids.length === 0) return; - if (ids.every((v) => this.idLabels[v] !== undefined)) return; - void this.fetchSuggestions(path.join("-"), path, node, true); + const unlabeled = ids.filter((v) => this.idLabels[v] === undefined); + if (unlabeled.length === 0) return; + const set = (byField[node.field] ??= new Set()); + unlabeled.forEach((v) => set.add(v)); return; } - node.children.forEach((child, idx) => visit(child, [...path, idx])); + node.children.forEach(visit); }; - this.filterTree.children.forEach((child, idx) => visit(child, [idx])); + this.filterTree.children.forEach(visit); + const fields = Object.keys(byField); + if (fields.length === 0) return; + await Promise.all( + fields.map(async (field) => { + const ids = [...byField[field]].join(","); + try { + const response = await fetch( + `/api/filter/labels?field=${encodeURIComponent(field)}&ids=${encodeURIComponent(ids)}`, + { headers: authHeaders() }, + ); + if (!response.ok) return; + const data = (await response.json()) as { labels?: Record }; + for (const [id, label] of Object.entries(data.labels ?? {})) { + this.idLabels[id] = label; + } + } catch { + // offline/failed: cells keep the raw ids (the pre-embed floor) + } + }), + ); }, // valuePlaceholderFor — the value cell's hint. Id-kind cells are @@ -930,20 +960,13 @@ export function filterBuilderData() { void this.fetchSuggestions(rowKey, path, rule); }, 250); }, - async fetchSuggestions( - rowKey: string, - path: number[], - rule: FilterRuleState, - quiet = false, - ): Promise { + async fetchSuggestions(rowKey: string, path: number[], rule: FilterRuleState): Promise { const meta = FIELD_BY_KEY.get(rule.field); if (!meta || !meta.suggest) return; // Fixed-option fields never hit the server. if (meta.valueKind === "fixed") return; const typed = ( - this.isIdValueRow(path) - ? (this.idDrafts[path.join("-")] ?? (quiet ? rule.values.join(",") : "")) - : (rule.values[0] ?? "") + this.isIdValueRow(path) ? (this.idDrafts[path.join("-")] ?? "") : (rule.values[0] ?? "") ).trim(); // 1 char is too little to filter on; EMPTY means BROWSE — the // user clicked the box to see the field's top values under the @@ -952,12 +975,9 @@ export function filterBuilderData() { if (typed.length === 1) return; // cookie auth carries the call; a null bearer must not strand it // Open the menu NOW (loading state) — never a blank flash. - // QUIET mode (label resolution) touches no menu state. - if (!quiet) { - this.suggestions[rowKey] = []; - this.suggestionsLoading = rowKey; - this.suggestionsOpen = rowKey; - } + this.suggestions[rowKey] = []; + this.suggestionsLoading = rowKey; + this.suggestionsOpen = rowKey; // The menu is computed under the suggest-constraint tree: AND // siblings keep narrowing, OR siblings drop (a same-field pick // negates in, taking its value off the list), and the edited @@ -986,10 +1006,10 @@ export function filterBuilderData() { for (const s of data.results ?? []) { if (s.display) this.idLabels[s.value] = s.display; } - if (!quiet) this.suggestions[rowKey] = data.results ?? []; + this.suggestions[rowKey] = data.results ?? []; } catch (error) { console.warn("suggestion fetch failed", error); - if (!quiet) this.suggestions[rowKey] = []; + this.suggestions[rowKey] = []; } finally { if (this.suggestionsLoading === rowKey) this.suggestionsLoading = ""; }