fix(feed): round-2 deep-review remediations (incl. a real coerce-on-seed bug)
The re-review of the fix round surfaced 4 findings; fixing the coverage one
exposed that the C1 migration was ineffective:
- FeedFiltersProvider.loadAccountFilters: run coerceTagModes on the RAW stored
blob, not the object merged over DEFAULT_FILTERS. The merge injects
DEFAULT_TAG_MODES (all "or"), which coerceTagModes prefers over the legacy
scalar `tagMode` — so the migration never fired. Now a persisted legacy
tagMode:'and' actually migrates into tagModes.topic (proven by a new test).
- test_feed_views: the tag_mode_other=and case was vacuous (only chC carried
either "other" tag, so AND == OR). Give chA an "other" tag too so OR-other
{chA,chC} differs from AND-other {chC} — the conjunctive path now narrows.
- DateFilterSelect: reset the custom reveal in useLayoutEffect (not useEffect)
so the correction commits before paint — no one-frame "Custom range" flicker
on the trigger after an external clear.
- SavedViewsProvider: extract the sticky-view tie-break to a pure, tested
lib/savedViews.ts (preferSticky).
New tests: savedViews (preferSticky), FeedFiltersProvider.dom (legacy tagMode
migration on the localStorage/default-view seed).
This commit is contained in:
@@ -105,11 +105,12 @@ def _seed_tagged(db, make_user):
|
||||
chans = {c: Channel(id=c, title=c, subscriber_count=10) for c in ("chA", "chB", "chC", "chD")}
|
||||
db.add_all(chans.values())
|
||||
db.flush()
|
||||
# channel -> (system) tag keys. chA carries two languages (L1+L2) and chC two "other" tags
|
||||
# (O1+O2) so the per-category any/all match narrows non-trivially for language and other, not
|
||||
# only topic (chA/chB share topic tags T1/T2).
|
||||
# channel -> (system) tag keys. chA carries two languages (L1+L2); chC two "other" tags (O1+O2)
|
||||
# AND chA carries O1 too — so the per-category any/all match narrows NON-TRIVIALLY (AND != OR) for
|
||||
# language and other, not only topic: e.g. other {O1,O2} → OR {chA,chC} vs AND {chC}; language
|
||||
# {L1,L2} → OR {chA,chB,chC} vs AND {chA}. (chA/chB share topic tags T1/T2 for the topic case.)
|
||||
links = {
|
||||
"chA": ["L1", "L2", "T1"],
|
||||
"chA": ["L1", "L2", "T1", "O1"],
|
||||
"chB": ["L1", "T2"],
|
||||
"chC": ["L2", "T1", "O1", "O2"],
|
||||
"chD": [],
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useEffect, useState } from "react";
|
||||
import { useLayoutEffect, useState } from "react";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { CalendarDays, Check, ChevronDown } from "lucide-react";
|
||||
import { PopoverItem, PopoverSurface, useAnchoredPopover } from "./AnchoredPopover";
|
||||
@@ -36,7 +36,9 @@ export default function DateFilterSelect({
|
||||
// is closed, drop the custom reveal so the trigger reverts to "Any time" instead of lingering on
|
||||
// "Custom range" with empty fields. Guarded on !open so it never closes a freshly-picked Custom the
|
||||
// user hasn't filled in yet — that stays open until they close it (handled by onOpenChange below).
|
||||
useEffect(() => {
|
||||
// useLayoutEffect (not useEffect) so the reset commits BEFORE paint — otherwise the trigger flashes
|
||||
// one stale "Custom range" frame between the external clear and this reset.
|
||||
useLayoutEffect(() => {
|
||||
if (!open && customOpen && !filters.dateFrom && !filters.dateTo) setCustomOpen(false);
|
||||
}, [open, customOpen, filters.dateFrom, filters.dateTo]);
|
||||
|
||||
|
||||
@@ -0,0 +1,45 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// loadAccountFilters is the localStorage seed path for the feed filters. It must migrate a persisted
|
||||
// legacy global `tagMode` (older builds wrote it) into the per-category `tagModes` — the same coercion
|
||||
// the saved-view and URL paths do — so an upgrading user's topic any/all match isn't silently lost.
|
||||
import { describe, it, expect, beforeEach } from "vitest";
|
||||
import { loadAccountFilters } from "./FeedFiltersProvider";
|
||||
import { accountKey, LS } from "../lib/storage";
|
||||
|
||||
const ID = 7;
|
||||
|
||||
beforeEach(() => localStorage.clear());
|
||||
|
||||
describe("loadAccountFilters — legacy tagMode migration on seed", () => {
|
||||
it("migrates a persisted legacy tagMode:'and' from the last-session blob into topic", () => {
|
||||
localStorage.setItem(accountKey(LS.filters, ID)!, JSON.stringify({ q: "hi", tagMode: "and" }));
|
||||
const f = loadAccountFilters(ID);
|
||||
expect(f.tagModes).toEqual({ language: "or", topic: "and", other: "or" });
|
||||
expect(f.q).toBe("hi"); // other fields preserved through the merge
|
||||
});
|
||||
|
||||
it("migrates a legacy tagMode:'and' from the default-view mirror when no last-session blob exists", () => {
|
||||
localStorage.setItem(
|
||||
accountKey(LS.defaultViewFilters, ID)!,
|
||||
JSON.stringify({ tagMode: "and" }),
|
||||
);
|
||||
expect(loadAccountFilters(ID).tagModes.topic).toBe("and");
|
||||
});
|
||||
|
||||
it("leaves tagModes at defaults when nothing is stored", () => {
|
||||
expect(loadAccountFilters(ID).tagModes).toEqual({ language: "or", topic: "or", other: "or" });
|
||||
});
|
||||
|
||||
it("preserves an already-migrated per-category tagModes blob", () => {
|
||||
localStorage.setItem(
|
||||
accountKey(LS.filters, ID)!,
|
||||
JSON.stringify({ tagModes: { language: "and", topic: "or", other: "and" } }),
|
||||
);
|
||||
expect(loadAccountFilters(ID).tagModes).toEqual({
|
||||
language: "and",
|
||||
topic: "or",
|
||||
other: "and",
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -18,7 +18,7 @@ import {
|
||||
} from "../lib/api";
|
||||
import { filterParamUpdates, hasFilterParams, paramsToFilters } from "../lib/urlState";
|
||||
import { useUrlMirror } from "../lib/useUrlMirror";
|
||||
import { accountKey, LS, readJSON, readMerged, writeJSON } from "../lib/storage";
|
||||
import { accountKey, LS, readJSON, writeJSON } from "../lib/storage";
|
||||
import { useNavigation } from "./NavigationProvider";
|
||||
|
||||
// Owns the GLOBAL feed-filter state (the feed's scope/tags/content/sort/show), lifted out of the App
|
||||
@@ -55,7 +55,7 @@ const FILTERS_KEY = LS.filters;
|
||||
// default saved view wins over the last-session filters (SavedViewsWidget mirrors it here). When
|
||||
// the account isn't known yet (first load, before the tab is pinned), start from defaults; the
|
||||
// post-login effect loads the real ones once the id arrives.
|
||||
function loadAccountFilters(id: number | null): FeedFilters {
|
||||
export function loadAccountFilters(id: number | null): FeedFilters {
|
||||
// Your last-applied filters win, so a reload keeps whatever view you're on (a saved view you
|
||||
// picked, or manual tweaks) — setFilters persists them under LS.filters on every change. The
|
||||
// starred default view only SEEDS a fresh account that has never stored filters: it drives the
|
||||
@@ -68,18 +68,18 @@ function loadAccountFilters(id: number | null): FeedFilters {
|
||||
} catch {
|
||||
/* localStorage may be unavailable */
|
||||
}
|
||||
// Migrate a persisted legacy global `tagMode` into the per-category `tagModes` on every seed path
|
||||
// (readMerged is a shallow merge that leaves the legacy scalar as ignored junk), matching what the
|
||||
// saved-view (asFilters) and URL (paramsToFilters) paths already do.
|
||||
// Migrate a persisted legacy global `tagMode` into the per-category `tagModes` on every seed path,
|
||||
// matching what the saved-view (asFilters) and URL (paramsToFilters) paths already do. coerceTagModes
|
||||
// MUST run on the RAW stored blob, not the object merged over DEFAULT_FILTERS: the merge injects
|
||||
// DEFAULT_TAG_MODES (all "or"), which coerceTagModes prefers over the legacy scalar and would mask it.
|
||||
if (hasStored && fKey) {
|
||||
const merged = readMerged(fKey, DEFAULT_FILTERS);
|
||||
return { ...merged, tagModes: coerceTagModes(merged) };
|
||||
const raw = readJSON<Partial<FeedFilters>>(fKey, {});
|
||||
return { ...DEFAULT_FILTERS, ...raw, tagModes: coerceTagModes(raw) };
|
||||
}
|
||||
const dvKey = accountKey(LS.defaultViewFilters, id);
|
||||
const def = dvKey ? readJSON<FeedFilters | null>(dvKey, null) : null;
|
||||
const def = dvKey ? readJSON<Partial<FeedFilters> | null>(dvKey, null) : null;
|
||||
if (def) {
|
||||
const merged = { ...DEFAULT_FILTERS, ...def };
|
||||
return { ...merged, tagModes: coerceTagModes(merged) };
|
||||
return { ...DEFAULT_FILTERS, ...def, tagModes: coerceTagModes(def) };
|
||||
}
|
||||
return { ...DEFAULT_FILTERS };
|
||||
}
|
||||
|
||||
@@ -10,6 +10,7 @@ import {
|
||||
type SavedView,
|
||||
} from "../lib/api";
|
||||
import { filtersToParams } from "../lib/urlState";
|
||||
import { preferSticky } from "../lib/savedViews";
|
||||
import { notifyActionError } from "../lib/actionErrors";
|
||||
import { accountKey, LS, writeJSON } from "../lib/storage";
|
||||
import { useConfirm } from "./ConfirmProvider";
|
||||
@@ -85,12 +86,16 @@ export function SavedViewsProvider({ children }: { children: ReactNode }) {
|
||||
}, [viewsQuery.data]);
|
||||
|
||||
const currentKey = keyOf(filters);
|
||||
const activeView = useMemo(() => {
|
||||
const matches = views.filter((v) => keyOf(v.filters) === currentKey);
|
||||
const activeView = useMemo(
|
||||
// When several views capture identical filters, prefer the one the user explicitly applied
|
||||
// (sticky) so apply(B) isn't silently re-attributed to an earlier duplicate A; else the first.
|
||||
return matches.find((v) => v.id === stickyId) ?? matches[0] ?? null;
|
||||
}, [views, currentKey, stickyId]);
|
||||
() =>
|
||||
preferSticky(
|
||||
views.filter((v) => keyOf(v.filters) === currentKey),
|
||||
stickyId,
|
||||
),
|
||||
[views, currentKey, stickyId],
|
||||
);
|
||||
// An exact match becomes the sticky selection, so filters that happen to equal a view (deep-link,
|
||||
// manual, or an applied view) name that view in the chip and keep naming it after a later tweak.
|
||||
useEffect(() => {
|
||||
|
||||
@@ -0,0 +1,26 @@
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { preferSticky } from "./savedViews";
|
||||
|
||||
const v = (id: number) => ({ id, name: `v${id}` });
|
||||
|
||||
describe("preferSticky", () => {
|
||||
it("returns null when nothing matches", () => {
|
||||
expect(preferSticky([], 3)).toBeNull();
|
||||
expect(preferSticky([], null)).toBeNull();
|
||||
});
|
||||
|
||||
it("returns the only match regardless of stickyId", () => {
|
||||
expect(preferSticky([v(5)], null)?.id).toBe(5);
|
||||
expect(preferSticky([v(5)], 99)?.id).toBe(5);
|
||||
});
|
||||
|
||||
it("prefers the sticky view among several identical-filter matches", () => {
|
||||
// A (id 1) is earlier in the list, B (id 2) was explicitly applied (sticky).
|
||||
expect(preferSticky([v(1), v(2)], 2)?.id).toBe(2);
|
||||
});
|
||||
|
||||
it("falls back to the first match when the sticky id isn't among the matches", () => {
|
||||
expect(preferSticky([v(1), v(2)], 99)?.id).toBe(1);
|
||||
expect(preferSticky([v(1), v(2)], null)?.id).toBe(1);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,12 @@
|
||||
// Pure selection helpers for saved ("personal") views, split out of SavedViewsProvider so the
|
||||
// tie-break logic is unit-testable without mounting the provider (its React Query / context deps).
|
||||
|
||||
/** Among the views whose filters match the live filters, pick the one to treat as active: prefer the
|
||||
* explicitly-applied ("sticky") view when several capture identical filters, so apply(B) isn't
|
||||
* silently re-attributed to an earlier duplicate A; otherwise the first match. Null if none match. */
|
||||
export function preferSticky<T extends { id: number }>(
|
||||
matches: T[],
|
||||
stickyId: number | null,
|
||||
): T | null {
|
||||
return matches.find((v) => v.id === stickyId) ?? matches[0] ?? null;
|
||||
}
|
||||
Reference in New Issue
Block a user