From 4ad4a8381248052a37d6c2fb9c17fd3900bba3bf Mon Sep 17 00:00:00 2001 From: ssavutu Date: Thu, 3 Sep 2026 23:27:51 -0400 Subject: [PATCH 1/2] Make the breaking and featured filters switches They shipped as three-state chip rows -- All / Breaking / Not breaking -- on the reasoning that an editor clearing a false alarm wants the stories that are not flagged. In use that third state earns nothing: the reason to open this screen is to find what IS flagged, and two rows of three chips cost more room in the filter bar than the one question they answer. Each is now a switch: on narrows to the flagged articles, off does not filter. Hand-rolled, because there is no switch in components/ui and no @radix-ui/react-switch in the tree, and pulling one in for two toggles would be more code than the twenty lines here. role="switch" with aria-checked is what makes it announce correctly; the track and knob are aria-hidden spans inside the button rather than focusable elements of their own. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01L4qBhBdQto1yNp1zP7VLYc --- frontend/src/pages/articleView.tsx | 104 ++++++++++++++++++----------- 1 file changed, 66 insertions(+), 38 deletions(-) diff --git a/frontend/src/pages/articleView.tsx b/frontend/src/pages/articleView.tsx index cd541ea..71e36ed 100644 --- a/frontend/src/pages/articleView.tsx +++ b/frontend/src/pages/articleView.tsx @@ -99,12 +99,6 @@ type ArticleViewProps = { // means send no status filter at all. type PublishedFilter = "all" | "published" | "draft" | "scheduled" -// A flag filter is three-state: "all" sends no parameter at all, the other two -// send breaking/featured explicitly. "Not breaking" has to be a real choice -// rather than the absence of one -- an editor clearing a false alarm wants the -// stories that are NOT flagged. -type FlagFilter = "all" | "on" | "off" - type ArticleViewUIState = { searchQuery?: string activeTab?: "all" | "trash" @@ -112,14 +106,15 @@ type ArticleViewUIState = { sectionFilterSlug?: string subsectionFilterSlug?: string publishedFilter?: PublishedFilter - breakingFilter?: FlagFilter - featuredFilter?: FlagFilter + // Each narrows to the flagged articles when on and does not filter at all + // when off, so neither ever asks for the unflagged half. The endpoint still + // accepts breaking=false/featured=false; nothing here sends it. + breakingOnly?: boolean + featuredOnly?: boolean dateSortDirection?: "asc" | "desc" pageSize?: number } -const flagFilterParam = (filter: FlagFilter) => (filter === "on" ? "true" : "false") - type ArticleResultsCacheEntry = { items: ArticleItem[] totalArticleCount: number @@ -141,6 +136,48 @@ const writeSessionJSON = (key: string, value: unknown) => { window.sessionStorage.setItem(key, JSON.stringify(value)) } +// A switch rather than a checkbox because these read as "on/off", not as items +// ticked off a list. Hand-rolled: there is no switch in components/ui and no +// @radix-ui/react-switch in the tree, and pulling one in for two toggles would +// be more code than this. +// +// role="switch" with aria-checked is what makes it announce as a switch; the +// track and knob are presentational, so they are spans inside the button rather +// than focusable elements of their own. +function FilterSwitch({ + checked, + label, + onChange, +}: { + checked: boolean + label: string + onChange: (next: boolean) => void +}) { + return ( + + ) +} + function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: ArticleViewProps) { const navigate = useNavigate() const apiFetch = useApiFetch() @@ -166,8 +203,8 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article const [subsections, setSubsections] = useState([]) const [subsectionFilterSlug, setSubsectionFilterSlug] = useState(() => loadUIState().subsectionFilterSlug ?? "") const [publishedFilter, setPublishedFilter] = useState(() => loadUIState().publishedFilter ?? "all") - const [breakingFilter, setBreakingFilter] = useState(() => loadUIState().breakingFilter ?? "all") - const [featuredFilter, setFeaturedFilter] = useState(() => loadUIState().featuredFilter ?? "all") + const [breakingOnly, setBreakingOnly] = useState(() => loadUIState().breakingOnly ?? false) + const [featuredOnly, setFeaturedOnly] = useState(() => loadUIState().featuredOnly ?? false) const [dateSortDirection, setDateSortDirection] = useState<"asc" | "desc">(() => loadUIState().dateSortDirection ?? "desc") const [isLoading, setIsLoading] = useState(true) const [error, setError] = useState(null) @@ -183,17 +220,17 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article sectionFilterSlug, subsectionFilterSlug, publishedFilter, - breakingFilter, - featuredFilter, + breakingOnly, + featuredOnly, dateSortDirection, pageSize, } satisfies ArticleViewUIState) }, [ activeTab, authorQuery, - breakingFilter, + breakingOnly, dateSortDirection, - featuredFilter, + featuredOnly, pageSize, publishedFilter, searchQuery, @@ -408,11 +445,11 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article if (subsectionFilterSlug) { params.set("subsection_slug", subsectionFilterSlug) } - if (breakingFilter !== "all") { - params.set("breaking", flagFilterParam(breakingFilter)) + if (breakingOnly) { + params.set("breaking", "true") } - if (featuredFilter !== "all") { - params.set("featured", flagFilterParam(featuredFilter)) + if (featuredOnly) { + params.set("featured", "true") } if (fixedType) { params.set("type", fixedType) @@ -494,7 +531,7 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article return () => { cancelled = true } - }, [activeTab, apiFetch, authorQuery, breakingFilter, dateSortDirection, excludeType, featuredFilter, fixedType, page, pageSize, publishedFilter, resultsCacheKey, searchQuery, sectionFilterSlug, selectedAuthorSlug, subsectionFilterSlug]) + }, [activeTab, apiFetch, authorQuery, breakingOnly, dateSortDirection, excludeType, featuredOnly, fixedType, page, pageSize, publishedFilter, resultsCacheKey, searchQuery, sectionFilterSlug, selectedAuthorSlug, subsectionFilterSlug]) const onChangeTab = (tab: "all" | "trash") => { setActiveTab(tab) @@ -508,7 +545,7 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article useEffect(() => { setPage(0) - }, [authorQuery, publishedFilter, breakingFilter, featuredFilter, dateSortDirection, searchQuery, sectionFilterSlug, subsectionFilterSlug, pageSize]) + }, [authorQuery, publishedFilter, breakingOnly, featuredOnly, dateSortDirection, searchQuery, sectionFilterSlug, subsectionFilterSlug, pageSize]) const effectiveTotalCount = Math.max(totalArticleCount, (page * pageSize) + articles.length) const totalPages = Math.max(1, Math.ceil(effectiveTotalCount / pageSize)) @@ -779,23 +816,14 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article {/* Breaking and Featured are the two flags an editor sets from the article form, and the two they need to audit: which stories are - still flagged as breaking, and what is currently pinned. Both are - three-state, so "not flagged" is reachable rather than implied. */} + still flagged as breaking, and what is currently pinned. Each one + narrows to its flag or does nothing, so it is a switch rather than a + row of chips -- there is no third thing to pick. */}
- Breaking -
- - - -
-
- -
- Featured -
- - - + Flags +
+ +
From 1dfc2a59eb905a3e2f9836ac8bd35d1f4fb6c12b Mon Sep 17 00:00:00 2001 From: ssavutu Date: Thu, 3 Sep 2026 23:38:04 -0400 Subject: [PATCH 2/2] Drop the unflagged half of the flag filters ?breaking=false and ?featured=false answered with the articles that do NOT carry the flag. Nothing asks for that: the screen's switches cannot, and a switch is the only client there is. It was carried on the theory that someone would want it later, which is how a second, subtler code path stays in the listing query earning nothing. What goes with it is the COALESCE(col, 0) = 0 form that made it correct. Both columns are nullable and the WordPress archive holds NULL, so the unflagged half had to be spelled carefully or it would have answered with only the few hundred rows the CMS itself wrote. The remaining `= 1` needs no such care -- NULL simply is not 1 -- so removing the half removes the trap with it. The parameter still parses the same set of true spellings, including bare ?breaking. Anything else, false included, now means "do not filter", and the test pins that so it cannot quietly start inverting instead. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01L4qBhBdQto1yNp1zP7VLYc --- server/docs/docs.go | 4 +- server/docs/swagger.json | 4 +- server/docs/swagger.yaml | 8 +-- .../article_flag_filters_integration_test.go | 27 +++++---- server/internal/handlers/handlers.go | 55 +++++++------------ 5 files changed, 42 insertions(+), 56 deletions(-) diff --git a/server/docs/docs.go b/server/docs/docs.go index 0d85cf0..b0d92c2 100644 --- a/server/docs/docs.go +++ b/server/docs/docs.go @@ -144,13 +144,13 @@ const docTemplate = `{ }, { "type": "boolean", - "description": "When true, return only articles flagged as breaking news; when false, only those not flagged.", + "description": "When true, return only articles flagged as breaking news. Any other value does not filter.", "name": "breaking", "in": "query" }, { "type": "boolean", - "description": "When true, return only featured (pinned) articles; when false, only those not featured.", + "description": "When true, return only featured (pinned) articles. Any other value does not filter.", "name": "featured", "in": "query" }, diff --git a/server/docs/swagger.json b/server/docs/swagger.json index f835a1c..cfa2147 100644 --- a/server/docs/swagger.json +++ b/server/docs/swagger.json @@ -141,13 +141,13 @@ }, { "type": "boolean", - "description": "When true, return only articles flagged as breaking news; when false, only those not flagged.", + "description": "When true, return only articles flagged as breaking news. Any other value does not filter.", "name": "breaking", "in": "query" }, { "type": "boolean", - "description": "When true, return only featured (pinned) articles; when false, only those not featured.", + "description": "When true, return only featured (pinned) articles. Any other value does not filter.", "name": "featured", "in": "query" }, diff --git a/server/docs/swagger.yaml b/server/docs/swagger.yaml index 42fb8a2..52310bc 100644 --- a/server/docs/swagger.yaml +++ b/server/docs/swagger.yaml @@ -1362,13 +1362,13 @@ paths: in: query name: slug type: string - - description: When true, return only articles flagged as breaking news; when - false, only those not flagged. + - description: When true, return only articles flagged as breaking news. Any + other value does not filter. in: query name: breaking type: boolean - - description: When true, return only featured (pinned) articles; when false, - only those not featured. + - description: When true, return only featured (pinned) articles. Any other + value does not filter. in: query name: featured type: boolean diff --git a/server/internal/handlers/article_flag_filters_integration_test.go b/server/internal/handlers/article_flag_filters_integration_test.go index ec85a2e..6a1b999 100644 --- a/server/internal/handlers/article_flag_filters_integration_test.go +++ b/server/internal/handlers/article_flag_filters_integration_test.go @@ -12,14 +12,14 @@ import ( db "server/internal/database" ) -// The Articles screen offers Breaking and Featured as filters, so both have to +// The Articles screen offers Breaking and Featured as switches, so both have to // narrow the listing on their own and in combination with the section filter // the editor may already have set. // -// The awkward part is the "off" half. Both columns are nullable and the -// WordPress archive leaves them NULL rather than 0, so `breaking_news` = 0 -// matches none of it; a filter written that way would answer "not breaking" -// with only the handful of rows the CMS itself has written. +// The seed keeps a row with both flags NULL, which is the shape the WordPress +// archive is in. It must stay out of every "on" result: `= 1` is false for +// NULL, and a truthiness test written some other way could let nine thousand +// imported rows into a list of breaking stories. func seedFlagFilterArticles(t *testing.T, conn *sql.DB) { t.Helper() ctx := context.Background() @@ -89,21 +89,20 @@ func TestArticleFlagFiltersHTTP(t *testing.T) { query: "?breaking=true", want: []string{"breaking-campus-story", "breaking-sports-story"}, }, - { - // The NULL row has to be here, or "not breaking" hides the archive. - name: "not breaking", - query: "?breaking=false", - want: []string{"featured-campus-story", "imported-campus-story", "ordinary-campus-story"}, - }, { name: "featured only", query: "?featured=true", want: []string{"breaking-sports-story", "featured-campus-story"}, }, { - name: "not featured", - query: "?featured=false", - want: []string{"breaking-campus-story", "imported-campus-story", "ordinary-campus-story"}, + // A switch cannot ask for the unflagged half, so false is inert. The + // failure this pins is it being read as "not breaking" instead. + name: "false does not filter", + query: "?breaking=false", + want: []string{ + "breaking-campus-story", "breaking-sports-story", "featured-campus-story", + "imported-campus-story", "ordinary-campus-story", + }, }, { // Both at once: the article that is breaking AND pinned. diff --git a/server/internal/handlers/handlers.go b/server/internal/handlers/handlers.go index 9de4367..322ca43 100644 --- a/server/internal/handlers/handlers.go +++ b/server/internal/handlers/handlers.go @@ -1260,8 +1260,8 @@ func GetAuthorArticles(conn *sql.DB) http.HandlerFunc { // @Param archived query bool false "When true, return only soft-deleted articles. Ignored for unauthenticated callers." // @Param title query string false "Filter by title (partial match)" // @Param slug query string false "Filter by slug (partial match)" -// @Param breaking query bool false "When true, return only articles flagged as breaking news; when false, only those not flagged." -// @Param featured query bool false "When true, return only featured (pinned) articles; when false, only those not featured." +// @Param breaking query bool false "When true, return only articles flagged as breaking news. Any other value does not filter." +// @Param featured query bool false "When true, return only featured (pinned) articles. Any other value does not filter." // @Param sort_by query string false "Sort field" Enums(title,slug,creation_date,published_date,status,comment_status) // @Param sort_direction query string false "Sort direction" Enums(asc,desc) // @Success 200 {object} models.ArticlesResponse @@ -1708,49 +1708,36 @@ func articleQueryFilters(r *http.Request, params ArticleParams) ([]string, []any args = append(args, "%"+slug+"%") } - // Both columns are nullable and default to 0, and rows imported from - // WordPress leave them NULL rather than 0. COALESCE on the "off" side is - // what keeps ?breaking=false from hiding the whole archive, since - // `breaking_news` = 0 is never true for NULL. - if breaking, ok := parseBoolFilter(q, "breaking"); ok { - if breaking { - conditions = append(conditions, "`breaking_news` = 1") - } else { - conditions = append(conditions, "COALESCE(`breaking_news`, 0) = 0") - } + // Both flags narrow to the articles that carry them. `priority` is the + // featured/pinned column; is_featured is the name it goes by over the wire, + // and GetFeaturedArticles reads the same one. + if flagFilterOn(q, "breaking") { + conditions = append(conditions, "`breaking_news` = 1") } - - // `priority` is the featured/pinned flag; is_featured is the name it goes by - // over the wire. GetFeaturedArticles reads the same column. - if featured, ok := parseBoolFilter(q, "featured"); ok { - if featured { - conditions = append(conditions, "`priority` = 1") - } else { - conditions = append(conditions, "COALESCE(`priority`, 0) = 0") - } + if flagFilterOn(q, "featured") { + conditions = append(conditions, "`priority` = 1") } return conditions, args } -// parseBoolFilter reads an optional boolean query param. The second result says -// whether the caller asked at all, so an absent param and an explicit false stay -// distinguishable: absent means "do not filter", false means "only the ones that -// are off". A value that parses as neither is treated as absent rather than -// guessed at, so a typo widens the listing instead of silently narrowing it to -// the wrong half. -func parseBoolFilter(q url.Values, key string) (bool, bool) { +// flagFilterOn reports whether a flag filter is switched on. Absent, false and +// unparseable all mean "do not filter", which is the whole of the contract: the +// parameter is a switch, and a switch cannot ask for the unflagged half. +// +// `= 1` rather than a truthiness test because both columns are nullable, and +// NULL is what the WordPress archive holds -- it must read as unflagged, not as +// unknown. +func flagFilterOn(q url.Values, key string) bool { if _, provided := q[key]; !provided { - return false, false + return false } switch strings.ToLower(strings.TrimSpace(q.Get(key))) { + // Bare ?breaking is the HTML-form spelling of true. case "", "1", "true", "yes": - // Bare ?breaking is the HTML-form spelling of true. - return true, true - case "0", "false", "no": - return false, true + return true default: - return false, false + return false } }