From a47a5928676af13caed7212c9063ed24ae4732ca Mon Sep 17 00:00:00 2001 From: Bretton Date: Tue, 4 Aug 2026 00:28:52 -0700 Subject: [PATCH] fix(feeds): migrate sort controls to Coves API and fix feed cache staleness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The community feed's sort selector rendered blank and sorting silently fell back to "hot": PostListShell still used the legacy Lemmy selector (capitalized values like TopAll) while the loaders speak lowercase Coves sorts. Fixing that surfaced deeper issues — a route-level feed cache that could serve the wrong community after client-side navigation, and persisted settings polluted with legacy values. A six-reviewer multi-model pass then hardened the settings import path and the cache's concurrency semantics. Changes: - Replace legacy Sort.svelte/Location.svelte with the Coves-native SortMenu (hot/top/new + hour..all timeframes) in PostListShell and settings; migrate command palette entries to valid sort/type URLs - Fix stale feed cache: init closures read only their load() params, feed() refreshes the cached fetcher (setFetch), and a generation counter discards superseded fetch results/errors - Make settings.defaultSort a live, validated setting: resolveFeedSort gives URL params precedence, applies the saved timeframe only when sort came from settings, and grace-maps legacy URLs (?sort=TopWeek) - Persist defaults only on explicit user selection (SortMenu/FeedTabs, after successful navigation) so shared links and legacy bookmarks can't rewrite a viewer's saved preferences - Harden settings import: plain-object check, detached mergeDeep candidate (drops __proto__/unknown keys, CWE-1321), total normalizers that never throw on foreign input; clone on reset; guard localStorage writes (private-mode Safari) - Normalize legacy persisted values (New/TopAll/Subscribed→timeline) on load and import; tighten settings types to the Coves unions - Single TIMEFRAME_OPTIONS source of truth (type + validation derived); add hour/year timeframes end to end - Tests: 45 new across feed cache races, loader regressions, resolveFeedSort precedence, normalizers, and settings import (759 passing) Co-Authored-By: Claude Opus 5 (1M context) --- src/lib/app/i18n/en.json | 4 + src/lib/app/settings.svelte.ts | 124 ++++++++--- src/lib/app/settings.test.ts | 209 +++++++++++++++++++ src/lib/app/sort.test.ts | 207 +++++++++++++++++- src/lib/app/sort.ts | 129 ++++++++++-- src/lib/feature/feeds/feed.svelte.test.ts | 146 +++++++++++++ src/lib/feature/feeds/feed.svelte.ts | 38 +++- src/lib/feature/filter/FeedTabs.svelte | 4 + src/lib/feature/filter/Location.svelte | 68 ------ src/lib/feature/filter/Sort.svelte | 135 ------------ src/lib/feature/filter/SortMenu.svelte | 65 ++++-- src/lib/ui/layout/pages/PostListShell.svelte | 96 ++++++--- src/lib/ui/navbar/commands/actions.svelte.ts | 115 ++-------- src/routes/+page.svelte | 9 +- src/routes/+page.ts | 6 +- src/routes/c/[handle=handle]/+page.svelte | 1 + src/routes/c/[handle=handle]/+page.ts | 24 ++- src/routes/c/[handle=handle]/page.test.ts | 184 ++++++++++++++++ src/routes/settings/+layout.svelte | 19 +- src/routes/settings/app/+page.svelte | 47 ++++- 20 files changed, 1193 insertions(+), 437 deletions(-) create mode 100644 src/lib/app/settings.test.ts create mode 100644 src/lib/feature/feeds/feed.svelte.test.ts delete mode 100644 src/lib/feature/filter/Location.svelte delete mode 100644 src/lib/feature/filter/Sort.svelte create mode 100644 src/routes/c/[handle=handle]/page.test.ts diff --git a/src/lib/app/i18n/en.json b/src/lib/app/i18n/en.json index 547269d4..ac83b346 100644 --- a/src/lib/app/i18n/en.json +++ b/src/lib/app/i18n/en.json @@ -84,6 +84,7 @@ "time": { "label": "Period", "all": "All time", + "year": "Past year", "9months": "9 months", "6months": "6 months", "3months": "3 months", @@ -103,6 +104,7 @@ "newcomments": "New Replies" }, "feed": { + "label": "Feed", "discover": "Discover", "forYou": "For You" }, @@ -815,7 +817,9 @@ "unblockUser": "Unblocked that user.", "purgeUser": "Purged that user.", "settingsImport": "Successfully imported settings", + "settingsImportFailed": "Couldn't import those settings. Paste the JSON object from a settings export.", "settingsImportWarning": "The imported settings don't seem valid. Are you sure you want to import this?", + "sortFailed": "Couldn't change the sort. Please try again.", "userLoading": "Still loading your user data...", "sessionExpired": "Your session has expired. Please log in again.", "serverUnreachable": "Can't reach the server right now. You may appear logged out, but your session is preserved." diff --git a/src/lib/app/settings.svelte.ts b/src/lib/app/settings.svelte.ts index 360bb043..0ed23440 100644 --- a/src/lib/app/settings.svelte.ts +++ b/src/lib/app/settings.svelte.ts @@ -2,19 +2,15 @@ import { browser } from '$app/environment' import { env } from '$env/dynamic/public' import { locale } from './i18n' import { mergeDeep } from './merge' -import { normalizeCommentSort } from './sort' - -/** - * Sort type values for the Coves API. - * - * The `| (string & {})` fallback allows values from env vars and localStorage - * that may not match these known literals. - */ -type SortType = 'hot' | 'new' | 'top' | (string & {}) - -type ListingType = 'discover' | 'timeline' | (string & {}) - -type CommentSortType = 'hot' | 'top' | 'new' | (string & {}) +import { + normalizeCommentSort, + normalizeListing, + normalizeSort, + normalizeTimeframe, + type CovesListingType, + type CovesSortType, + type CovesTimeframe, +} from './sort' export type View = 'cozy' | 'compact' @@ -41,11 +37,16 @@ interface Settings { view: View + /** + * Defaults applied when a feed URL carries no sort params. Every write path + * runs these through {@link normalizeSettings}, so they always hold values + * the Coves API accepts. + */ defaultSort: { - sort: SortType - feed: ListingType - comments: CommentSortType - timeframe: string + sort: CovesSortType + feed: CovesListingType + comments: CovesSortType + timeframe: CovesTimeframe } hidePosts: { deleted: boolean @@ -115,10 +116,10 @@ export const defaultSettings: Settings = { expandableImages: toBool(env.PUBLIC_EXPANDABLE_IMAGES) ?? true, markReadPosts: toBool(env.PUBLIC_MARK_READ_POSTS) ?? true, defaultSort: { - sort: (env.PUBLIC_DEFAULT_FEED_SORT ?? 'hot') as SortType, - feed: (env.PUBLIC_DEFAULT_FEED ?? 'discover') as ListingType, - comments: (env.PUBLIC_DEFAULT_COMMENT_SORT ?? 'hot') as CommentSortType, - timeframe: env.PUBLIC_DEFAULT_FEED_TIMEFRAME ?? 'all', + sort: normalizeSort(env.PUBLIC_DEFAULT_FEED_SORT ?? 'hot'), + feed: normalizeListing(env.PUBLIC_DEFAULT_FEED ?? 'discover'), + comments: normalizeCommentSort(env.PUBLIC_DEFAULT_COMMENT_SORT ?? 'hot'), + timeframe: normalizeTimeframe(env.PUBLIC_DEFAULT_FEED_TIMEFRAME ?? 'all'), }, hidePosts: { deleted: toBool(env.PUBLIC_HIDE_DELETED) ?? false, @@ -220,14 +221,72 @@ function getInitialSettings(defaultValue: Settings): Settings { } } +/** + * Coerces the feed defaults to values the Coves API accepts, in place. + * + * Legacy Lemmy-era values ('Hot', 'TopWeek', 'Subscribed', ...) persisted by + * older versions of the app, and anything supplied by a hand-edited settings + * import, would otherwise render blank in the settings selects and be rejected + * by `mapSort`/`mapListing` on every feed load. Call this on any path that can + * introduce foreign values. + */ +export function normalizeSettings(target: Settings): void { + target.defaultSort.comments = normalizeCommentSort( + target.defaultSort.comments, + ) + target.defaultSort.sort = normalizeSort(target.defaultSort.sort) + target.defaultSort.timeframe = normalizeTimeframe( + target.defaultSort.timeframe, + ) + target.defaultSort.feed = normalizeListing(target.defaultSort.feed) +} + +/** + * Restores every setting to its default. + * + * The clone matters: assigning `defaultSettings` directly would alias its + * nested objects (`defaultSort`, `embeds`, ...) into the live state, so the + * next settings edit would mutate the defaults singleton and leave the user + * with nothing to reset to. + */ +export function resetSettings(): void { + Object.assign(settings, cloneDefaults(defaultSettings)) +} + +/** + * Replaces the live settings with a user-supplied JSON export. + * + * The payload is layered onto a *detached* clone of the defaults rather than + * onto the live object: `mergeDeep` keeps only keys the current schema defines + * and only values whose shape matches, which is also what drops a crafted + * `__proto__` key — spreading the parsed JSON into `Object.assign` would hand + * it the live settings object's prototype. Building the candidate first also + * means a payload that fails partway leaves the live settings untouched + * instead of half-written. + * + * @throws {SyntaxError} if the text is not JSON. + * @throws {Error} if the JSON is not an object (`42`, `"oops"`, `[]`), which + * would otherwise merge nothing and silently reset every setting. + */ +export function importSettings(json: string): void { + const parsed: unknown = JSON.parse(json) + + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { + throw new Error('Settings must be a JSON object') + } + + const candidate = mergeDeep( + cloneDefaults(defaultSettings) as unknown as Record, + parsed, + ) as unknown as Settings + normalizeSettings(candidate) + + Object.assign(settings, candidate) +} + function createSettingsState(initial: Settings): Settings { const loaded = getInitialSettings(initial) - // Migrate legacy capitalized comment sort values ('Hot', 'Top', 'TopAll', - // 'Old', ...) persisted by older app versions (or set via env) to valid - // lowercase Coves values. - loaded.defaultSort.comments = normalizeCommentSort( - loaded.defaultSort.comments, - ) + normalizeSettings(loaded) const settings = $state(loaded) return settings } @@ -236,7 +295,16 @@ export const settings = createSettingsState(defaultSettings) $effect.root(() => { $effect(() => { - localStorage.setItem('settings', JSON.stringify(settings)) + try { + localStorage.setItem('settings', JSON.stringify(settings)) + } catch (err) { + // Storage can be unavailable or full (private browsing, blocked + // cookies). Losing persistence must not take the reactive graph with it. + console.error( + '[settings] Failed to persist settings:', + err instanceof Error ? err.message : String(err), + ) + } if (settings.language) { locale.set(settings.language) diff --git a/src/lib/app/settings.test.ts b/src/lib/app/settings.test.ts new file mode 100644 index 00000000..4e26b9b5 --- /dev/null +++ b/src/lib/app/settings.test.ts @@ -0,0 +1,209 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +// --------------------------------------------------------------------------- +// Mocks +// +// `settings.svelte.ts` reads localStorage during boot and writes to it from a +// module-level `$effect`, so both need to exist before the import is evaluated. +// `browser: false` keeps the boot path on the defaults instead of whatever a +// previous test left in the stub. +// --------------------------------------------------------------------------- + +vi.mock('$app/environment', () => ({ + browser: false, + dev: false, + building: false, + version: 'test', +})) + +vi.hoisted(() => { + const store = new Map() + globalThis.localStorage = { + getItem: (key: string) => store.get(key) ?? null, + setItem: (key: string, value: string) => void store.set(key, value), + removeItem: (key: string) => void store.delete(key), + clear: () => store.clear(), + key: () => null, + get length() { + return store.size + }, + } as Storage +}) + +import { + defaultSettings, + importSettings, + normalizeSettings, + resetSettings, + settings, +} from './settings.svelte' + +/** Reaches past the schema types to plant the foreign values under test. */ +function loosen(value: object): Record { + return value as Record +} + +describe('normalizeSettings', () => { + it('migrates legacy Lemmy-era values written by older versions', () => { + const target = structuredClone(defaultSettings) + Object.assign(loosen(target.defaultSort), { + sort: 'TopWeek', + feed: 'Subscribed', + comments: 'Old', + timeframe: 'TopNineMonths', + }) + + normalizeSettings(target) + + expect(target.defaultSort).toMatchObject({ + sort: 'top', + feed: 'timeline', + comments: 'hot', + timeframe: 'all', + }) + }) + + it('fills in fields a partial payload left missing', () => { + const target = structuredClone(defaultSettings) + const defaultSort = loosen(target.defaultSort) + delete defaultSort.timeframe + delete defaultSort.feed + + expect(() => normalizeSettings(target)).not.toThrow() + expect(target.defaultSort.timeframe).toBe('all') + expect(target.defaultSort.feed).toBe('discover') + }) + + it('coerces non-string leaves rather than throwing', () => { + // A hand-edited import or corrupted localStorage can put any JSON value + // here, and this runs during app boot — throwing would brick startup. + const target = structuredClone(defaultSettings) + Object.assign(loosen(target.defaultSort), { + sort: 42, + feed: null, + comments: ['top'], + timeframe: { value: 'week' }, + }) + + expect(() => normalizeSettings(target)).not.toThrow() + expect(target.defaultSort).toMatchObject({ + sort: 'hot', + feed: 'discover', + comments: 'hot', + timeframe: 'all', + }) + }) + + it('leaves already-valid values alone', () => { + const target = structuredClone(defaultSettings) + Object.assign(target.defaultSort, { + sort: 'top', + feed: 'timeline', + comments: 'new', + timeframe: 'week', + }) + + normalizeSettings(target) + + expect(target.defaultSort).toMatchObject({ + sort: 'top', + feed: 'timeline', + comments: 'new', + timeframe: 'week', + }) + }) +}) + +describe('importSettings', () => { + beforeEach(() => { + resetSettings() + }) + + it('applies the keys a payload names and defaults the rest', () => { + settings.view = 'compact' + + importSettings('{"defaultSort":{"sort":"top","timeframe":"week"}}') + + expect(settings.defaultSort.sort).toBe('top') + expect(settings.defaultSort.timeframe).toBe('week') + // Absent keys come from the defaults, not from the pre-import state. + expect(settings.view).toBe(defaultSettings.view) + }) + + it('accepts a partial defaultSort without leaving fields undefined', () => { + // Regression: a shallow merge left timeframe/comments/feed undefined and + // normalization then threw on undefined.toLowerCase(), after the live + // settings had already been overwritten. + expect(() => importSettings('{"defaultSort":{"sort":"hot"}}')).not.toThrow() + + expect(settings.defaultSort).toMatchObject({ + sort: 'hot', + timeframe: 'all', + comments: 'hot', + feed: 'discover', + }) + }) + + it('normalizes legacy values in the payload', () => { + importSettings('{"defaultSort":{"sort":"TopAll","feed":"Subscribed"}}') + + expect(settings.defaultSort.sort).toBe('top') + expect(settings.defaultSort.feed).toBe('timeline') + }) + + it('ignores a __proto__ key instead of polluting prototypes', () => { + importSettings('{"__proto__":{"polluted":"yes"}}') + + expect(loosen({}).polluted).toBeUndefined() + expect(loosen(settings).polluted).toBeUndefined() + expect(Object.getPrototypeOf(settings)).toBe(Object.prototype) + }) + + it('prunes unknown keys and values of the wrong shape', () => { + importSettings('{"bogusKey":1,"expandableImages":"yes","view":"compact"}') + + expect(loosen(settings).bogusKey).toBeUndefined() + expect(settings.expandableImages).toBe(defaultSettings.expandableImages) + expect(settings.view).toBe('compact') + }) + + it('rejects JSON that is not an object, leaving settings untouched', () => { + settings.view = 'compact' + + for (const payload of ['42', '"oops"', '[]', 'null']) { + expect(() => importSettings(payload)).toThrow() + } + + // Regression: these spread to nothing and silently reset every setting. + expect(settings.view).toBe('compact') + }) + + it('rejects unparseable text', () => { + settings.view = 'compact' + + expect(() => importSettings('not json')).toThrow(SyntaxError) + expect(() => importSettings('')).toThrow(SyntaxError) + expect(settings.view).toBe('compact') + }) +}) + +describe('resetSettings', () => { + it('restores the defaults', () => { + settings.view = 'compact' + settings.defaultSort.sort = 'top' + + resetSettings() + + expect(settings.view).toBe(defaultSettings.view) + expect(settings.defaultSort.sort).toBe(defaultSettings.defaultSort.sort) + }) + + it('does not alias the defaults into the live settings', () => { + // Regression: assigning defaultSettings directly shared its nested objects, + // so the next edit mutated the singleton the reset restores from. + resetSettings() + settings.defaultSort.sort = 'new' + + expect(defaultSettings.defaultSort.sort).toBe('hot') + }) +}) diff --git a/src/lib/app/sort.test.ts b/src/lib/app/sort.test.ts index e7627a50..54023c80 100644 --- a/src/lib/app/sort.test.ts +++ b/src/lib/app/sort.test.ts @@ -1,8 +1,14 @@ -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import { + isValidTimeframe, mapListing, mapSort, normalizeCommentSort, + normalizeListing, + normalizeSort, + normalizeTimeframe, + resolveFeedSort, + TIMEFRAME_OPTIONS, toLemmyCommentSort, } from './sort' @@ -22,6 +28,10 @@ describe('mapSort', () => { }) describe('top sort with timeframes', () => { + it('maps "top" with timeframe "hour"', () => { + expect(mapSort('top', 'hour')).toEqual({ sort: 'top', timeframe: 'hour' }) + }) + it('maps "top" with timeframe "day"', () => { expect(mapSort('top', 'day')).toEqual({ sort: 'top', timeframe: 'day' }) }) @@ -37,6 +47,10 @@ describe('mapSort', () => { }) }) + it('maps "top" with timeframe "year"', () => { + expect(mapSort('top', 'year')).toEqual({ sort: 'top', timeframe: 'year' }) + }) + it('maps "top" with timeframe "all"', () => { expect(mapSort('top', 'all')).toEqual({ sort: 'top', timeframe: 'all' }) }) @@ -80,6 +94,102 @@ describe('mapSort', () => { }) }) +describe('resolveFeedSort', () => { + const feedUrl = (query = ''): URL => new URL(`https://coves.test/${query}`) + const defaults = { sort: 'top', timeframe: 'week' } + + describe('URL params win', () => { + it('uses the URL sort and timeframe over the saved defaults', () => { + expect( + resolveFeedSort(feedUrl('?sort=top&timeframe=day'), defaults), + ).toEqual({ sort: 'top', timeframe: 'day' }) + }) + + it('uses the URL timeframe even when the sort came from settings', () => { + expect(resolveFeedSort(feedUrl('?timeframe=month'), defaults)).toEqual({ + sort: 'top', + timeframe: 'month', + }) + }) + + it('does not inherit the saved timeframe for an explicit URL sort', () => { + // A shared link means the same thing to everyone who opens it. + expect(resolveFeedSort(feedUrl('?sort=top'), defaults)).toEqual({ + sort: 'top', + timeframe: 'all', + }) + }) + }) + + describe('settings defaults', () => { + it('applies the saved timeframe when the sort also came from settings', () => { + expect(resolveFeedSort(feedUrl(), defaults)).toEqual({ + sort: 'top', + timeframe: 'week', + }) + }) + + it('ignores the saved timeframe for non-top saved sorts', () => { + const result = resolveFeedSort(feedUrl(), { + sort: 'hot', + timeframe: 'week', + }) + expect(result).toEqual({ sort: 'hot' }) + expect(result).not.toHaveProperty('timeframe') + }) + + it('falls back to "all" for an invalid saved timeframe', () => { + expect( + resolveFeedSort(feedUrl(), { sort: 'top', timeframe: 'TopWeek' }), + ).toEqual({ sort: 'top', timeframe: 'all' }) + }) + }) + + describe('legacy URL sorts', () => { + it('salvages a Lemmy-era bookmark to the nearest Coves sort', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + expect(resolveFeedSort(feedUrl('?sort=TopWeek'), defaults)).toEqual({ + sort: 'top', + timeframe: 'all', + }) + expect(resolveFeedSort(feedUrl('?sort=Hot'), defaults)).toEqual({ + sort: 'hot', + }) + expect(warn).toHaveBeenCalled() + + warn.mockRestore() + }) + + it('falls back to "hot" for sorts with no Coves equivalent', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + expect(resolveFeedSort(feedUrl('?sort=Controversial'), defaults)).toEqual( + { sort: 'hot' }, + ) + + warn.mockRestore() + }) + + it('treats an empty ?sort= as an explicit sort, not a missing one', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + // Present-but-empty means the saved 'top'/'week' pair is not inherited. + expect(resolveFeedSort(feedUrl('?sort='), defaults)).toEqual({ + sort: 'hot', + }) + + warn.mockRestore() + }) + }) + + it('keeps other query params out of the result', () => { + expect( + resolveFeedSort(feedUrl('?cursor=abc&type=timeline'), defaults), + ).toEqual({ sort: 'top', timeframe: 'week' }) + }) +}) + describe('mapListing', () => { describe('discover listing', () => { it('returns "discover" when authenticated', () => { @@ -145,6 +255,101 @@ describe('normalizeCommentSort', () => { }) }) +describe('normalizeListing', () => { + it('passes through valid listing types regardless of auth state', () => { + expect(normalizeListing('discover')).toBe('discover') + expect(normalizeListing('timeline')).toBe('timeline') + }) + + it('migrates legacy Lemmy listing types persisted by the old selector', () => { + expect(normalizeListing('All')).toBe('discover') + expect(normalizeListing('Local')).toBe('discover') + expect(normalizeListing('ModeratorView')).toBe('discover') + }) + + it('maps the legacy subscribed feed to the timeline', () => { + expect(normalizeListing('Subscribed')).toBe('timeline') + expect(normalizeListing('subscribed')).toBe('timeline') + }) + + it('coerces non-string values instead of throwing', () => { + expect(normalizeListing(undefined)).toBe('discover') + expect(normalizeListing(null)).toBe('discover') + expect(normalizeListing(42)).toBe('discover') + expect(normalizeListing({ sort: 'timeline' })).toBe('discover') + }) + + it('accepts capitalized Coves values', () => { + expect(normalizeListing('Timeline')).toBe('timeline') + }) + + it('falls back to "discover" for unrecognized values', () => { + expect(normalizeListing('bogus')).toBe('discover') + expect(normalizeListing('')).toBe('discover') + }) +}) + +describe('normalizeSort', () => { + it('passes through valid lowercase values', () => { + expect(normalizeSort('hot')).toBe('hot') + expect(normalizeSort('top')).toBe('top') + expect(normalizeSort('new')).toBe('new') + }) + + it('migrates legacy capitalized feed sorts persisted by the old selector', () => { + expect(normalizeSort('New')).toBe('new') + expect(normalizeSort('TopWeek')).toBe('top') + }) + + it('falls back to "hot" for sorts the Coves API does not support', () => { + expect(normalizeSort('Active')).toBe('hot') + expect(normalizeSort('MostComments')).toBe('hot') + expect(normalizeSort('')).toBe('hot') + }) + + it('coerces non-string values instead of throwing', () => { + // Corrupted localStorage and hand-edited imports reach these during boot. + expect(normalizeSort(undefined)).toBe('hot') + expect(normalizeSort(null)).toBe('hot') + expect(normalizeSort(42)).toBe('hot') + expect(normalizeSort(['top'])).toBe('hot') + }) +}) + +describe('normalizeTimeframe', () => { + it('passes through valid timeframes', () => { + expect(normalizeTimeframe('hour')).toBe('hour') + expect(normalizeTimeframe('year')).toBe('year') + expect(normalizeTimeframe('week')).toBe('week') + }) + + it('falls back to "all" for unsupported values', () => { + expect(normalizeTimeframe('9months')).toBe('all') + expect(normalizeTimeframe('')).toBe('all') + }) + + it('coerces non-string values instead of throwing', () => { + expect(normalizeTimeframe(undefined)).toBe('all') + expect(normalizeTimeframe(null)).toBe('all') + expect(normalizeTimeframe(7)).toBe('all') + }) +}) + +describe('TIMEFRAME_OPTIONS', () => { + it('is the single source of truth for timeframe validation', () => { + for (const option of TIMEFRAME_OPTIONS) { + expect(isValidTimeframe(option.value)).toBe(true) + expect(normalizeTimeframe(option.value)).toBe(option.value) + } + }) + + it('gives every timeframe a label key', () => { + for (const option of TIMEFRAME_OPTIONS) { + expect(option.labelKey).toMatch(/^filter\.sort\.top\.time\./) + } + }) +}) + describe('toLemmyCommentSort', () => { it('maps Coves values to capitalized Lemmy values', () => { expect(toLemmyCommentSort('hot')).toBe('Hot') diff --git a/src/lib/app/sort.ts b/src/lib/app/sort.ts index 712a58aa..3d7909cd 100644 --- a/src/lib/app/sort.ts +++ b/src/lib/app/sort.ts @@ -3,9 +3,27 @@ // --------------------------------------------------------------------------- export type CovesSortType = 'hot' | 'new' | 'top' -export type CovesTimeframe = 'day' | 'week' | 'month' | 'all' export type CovesListingType = 'discover' | 'timeline' +/** + * The timeframes `sort=top` accepts, ascending, each with its label key. + * + * Single source of truth for every place a viewer picks a timeframe — the sort + * menu, the settings default, the noscript form, the command palette — so the + * lists cannot drift apart. {@link CovesTimeframe} is derived from it, which + * is why a timeframe added here needs no other type change. + */ +export const TIMEFRAME_OPTIONS = [ + { value: 'hour', labelKey: 'filter.sort.top.time.hour' }, + { value: 'day', labelKey: 'filter.sort.top.time.day' }, + { value: 'week', labelKey: 'filter.sort.top.time.week' }, + { value: 'month', labelKey: 'filter.sort.top.time.month' }, + { value: 'year', labelKey: 'filter.sort.top.time.year' }, + { value: 'all', labelKey: 'filter.sort.top.time.all' }, +] as const + +export type CovesTimeframe = (typeof TIMEFRAME_OPTIONS)[number]['value'] + export type CovesSortParams = | { sort: 'hot'; timeframe?: undefined } | { sort: 'new'; timeframe?: undefined } @@ -16,18 +34,26 @@ const VALID_SORTS: ReadonlySet = new Set([ 'new', 'top', ]) -const VALID_TIMEFRAMES: ReadonlySet = new Set([ - 'day', - 'week', - 'month', - 'all', -]) +const VALID_TIMEFRAMES: ReadonlySet = new Set( + TIMEFRAME_OPTIONS.map((option) => option.value), +) + +/** + * Lowercases anything for validation, mapping non-strings to `''`. + * + * The normalizers below run on hand-edited imports and corrupted localStorage, + * where a leaf can be any JSON value; they must coerce rather than throw, since + * one of their callers runs during app boot. + */ +function normalizeCase(value: unknown): string { + return typeof value === 'string' ? value.toLowerCase() : '' +} -function isValidSort(s: string): s is CovesSortType { +export function isValidSort(s: string): s is CovesSortType { return (VALID_SORTS as ReadonlySet).has(s) } -function isValidTimeframe(t: string): t is CovesTimeframe { +export function isValidTimeframe(t: string): t is CovesTimeframe { return (VALID_TIMEFRAMES as ReadonlySet).has(t) } @@ -54,23 +80,68 @@ export function mapSort(sort: string, timeframe?: string): CovesSortParams { return { sort } } -// --------------------------------------------------------------------------- -// Comment sort normalization and legacy (Lemmy) mapping -// --------------------------------------------------------------------------- - /** - * Normalizes a persisted or env-provided comment sort value to a valid Coves - * sort. Handles legacy capitalized values ('Hot', 'Top', 'TopAll', 'New', ...) + * Normalizes a persisted or env-provided sort value to a valid Coves sort. + * Handles legacy capitalized values ('Hot', 'Top', 'TopAll', 'New', ...) * written by older versions of the app; anything unrecognized (e.g. 'Old', - * 'Controversial') falls back to `'hot'`. + * 'Controversial') or not a string at all falls back to `'hot'`. */ -export function normalizeCommentSort(sort: string): CovesSortType { - const lower = sort.toLowerCase() - if (isValidSort(lower)) return lower - if (lower.startsWith('top')) return 'top' +export function normalizeSort(sort: unknown): CovesSortType { + const value = normalizeCase(sort) + if (isValidSort(value)) return value + if (value.startsWith('top')) return 'top' return 'hot' } +/** + * Normalizes a persisted or env-provided timeframe to a valid Coves timeframe. + * Falls back to `'all'`. + */ +export function normalizeTimeframe(timeframe: unknown): CovesTimeframe { + const value = normalizeCase(timeframe) + return isValidTimeframe(value) ? value : 'all' +} + +/** + * Resolves the sort params for a feed load from the URL, falling back to the + * viewer's saved defaults. + * + * The saved timeframe only applies when the sort itself came from settings. A + * URL that names a sort explicitly (`?sort=top`, a shared link, the command + * palette) gets `mapSort`'s `'all'` fallback instead, so a link means the same + * thing to everyone who opens it rather than inheriting the reader's settings. + * + * A sort the API doesn't accept is graced through {@link normalizeSort} rather + * than dropped: bookmarks from the Lemmy-era UI carry `?sort=TopWeek`, and + * salvaging the intent ('top') beats silently showing them 'hot'. + */ +export function resolveFeedSort( + url: URL, + defaults: { sort: string; timeframe: string }, +): CovesSortParams { + const urlSort = url.searchParams.get('sort') + const urlTimeframe = url.searchParams.get('timeframe') ?? undefined + + const sort = urlSort ?? defaults.sort + const timeframe = + urlTimeframe ?? (urlSort === null ? defaults.timeframe : undefined) + + if (isValidSort(sort)) return mapSort(sort, timeframe) + + const salvaged = normalizeSort(sort) + console.warn(`[sort] Legacy sort value "${sort}", mapping to "${salvaged}"`) + return mapSort(salvaged, timeframe) +} + +// --------------------------------------------------------------------------- +// Comment sort normalization and legacy (Lemmy) mapping +// --------------------------------------------------------------------------- + +/** @see {@link normalizeSort} — comment sorts share the same value space. */ +export function normalizeCommentSort(sort: unknown): CovesSortType { + return normalizeSort(sort) +} + /** Comment sort values accepted by the legacy Lemmy API. */ export type LemmyCommentSortType = | 'Hot' @@ -119,6 +190,24 @@ export function mapCommunitySort(sort: string): CommunitySortType { return 'popular' } +/** + * Normalizes a persisted or env-provided listing type to a valid Coves + * listing. Legacy Lemmy 'Subscribed' becomes 'timeline', the feed of the + * communities you follow; every other legacy value ('All', 'Local', + * 'ModeratorView'), and anything that isn't a string, falls back to + * 'discover'. + * + * Auth state is deliberately not considered: this only guarantees the stored + * value is one the app can render. {@link mapListing} still downgrades + * 'timeline' to 'discover' for signed-out requests when a feed is loaded. + */ +export function normalizeListing(listing: unknown): CovesListingType { + const value = normalizeCase(listing) + return value === 'timeline' || value === 'subscribed' + ? 'timeline' + : 'discover' +} + /** * Validates and returns Coves listing type. * Falls back to `'discover'` for invalid input or unauthenticated timeline requests. diff --git a/src/lib/feature/feeds/feed.svelte.test.ts b/src/lib/feature/feeds/feed.svelte.test.ts new file mode 100644 index 00000000..41b559c9 --- /dev/null +++ b/src/lib/feature/feeds/feed.svelte.test.ts @@ -0,0 +1,146 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +// `feed.svelte.ts` reads `browser` and imports `profile` (whose real module +// touches localStorage at import time, which node has no notion of). The cache +// factory only hands back an existing instance when `browser` is true, which is +// the mode these tests care about. +vi.mock('$app/environment', () => ({ + browser: true, + dev: false, + building: false, + version: 'test', +})) + +vi.mock('$lib/app/auth.svelte', () => ({ + profile: { meta: { profile: undefined } }, +})) + +import { Feed, feed, feeds } from '$lib/feature/feeds/feed.svelte' + +interface Params { + community: string +} +interface Result { + from: string +} + +/** + * `FetchFn` declares a synchronous return while `load()` awaits it; the + * production `feed()` factory bridges that gap with the same cast, so tests + * construct `Feed` through this helper rather than restating the cast inline. + */ +function asFetchFn( + fn: (params: Params) => Promise, +): (params: Params) => Result { + return fn as unknown as (params: Params) => Result +} + +/** A promise whose settlement this test controls, to force an interleaving. */ +function deferred() { + let resolve!: (value: T) => void + let reject!: (reason: unknown) => void + const promise = new Promise((res, rej) => { + resolve = res + reject = rej + }) + return { promise, resolve, reject } +} + +const PARAMS_A: Params = { community: 'news.coves.social' } +const PARAMS_B: Params = { community: 'linux.coves.social' } + +describe('Feed.load concurrency', () => { + it('discards a superseded fetch that resolves last', async () => { + const first = deferred() + const second = deferred() + const fetcher = vi + .fn<(params: Params) => Promise>() + .mockReturnValueOnce(first.promise) + .mockReturnValueOnce(second.promise) + + const subject = new Feed(asFetchFn(fetcher)) + + // Both in flight; the second load supersedes the first. + const loadA = subject.load(PARAMS_A) + const loadB = subject.load(PARAMS_B) + + second.resolve({ from: 'B' }) + await expect(loadB).resolves.toEqual({ from: 'B' }) + + // The superseded fetch lands late. It still settles its own caller... + first.resolve({ from: 'A' }) + await expect(loadA).resolves.toEqual({ from: 'A' }) + + // ...but must not have overwritten the cache, which now describes B. + expect(subject.peek()).toEqual({ from: 'B' }) + + // The regression this guards: a later load for B (back/forward, or any + // re-run with matching params) saw a non-null `#data` holding A's result + // and served it without refetching. + await expect(subject.load(PARAMS_B)).resolves.toEqual({ from: 'B' }) + expect(fetcher).toHaveBeenCalledTimes(2) + }) + + it('does not surface an error from a superseded fetch that rejects last', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + const first = deferred() + const second = deferred() + const fetcher = vi + .fn<(params: Params) => Promise>() + .mockReturnValueOnce(first.promise) + .mockReturnValueOnce(second.promise) + + const subject = new Feed(asFetchFn(fetcher)) + + const loadA = subject.load(PARAMS_A) + const loadB = subject.load(PARAMS_B) + + second.resolve({ from: 'B' }) + await loadB + + const stale = new Error('superseded request failed') + first.reject(stale) + // The superseded caller is still rejected — the failure is not swallowed. + await expect(loadA).rejects.toBe(stale) + + // But it must not raise a banner over the page B rendered successfully. + expect(subject.error).toBeUndefined() + expect(subject.peek()).toEqual({ from: 'B' }) + + consoleError.mockRestore() + }) + + it('still records an error when the newest fetch is the one that fails', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + const boom = new Error('current request failed') + const subject = new Feed( + asFetchFn(() => Promise.reject(boom)), + ) + + await expect(subject.load(PARAMS_A)).rejects.toBe(boom) + expect(subject.error).toBe(boom) + + consoleError.mockRestore() + }) +}) + +describe('feed() cache', () => { + beforeEach(() => { + feeds.clear() + }) + + it('reuses the cached instance for a route ID and adopts the newest fetcher', async () => { + const firstFetcher = vi.fn(async () => ({ communities: [] })) + const secondFetcher = vi.fn(async () => ({ communities: [] })) + + const a = feed('/explore/communities', firstFetcher) + const b = feed('/explore/communities', secondFetcher) + expect(b).toBe(a) + + await b.load({ sort: 'new' }) + + // The first navigation's closure must not be the one that runs. + expect(firstFetcher).not.toHaveBeenCalled() + expect(secondFetcher).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/lib/feature/feeds/feed.svelte.ts b/src/lib/feature/feeds/feed.svelte.ts index 22ba1f90..e96bf390 100644 --- a/src/lib/feature/feeds/feed.svelte.ts +++ b/src/lib/feature/feeds/feed.svelte.ts @@ -23,12 +23,31 @@ export class Feed { #data = $state() #fetch: FetchFn #lastParams?: Params + /** + * Monotonic load counter. Loads overlap routinely on a cached instance — + * rapid client-side navigations, and sort changes that `goto` + + * `invalidateAll` — and their fetches can settle out of order. Only the + * newest load may commit to `#data`/`error`; a superseded fetch still + * settles its own caller's promise, but must never write state that by then + * describes a later load. + */ + #generation = 0 error = $state() constructor(fetch: FetchFn) { this.#fetch = fetch } + /** + * Replaces the fetcher on a cached instance. Each load() run builds a fresh + * init closure (over that run's `fetch`, and potentially that run's route + * params); keeping the newest one means a cached feed can never refetch + * using a previous navigation's captured values. + */ + setFetch(fetch: FetchFn): void { + this.#fetch = fetch + } + async load(params: Params) { if (!recursiveEqual(params, this.#lastParams)) { this.#data = undefined @@ -36,13 +55,22 @@ export class Feed { } this.#lastParams = params + const generation = ++this.#generation + if (this.#data == null) { try { - this.#data = await this.#fetch(params) + const result = await this.#fetch(params) + // A newer load started while this fetch was in flight: hand the result + // back to this caller (it is coherent with the params *it* asked for) + // but leave the cache to the newer load, which now owns `#lastParams`. + if (generation !== this.#generation) return result + this.#data = result this.error = undefined } catch (err) { console.error('[Feed] fetch failed:', err) - this.error = err + // Superseded failures still reject their own caller, but must not + // raise an error banner over whatever the newer load rendered. + if (generation === this.#generation) this.error = err throw err } } @@ -164,7 +192,11 @@ export function feed( const existing = feeds.get(id) // The map erases per-route type info; the cast is safe because each route ID // is only ever written with its matching Feed. - if (browser && existing) return existing as Feed + if (browser && existing) { + const cached = existing as Feed + cached.setFetch(init as unknown as FetchFn) + return cached + } const feedData = new Feed(init as unknown as FetchFn) feeds.set(id, feedData as Feed) diff --git a/src/lib/feature/filter/FeedTabs.svelte b/src/lib/feature/filter/FeedTabs.svelte index 416ea3ba..89a9b1d1 100644 --- a/src/lib/feature/filter/FeedTabs.svelte +++ b/src/lib/feature/filter/FeedTabs.svelte @@ -2,6 +2,7 @@ import { page } from '$app/state' import { profile } from '$lib/app/auth.svelte' import { t } from '$lib/app/i18n' + import { settings } from '$lib/app/settings.svelte' import type { CovesListingType } from '$lib/app/sort' import { searchParam } from '$lib/app/util.svelte' @@ -16,6 +17,9 @@ function select(value: CovesListingType): void { selected = value + // Saved on selection only: merely opening a `?type=` link shouldn't + // rewrite which feed the viewer lands on next time. + settings.defaultSort.feed = value searchParam(page.url, 'type', value, 'page', 'cursor') } diff --git a/src/lib/feature/filter/Location.svelte b/src/lib/feature/filter/Location.svelte deleted file mode 100644 index b8f34c7b..00000000 --- a/src/lib/feature/filter/Location.svelte +++ /dev/null @@ -1,68 +0,0 @@ - - - diff --git a/src/lib/feature/filter/Sort.svelte b/src/lib/feature/filter/Sort.svelte deleted file mode 100644 index 5bece529..00000000 --- a/src/lib/feature/filter/Sort.svelte +++ /dev/null @@ -1,135 +0,0 @@ - - -
- - {#if selected?.startsWith('Top')} - - {/if} -
diff --git a/src/lib/feature/filter/SortMenu.svelte b/src/lib/feature/filter/SortMenu.svelte index 17c478fd..5fcd1aed 100644 --- a/src/lib/feature/filter/SortMenu.svelte +++ b/src/lib/feature/filter/SortMenu.svelte @@ -2,10 +2,16 @@ import { goto } from '$app/navigation' import { page } from '$app/state' import { t } from '$lib/app/i18n' - import type { CovesSortType, CovesTimeframe } from '$lib/app/sort' + import { settings } from '$lib/app/settings.svelte' + import { + normalizeTimeframe, + TIMEFRAME_OPTIONS, + type CovesSortType, + type CovesTimeframe, + } from '$lib/app/sort' import Menu from '$lib/ui/shared/popover/Menu.svelte' import MenuButton from '$lib/ui/shared/popover/MenuButton.svelte' - import { Button } from 'mono-svelte' + import { Button, toast } from 'mono-svelte' import { Check, ChevronDown, @@ -40,24 +46,23 @@ new: { icon: Star, labelKey: 'filter.sort.new' }, } - const timeframeConfig: { - value: CovesTimeframe - labelKey: string - }[] = [ - { value: 'day', labelKey: 'filter.sort.top.time.day' }, - { value: 'week', labelKey: 'filter.sort.top.time.week' }, - { value: 'month', labelKey: 'filter.sort.top.time.month' }, - { value: 'all', labelKey: 'filter.sort.top.time.all' }, - ] - let currentIcon = $derived(sortConfig[sort]?.icon ?? sortConfig.hot.icon) let currentLabel = $derived( $t(sortConfig[sort]?.labelKey ?? sortConfig.hot.labelKey), ) - async function selectSort( + /** + * Navigates to the chosen sort, then saves it as the viewer's default. + * + * @param timeframeChosen whether the viewer picked this period, as opposed + * to it being filled in so `sort=top` has one. Only a real choice is saved: + * otherwise every click on Top would overwrite the saved period with + * whatever happened to be on screen. + */ + async function applySort( newSort: CovesSortType, - newTimeframe?: CovesTimeframe, + newTimeframe: CovesTimeframe | undefined, + timeframeChosen: boolean, ): Promise { const url = new URL(page.url) url.searchParams.set('sort', newSort) @@ -80,13 +85,36 @@ await goto(url, { invalidateAll: true }) } catch (err) { console.error('[SortMenu] Navigation failed:', err) + toast({ content: t.get('toast.sortFailed'), type: 'error' }) sort = prevSort timeframe = prevTimeframe + return + } + + // Saved only once the navigation lands: feed load()s read these when a URL + // carries no sort of its own. + settings.defaultSort.sort = newSort + if (newSort === 'top' && timeframeChosen && newTimeframe) { + settings.defaultSort.timeframe = newTimeframe + } + } + + function selectSort(newSort: CovesSortType): void { + if (newSort !== 'top') { + void applySort(newSort, undefined, false) + return } + // `sort=top` needs a period in the URL; reuse the one on screen, else the + // viewer's saved default rather than a blanket 'all'. + void applySort( + 'top', + timeframe ?? normalizeTimeframe(settings.defaultSort.timeframe), + false, + ) } function selectTimeframe(newTimeframe: CovesTimeframe): void { - selectSort('top', newTimeframe) + void applySort('top', newTimeframe, true) } @@ -118,10 +146,7 @@ {/snippet} - selectSort('top', timeframe ?? 'all')} - > + selectSort('top')}> {$t('filter.sort.top.label')} {#snippet suffix()} {#if sort === 'top'} @@ -136,7 +161,7 @@ {#if sort === 'top'} - {#each timeframeConfig as tf (tf.value)} + {#each TIMEFRAME_OPTIONS as tf (tf.value)} { - if (filters.sort) settings.defaultSort.sort = filters.sort - }) + function resolveSort(sort?: string, timeframe?: string): CovesSortParams { + return sort ? mapSort(sort, timeframe) : { sort: 'hot' } + } + + const routeSort = $derived(resolveSort(params.sort, params.timeframe)) + + const initialSort = resolveSort(params.sort, params.timeframe) + let filters = $state<{ + sort: CovesSortType + timeframe: CovesTimeframe | undefined + }>({ sort: initialSort.sort, timeframe: initialSort.timeframe }) - let filters = $state({ - sort: params.sort, + // SortMenu navigates, which re-runs load() and hands down new params; the + // route stays the source of truth so back/forward navigation stays in sync. + $effect(() => { + filters.sort = routeSort.sort + filters.timeframe = routeSort.timeframe }) const FeedComponent = $derived( @@ -61,29 +82,54 @@ {/if} {#snippet extended()} {@render passedExtended?.()} -
-
- {#if filters.sort} - - {/if} - +
+ + - -
- + + +
{/snippet} {/if} diff --git a/src/lib/ui/navbar/commands/actions.svelte.ts b/src/lib/ui/navbar/commands/actions.svelte.ts index 7150fa88..df8383b1 100644 --- a/src/lib/ui/navbar/commands/actions.svelte.ts +++ b/src/lib/ui/navbar/commands/actions.svelte.ts @@ -6,15 +6,12 @@ import { } from '$lib/app/auth.svelte' import { t } from '$lib/app/i18n' import { settings } from '$lib/app/settings.svelte' +import { TIMEFRAME_OPTIONS } from '$lib/app/sort' import { theme, type ThemeData } from '$lib/app/theme/theme.svelte' import type { ResumableItem } from '$lib/feature/legacy/item.svelte' import { ArrowRightOnRectangle, - ArrowTrendingDown, - ArrowTrendingUp, ChartBar, - ChatBubbleLeftRight, - ChatBubbleOvalLeftEllipsis, Clock, Cog6Tooth, ComputerDesktop, @@ -22,13 +19,12 @@ import { GlobeAlt, GlobeAmericas, Home, - MapPin, Moon, Newspaper, PaintBrush, PencilSquare, Plus, - Scale, + Sparkles, Star, Sun, Swatch, @@ -90,23 +86,18 @@ export function getGroups( name: t.get('nav.commands.feeds'), actions: [ { - name: t.get('filter.location.label'), + name: t.get('filter.feed.label'), icon: GlobeAmericas, subActions: [ { - name: t.get('filter.location.all'), + name: t.get('filter.feed.discover'), icon: GlobeAmericas, - href: '/?type=All', + href: '/?type=discover', }, { - name: t.get('filter.location.local'), - icon: MapPin, - href: '/?type=Local', - }, - { - name: t.get('filter.location.subscribed'), - icon: Newspaper, - href: '/?type=Subscribed', + name: t.get('filter.feed.forYou'), + icon: Sparkles, + href: '/?type=timeline', }, ], }, @@ -114,96 +105,24 @@ export function getGroups( name: t.get('filter.sort.label'), icon: ChartBar, subActions: [ - { - name: t.get('filter.sort.top.label'), - icon: Trophy, - subActions: [ - { - name: t.get('filter.sort.top.time.all'), - icon: ChartBar, - href: '/?sort=TopAll', - }, - { - name: t.get('filter.sort.top.time.9months'), - icon: ChartBar, - href: '/?sort=TopNineMonths', - }, - { - name: t.get('filter.sort.top.time.6months'), - icon: ChartBar, - href: '/?sort=TopSixMonths', - }, - { - name: t.get('filter.sort.top.time.3months'), - icon: ChartBar, - href: '/?sort=TopThreeMonths', - }, - { - name: t.get('filter.sort.top.time.month'), - icon: ChartBar, - href: '/?sort=TopMonth', - }, - { - name: t.get('filter.sort.top.time.week'), - icon: ChartBar, - href: '/?sort=TopWeek', - }, - { - name: t.get('filter.sort.top.time.day'), - icon: ChartBar, - href: '/?sort=TopDay', - }, - { - name: t.get('filter.sort.top.time.6hours'), - icon: ChartBar, - href: '/?sort=TopSixHour', - }, - { - name: t.get('filter.sort.top.time.hour'), - icon: ChartBar, - href: '/?sort=TopHour', - }, - ], - }, - { - name: t.get('filter.sort.active'), - icon: ArrowTrendingUp, - href: '/?sort=Active', - }, { name: t.get('filter.sort.hot'), icon: Fire, - href: '/?sort=Hot', + href: '/?sort=hot', }, { - name: t.get('filter.sort.scaled'), - icon: Scale, - href: '/?sort=Scaled', + name: t.get('filter.sort.top.label'), + icon: Trophy, + subActions: TIMEFRAME_OPTIONS.map((timeframe) => ({ + name: t.get(timeframe.labelKey), + icon: Clock, + href: `/?sort=top&timeframe=${timeframe.value}`, + })), }, { name: t.get('filter.sort.new'), icon: Star, - href: '/?sort=New', - }, - { - name: t.get('filter.sort.old'), - icon: Clock, - href: '/?sort=Old', - }, - { - name: t.get('filter.sort.controversial'), - icon: ArrowTrendingDown, - href: '/?sort=Controversial', - }, - { - name: t.get('filter.sort.mostcomments'), - icon: ChatBubbleOvalLeftEllipsis, - href: '/?sort=MostComments', - }, - { - name: t.get('filter.sort.newcomments'), - icon: ChatBubbleLeftRight, - href: '/?sort=NewComments', + href: '/?sort=new', }, ], }, diff --git a/src/routes/+page.svelte b/src/routes/+page.svelte index dd74801b..85b996e3 100644 --- a/src/routes/+page.svelte +++ b/src/routes/+page.svelte @@ -17,12 +17,9 @@ let { data = $bindable() } = $props() - $effect(() => { - if (data.filters.value.sort) - settings.defaultSort.sort = data.filters.value.sort - if (data.filters.value.type_) - settings.defaultSort.feed = data.filters.value.type_ - }) + // Defaults are saved by the controls themselves (SortMenu, FeedTabs) on a + // real selection. Persisting from here instead would rewrite them for anyone + // who merely *opened* a link that named a sort or feed. const FeedComponent = $derived( settings.infiniteScroll && browser && !settings.posts.noVirtualize diff --git a/src/routes/+page.ts b/src/routes/+page.ts index 40fa75e2..c440fc16 100644 --- a/src/routes/+page.ts +++ b/src/routes/+page.ts @@ -3,7 +3,7 @@ import { coves } from '$lib/api/client.svelte' import { profile } from '$lib/app/auth.svelte' import { t } from '$lib/app/i18n' import { settings } from '$lib/app/settings.svelte' -import { mapListing, mapSort } from '$lib/app/sort' +import { mapListing, resolveFeedSort } from '$lib/app/sort' import { ReactiveState, awaitIfServer } from '$lib/app/util.svelte' import { feed } from '$lib/feature/feeds/feed.svelte' import { ChevronDoubleUp } from '@xylightdev/svelte-hero-icons' @@ -11,11 +11,9 @@ import { ChevronDoubleUp } from '@xylightdev/svelte-hero-icons' export async function load({ url, fetch, route }) { const cursor = url.searchParams.get('cursor') as string | undefined - const sort = url.searchParams.get('sort') ?? settings.defaultSort.sort - const timeframe = url.searchParams.get('timeframe') ?? undefined const listingType = url.searchParams.get('type') ?? settings.defaultSort.feed - const mapped = mapSort(sort, timeframe) + const mapped = resolveFeedSort(url, settings.defaultSort) const listing = mapListing(listingType, profile.isAuthenticated) const feedData = feed(route.id, async (params) => { diff --git a/src/routes/c/[handle=handle]/+page.svelte b/src/routes/c/[handle=handle]/+page.svelte index 2ef56a03..62eb683d 100644 --- a/src/routes/c/[handle=handle]/+page.svelte +++ b/src/routes/c/[handle=handle]/+page.svelte @@ -52,6 +52,7 @@ getParams={data.params} params={{ sort: data.params.sort, + timeframe: data.params.timeframe, }} loadFeed={data.loadFeed} > diff --git a/src/routes/c/[handle=handle]/+page.ts b/src/routes/c/[handle=handle]/+page.ts index 3c75fc56..6a937598 100644 --- a/src/routes/c/[handle=handle]/+page.ts +++ b/src/routes/c/[handle=handle]/+page.ts @@ -3,7 +3,7 @@ import { coves } from '$lib/api/client.svelte' import { XrpcError } from '$lib/api/coves/xrpc' import { settings } from '$lib/app/settings.svelte' import { error } from '@sveltejs/kit' -import { mapSort } from '$lib/app/sort' +import { resolveFeedSort } from '$lib/app/sort' import type { Handle } from '$lib/types/atproto' import CommunityCard from '$lib/feature/community/CommunityCard.svelte' import { feed } from '$lib/feature/feeds/feed.svelte' @@ -11,28 +11,32 @@ import { feed } from '$lib/feature/feeds/feed.svelte' export async function load({ params, fetch, url, route }) { const cursor = url.searchParams.get('cursor') as string | undefined - const sort = url.searchParams.get('sort') ?? settings.defaultSort.sort - const timeframe = url.searchParams.get('timeframe') ?? undefined - // Sent verbatim: the AppView resolves a DID, a bare handle, or a "c-" // prefixed handle, so the slug needs no rewriting here. const communityHandle = params.handle as Handle - const mapped = mapSort(sort, timeframe) + const mapped = resolveFeedSort(url, settings.defaultSort) let feedData try { feedData = await feed(route.id, async (p) => { const api = coves({ func: fetch }) + // Every community shares the route ID `/c/[handle=handle]`, so the cache + // can hand this closure back on a later navigation to a *different* + // community or sort. Read the identity of the request from `p` only — + // anything captured from the enclosing load() run would be stale. + // The route's `handle` matcher guarantees the slug is handle-shaped. + const community = p.community as Handle + const [feedResponse, communityData] = await Promise.all([ api.getCommunityFeed({ - community: communityHandle, - sort: mapped.sort, - timeframe: mapped.timeframe, + community, + sort: p.sort, + timeframe: p.timeframe, limit: p.limit, cursor: p.cursor, }), - api.getCommunity({ community: communityHandle }), + api.getCommunity({ community }), ]) return { @@ -42,7 +46,7 @@ export async function load({ params, fetch, url, route }) { params: { ...p, cursor: feedResponse.cursor }, } }).load({ - community: params.handle, + community: communityHandle, sort: mapped.sort, timeframe: mapped.timeframe, limit: 20, diff --git a/src/routes/c/[handle=handle]/page.test.ts b/src/routes/c/[handle=handle]/page.test.ts new file mode 100644 index 00000000..42354160 --- /dev/null +++ b/src/routes/c/[handle=handle]/page.test.ts @@ -0,0 +1,184 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +// --------------------------------------------------------------------------- +// Mocks +// +// Unlike the post-page loader test, this one runs against the *real* feed cache +// (`$lib/feature/feeds/feed.svelte`) with `browser` forced to `true`. That is +// the whole point: every community shares the route ID `/c/[handle=handle]`, so +// the cache hands the same `Feed` instance back on the next navigation, and the +// regression under test is the loader's init closure reading community/sort +// from the *first* load run instead of from the params it is handed. +// --------------------------------------------------------------------------- + +vi.mock('$app/environment', () => ({ + browser: true, + dev: false, + building: false, + version: 'test', +})) + +const mockCovesMethods = vi.hoisted(() => ({ + getCommunityFeed: vi.fn(), + getCommunity: vi.fn(), +})) + +// Every `func` the loader builds a client with, in call order. The loader calls +// `coves({ func: fetch })` inside its init closure, so this records which +// navigation's `fetch` each refetch actually ran through. +const clientFetchArgs = vi.hoisted(() => [] as unknown[]) + +vi.mock('$lib/api/client.svelte', () => ({ + coves: ({ func }: { func: unknown }) => { + clientFetchArgs.push(func) + return mockCovesMethods + }, +})) + +// `feed.svelte.ts` imports `profile` purely for its cache-clearing effect; the +// real module reads localStorage at import time, which node has no notion of. +vi.mock('$lib/app/auth.svelte', () => ({ + profile: { meta: { profile: undefined } }, +})) + +// Mutable so a test can stand in a different saved default; reset per test. +const mockSettings = vi.hoisted(() => ({ + defaultSort: { sort: 'hot', timeframe: 'all' }, +})) + +vi.mock('$lib/app/settings.svelte', () => ({ settings: mockSettings })) + +// `$lib/app/sort` is deliberately NOT mocked: `resolveFeedSort` is pure and +// dependency-free, so running the real one exercises the URL-vs-saved-defaults +// precedence end to end rather than re-implementing it here. + +import { XrpcError } from '$lib/api/coves/xrpc' +import { feeds } from '$lib/feature/feeds/feed.svelte' +import { load } from './+page' + +// The loader only destructures { params, fetch, url, route }; supplying those +// four is sufficient at runtime. Cast to the full LoadEvent for the type, the +// same way the repo's other load tests do. +function makeArgs( + handle: string, + query = '', + fetchFn: typeof globalThis.fetch = globalThis.fetch, +): Parameters[0] { + return { + params: { handle }, + url: new URL(`https://coves.test/c/${handle}${query}`), + fetch: fetchFn, + route: { id: '/c/[handle=handle]' }, + } as unknown as Parameters[0] +} + +/** A distinguishable stand-in for a single navigation's `fetch`. */ +function sentinelFetch(): typeof globalThis.fetch { + return vi.fn() as unknown as typeof globalThis.fetch +} + +function feedResponse(community: string) { + return { + feed: [{ post: { rkey: `post-in-${community}` } }], + cursor: `cursor-${community}`, + } +} + +describe('community loader', () => { + beforeEach(() => { + mockCovesMethods.getCommunityFeed.mockReset() + mockCovesMethods.getCommunity.mockReset() + clientFetchArgs.length = 0 + mockSettings.defaultSort = { sort: 'hot', timeframe: 'all' } + // The cache is module-level state shared across tests. + feeds.clear() + + mockCovesMethods.getCommunityFeed.mockImplementation( + ({ community }: { community: string }) => + Promise.resolve(feedResponse(community)), + ) + mockCovesMethods.getCommunity.mockImplementation( + ({ community }: { community: string }) => + Promise.resolve({ did: `did:plc:${community}`, handle: community }), + ) + }) + + it('fetches the requested community and returns its feed', async () => { + const result = await load(makeArgs('news.coves.social')) + + expect(mockCovesMethods.getCommunityFeed).toHaveBeenCalledWith( + expect.objectContaining({ community: 'news.coves.social', sort: 'hot' }), + ) + expect(result.community).toMatchObject({ handle: 'news.coves.social' }) + expect(result.feed).toEqual(feedResponse('news.coves.social').feed) + }) + + it('refetches the new community when navigating between communities on the cached feed', async () => { + await load(makeArgs('news.coves.social')) + const result = await load(makeArgs('linux.coves.social')) + + // Regression: the cached Feed's init closure used to have captured the + // first navigation's handle, so this refetched `news.coves.social`. + expect(mockCovesMethods.getCommunityFeed).toHaveBeenLastCalledWith( + expect.objectContaining({ community: 'linux.coves.social' }), + ) + expect(mockCovesMethods.getCommunity).toHaveBeenLastCalledWith({ + community: 'linux.coves.social', + }) + expect(result.community).toMatchObject({ handle: 'linux.coves.social' }) + expect(result.feed).toEqual(feedResponse('linux.coves.social').feed) + }) + + it('refetches with the new sort when the sort changes on the same community', async () => { + await load(makeArgs('news.coves.social')) + await load(makeArgs('news.coves.social', '?sort=top&timeframe=week')) + + expect(mockCovesMethods.getCommunityFeed).toHaveBeenLastCalledWith( + expect.objectContaining({ + community: 'news.coves.social', + sort: 'top', + timeframe: 'week', + }), + ) + }) + + it("applies the viewer's saved timeframe when the URL names no sort", async () => { + mockSettings.defaultSort = { sort: 'top', timeframe: 'week' } + + await load(makeArgs('news.coves.social')) + + // The saved timeframe only rides along when the sort itself came from + // settings, so a bare URL must reach the API as top/week. + expect(mockCovesMethods.getCommunityFeed).toHaveBeenCalledWith( + expect.objectContaining({ sort: 'top', timeframe: 'week' }), + ) + }) + + it('refetches through the newest navigation fetch, not the first one', async () => { + const firstFetch = sentinelFetch() + const secondFetch = sentinelFetch() + + await load(makeArgs('news.coves.social', '', firstFetch)) + await load(makeArgs('linux.coves.social', '', secondFetch)) + + // The cached Feed reruns an init closure, and that closure captures its + // navigation's `fetch`. Without `setFetch` the stale first closure runs, + // so the refetch would go out through a fetch belonging to a navigation + // SvelteKit has already torn down. + expect(clientFetchArgs.at(-1)).toBe(secondFetch) + expect(clientFetchArgs).toContain(firstFetch) + }) + + it('surfaces an XRPC 404 as a routable 404', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + mockCovesMethods.getCommunityFeed.mockRejectedValue( + new XrpcError(404, 'NotFound', 'Community not found'), + ) + + await expect(load(makeArgs('ghost.coves.social'))).rejects.toMatchObject({ + status: 404, + }) + + consoleError.mockRestore() + }) +}) diff --git a/src/routes/settings/+layout.svelte b/src/routes/settings/+layout.svelte index 5a32f973..1790b438 100644 --- a/src/routes/settings/+layout.svelte +++ b/src/routes/settings/+layout.svelte @@ -1,6 +1,10 @@