From b514c98148bd5153474302c4d120f3667fff6ff6 Mon Sep 17 00:00:00 2001 From: Bretton Date: Tue, 11 Aug 2026 23:29:39 -0700 Subject: [PATCH] =?UTF-8?q?fix(app):=20TDD=20bug=20sweep=20=E2=80=94=20vot?= =?UTF-8?q?e=20correctness,=20feed=20races,=20stale-state=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test-first sweep of correctness bugs across voting, feeds, markdown rendering, and profile loading, hardened by a multi-model pre-merge review (/second-opinion) whose blocking findings are folded in. Voting: - Extract pure vote arithmetic into vote.ts with literal-oracle tests; fixes score computed as `upvotes` (downvotes ignored) and the stale voteUri carried across a direction switch; counters clamp at zero on desync with a warning instead of rendering negative counts. - CreateVoteOutput.uri/cid become optional, encoding the backend's create-as-toggle 200-without-uri case; both vote components resync from the pre-press snapshot and toast voteOutOfSync when it fires. - deleteVote 404 keeps the optimistic un-vote only for the backend's VoteNotFound error name; infrastructure 404s roll back and surface. - Vote components capture their subject before awaiting and discard settlements that land after the component shows a different subject. Feeds: - VirtualFeed: per-feed state (seenUris/hasMore/error) reseeds on feed identity change, and in-flight loadMore settlements for a departed feed are discarded instead of splicing the old feed's posts and cursor into the new one. - VirtualList: the height cache truncates when items shrink (was a documented-only defect); an emptied list clears visibleItems. Profile: - Loader scopes "user not found" to a getProfile 404 (bare i18n key); all other failures — including 404s from the posts/comments calls — propagate untouched. New loader tests cover every routing path plus the SvelteKit fetch pass-through. - UserActions: block state re-syncs on a DID+URI latch across navigation; toggleBlock guards re-entrancy, captures its subject across the await, and reports errors via errorMessage() with a 401 sessionExpired toast. App shell & UI: - sessionExpired flash cookie replaced by a locals-driven page.data flag with a repeat-toast latch. - Markdown options context uses live getters so inline/noStyle toggles reach mounted renderers; Popover passes positioning config through the floating-ui action argument so prop updates actually apply. - Init-time untrack() seeds and effect latches documented accurately (state_referenced_locally intent, not fictitious SSR/dependency mechanics); tsconfig drops the removed vite-plugin-pwa types. Co-Authored-By: Claude Fable 5 --- src/lib/api/coves/types.ts | 60 ++++- src/lib/app/i18n/en.json | 1 + src/lib/app/markdown/Markdown.svelte | 37 ++- src/lib/app/markdown/MdTree.svelte | 10 +- src/lib/app/render/VirtualList.svelte | 55 +++-- .../feature/comment/CommentProvider.svelte | 8 +- src/lib/feature/comment/CommentVote.svelte | 124 ++++++++-- src/lib/feature/feeds/feed.svelte.ts | 2 +- src/lib/feature/post/PostVote.svelte | 124 ++++++++-- src/lib/feature/post/feed/VirtualFeed.svelte | 79 ++++++- src/lib/feature/post/form/PostForm.svelte | 6 +- src/lib/feature/post/helpers.test.ts | 66 ------ src/lib/feature/post/helpers.ts | 55 +---- src/lib/feature/post/vote.test.ts | 134 +++++++++++ src/lib/feature/post/vote.ts | 109 +++++++++ src/lib/ui/icon/AnimatedHeart.svelte | 8 +- src/lib/ui/info/ProgressBar.svelte | 8 +- src/lib/ui/layout/pages/PostListShell.svelte | 10 +- .../ui/navbar/commands/CommandsHost.svelte | 2 +- src/lib/ui/shared/popover/Popover.svelte | 11 +- src/routes/+layout.svelte | 53 ++--- src/routes/explore/+layout.svelte | 40 +++- src/routes/profile/[handle=handle]/+page.ts | 50 ++-- .../[handle=handle]/UserActions.svelte | 81 ++++++- .../profile/[handle=handle]/page.test.ts | 222 ++++++++++++++++++ tsconfig.json | 2 +- 26 files changed, 1077 insertions(+), 280 deletions(-) create mode 100644 src/lib/feature/post/vote.test.ts create mode 100644 src/lib/feature/post/vote.ts create mode 100644 src/routes/profile/[handle=handle]/page.test.ts diff --git a/src/lib/api/coves/types.ts b/src/lib/api/coves/types.ts index 6559d656..e000dd50 100644 --- a/src/lib/api/coves/types.ts +++ b/src/lib/api/coves/types.ts @@ -108,10 +108,27 @@ export interface CommunityRef { avatar?: string } -export interface PostStats { +/** + * The vote counters shared by `PostStats` and `CommentStats`. + * + * Declared here rather than alongside the vote arithmetic in + * `$lib/feature/post/vote.ts` so that this module does not have to import from + * a feature module to express `extends` — `vote.ts` already imports `AtUri` + * from here, and the reverse edge would close a cycle. `vote.ts` re-exports it + * so vote logic can keep importing it from the module that operates on it. + * + * The `extends` below is the point: it makes the subset relation a compile + * error to break. `stats = { ...base, ...counts }` in the vote components has + * no other guard, because TypeScript does not excess-property-check properties + * arriving via spread. + */ +export interface VoteCounts { upvotes: number downvotes: number score: number +} + +export interface PostStats extends VoteCounts { commentCount: number shareCount?: number tagCounts?: Record @@ -119,8 +136,14 @@ export interface PostStats { // TODO: Refactor to a discriminated union to enforce vote/voteUri correlation: // { vote: 'up' | 'down'; voteUri: AtUri } | { vote?: undefined; voteUri?: undefined } -// Blocked by PostVote.svelte castVote() which independently mutates vote and voteUri -// on a spread copy, which is incompatible with discriminated union assignment rules. +// Still blocked, though no longer by mutation: castVote() now assigns a whole +// new viewer object, but it builds that object as one spread literal whose +// `vote` ('up' | undefined) and `voteUri` (AtUri | undefined) are typed +// independently, so neither union arm accepts it. +// Note the union as sketched is also wrong for this domain — between the +// optimistic write and the server response the viewer legitimately holds +// vote: 'up' with no voteUri yet, which the sketch declares impossible. +// A faithful version needs a third "pending" arm. export interface PostViewerState { saved: boolean vote?: 'up' | 'down' @@ -263,17 +286,20 @@ export interface CommentRef { cid: CID } -export interface CommentStats { - upvotes: number - downvotes: number - score: number +export interface CommentStats extends VoteCounts { replyCount: number } // TODO: Refactor to a discriminated union to enforce vote/voteUri correlation: // { vote: 'up' | 'down'; voteUri: AtUri } | { vote?: undefined; voteUri?: undefined } -// Blocked by CommentVote.svelte castVote() which independently mutates vote and voteUri -// on a spread copy, which is incompatible with discriminated union assignment rules. +// Still blocked, though no longer by mutation: castVote() now assigns a whole +// new viewer object, but it builds that object as one spread literal whose +// `vote` ('up' | undefined) and `voteUri` (AtUri | undefined) are typed +// independently, so neither union arm accepts it. +// Note the union as sketched is also wrong for this domain — between the +// optimistic write and the server response the viewer legitimately holds +// vote: 'up' with no voteUri yet, which the sketch declares impossible. +// A faithful version needs a third "pending" arm. export interface CommentViewerState { vote?: 'up' | 'down' voteUri?: AtUri @@ -558,9 +584,21 @@ export interface CreateVoteInput { direction: 'up' | 'down' } +/** + * `uri` and `cid` are ABSENT when the create toggled an existing same-direction + * vote back off. The backend's create endpoint is itself a toggle: given a vote + * that already matches the requested direction it deletes that vote and answers + * 200 with the fields omitted (backend `internal/core/votes/service_impl.go`, + * and `internal/api/handlers/vote/create_vote.go`, where both are tagged + * `omitempty` with that case called out). + * + * Optional here so the compiler forces callers to handle it: an absent `uri` + * means the viewer's vote is now GONE server-side, which is the opposite of + * what a caller assuming success would render. + */ export interface CreateVoteOutput { - uri: AtUri - cid: CID + uri?: AtUri + cid?: CID } export interface DeleteVoteInput { diff --git a/src/lib/app/i18n/en.json b/src/lib/app/i18n/en.json index ac83b346..1f923b94 100644 --- a/src/lib/app/i18n/en.json +++ b/src/lib/app/i18n/en.json @@ -765,6 +765,7 @@ "noComments": "The API returned no comments.", "commentNotFound": "That comment could not be found.", "loginVoteGate": "You must be logged in to vote.", + "voteOutOfSync": "Your vote was already recorded elsewhere and has been withdrawn. Please try again.", "blockedCommunity": "Blocked that community.", "unblockedCommunity": "Unblocked that community.", "purgedCommunity": "Purged that community.", diff --git a/src/lib/app/markdown/Markdown.svelte b/src/lib/app/markdown/Markdown.svelte index 7ff57fb9..e0606e07 100644 --- a/src/lib/app/markdown/Markdown.svelte +++ b/src/lib/app/markdown/Markdown.svelte @@ -158,6 +158,17 @@ autoloadImages: boolean } + /** + * The value published on the 'options' context, read by MdParagraph, + * MdHeading and MdImage. Declaring it explicitly means a field added to + * RendererOptions is a compile error here rather than a silently missing + * option downstream. + */ + interface MarkdownContext extends RendererOptions { + inline: boolean + noStyle: boolean + } + interface Props { source?: string inline?: boolean @@ -180,11 +191,27 @@ }, }: Props = $props() - setContext('options', { - ...rendererOptions, - inline: inline, - noStyle: noStyle, - }) + // Context is set once at init, so spreading the prop values here would pin + // them to whatever they were when this instance mounted — a `noStyle` or + // `inline` toggle on a mounted Markdown would never reach the renderers. + // Getters keep the consumers (all of which read plain properties, none of + // which spread or serialize the object) on the live values. Caveat: MdImage + // copies `autoloadImages` into local $state once at init, so already-mounted + // images still don't follow a toggle — the live context benefits MdParagraph + // and MdHeading today. + const options: MarkdownContext = { + get autoloadImages() { + return rendererOptions.autoloadImages + }, + get inline() { + return inline + }, + get noStyle() { + return noStyle + }, + } + + setContext('options', options) let tokens = $derived(marked.lexer(preprocess(source))) diff --git a/src/lib/app/markdown/MdTree.svelte b/src/lib/app/markdown/MdTree.svelte index 2ef50176..4f4443b9 100644 --- a/src/lib/app/markdown/MdTree.svelte +++ b/src/lib/app/markdown/MdTree.svelte @@ -1,6 +1,6 @@ diff --git a/src/lib/feature/post/feed/VirtualFeed.svelte b/src/lib/feature/post/feed/VirtualFeed.svelte index 37c723f8..a8bf0806 100644 --- a/src/lib/feature/post/feed/VirtualFeed.svelte +++ b/src/lib/feature/post/feed/VirtualFeed.svelte @@ -19,7 +19,6 @@ } from 'svelte-hero-icons/dist' import InfiniteScroll from 'svelte-infinite-scroll' import { expoOut } from 'svelte/easing' - import { SvelteSet } from 'svelte/reactivity' import { fly } from 'svelte/transition' import { Post } from '..' @@ -56,11 +55,58 @@ (error.status === 401 || error.status === 403), ) let loading = $state(false) - let hasMore = $state(!!loadFeed) - let seenUris = new SvelteSet( - (posts ?? []).map((fp) => fp.post.uri as string), - ) + // A plain Set: `seenUris` is only ever read and written inside loadMore(), + // never from the template, so it carries no reactivity. + function seedSeenUris(feed: FeedViewPost[] | undefined): Set { + return new Set((feed ?? []).map((fp) => fp.post.uri as string)) + } + + // Neither of these can become a $derived: `hasMore` latches false once the + // API runs out of pages, and `seenUris` accumulates every URI loadMore() has + // appended. But this component is reused across client-side navigation, so + // carrying either into a different feed is wrong — the new feed would start + // with the old one's end-of-feed latch and silently drop any post whose URI + // the old feed had already shown. + // + // A feed switch is exactly a new `posts` array identity: loadMore() appends + // with posts.push(), which mutates in place and leaves identity untouched + // (the same property the {#key posts} block below relies on to avoid + // rebuilding the virtual list after every page). + // + // The identity comparison is REQUIRED, not an optimisation. The effect + // re-runs whenever route data is rebuilt, while Feed.load returns its SAME + // cached array when the params are unchanged — so a navigation or + // invalidation that lands back on the same feed re-runs this effect with an + // identical array. Re-seeding then would un-latch `hasMore` on an + // already-exhausted feed, replacing the end-of-feed placeholder with the + // spinner sentinel and firing a redundant fetch with a stale cursor. + // + // The untrack() inside the effect keeps its secondary reads (`loadFeed`, + // the seed iteration) out of the dependency set, so only a change of + // `posts` identity re-runs it. The init-time untrack()s below are one-time + // seeds: init reads are never reactive, so untrack() there documents the + // intent and silences state_referenced_locally. `lastPosts` is a plain + // `let`, not $state, so updating it here cannot re-trigger the effect. + let hasMore = $state(untrack(() => !!loadFeed)) + let seenUris = untrack(() => seedSeenUris(posts)) + let lastPosts = untrack(() => posts) + + $effect(() => { + const feed = posts + untrack(() => { + if (feed === lastPosts) return + lastPosts = feed + seenUris = seedSeenUris(feed) + hasMore = !!loadFeed + // `error` is per-feed state too. The markup is an if/else chain — + // {#if error} … {:else if hasMore} is what mounts the sentinel — so + // carrying a failure from the previous feed would report an error about + // a feed the user has left AND permanently stall the new one, because + // the sentinel that triggers page 2 never gets mounted. + error = undefined + }) + }) const SCROLL_THRESHOLD = 300 @@ -84,12 +130,22 @@ async function loadMore(): Promise { if (!hasMore || loading || !loadFeed) return + // Captured before the await: a navigation can swap the feed while a page + // is in flight, and every write below targets live bindables. A settlement + // that arrives for a feed the user has left must be discarded wholesale — + // the reset $effect above has already re-seeded the per-feed state, and + // committing would splice the old feed's page into the new feed's array + // and overwrite its cursor with the old feed's continuation. + const feed = posts + try { loading = true const response = await loadFeed(params) - error = null + if (posts !== feed) return + + error = undefined hasMore = response.feed.length !== 0 && !!response.cursor @@ -106,9 +162,20 @@ }), ) } catch (e) { + if (posts !== feed) { + // Not `error = e`: that would raise an error banner on the new feed + // about a request the old feed made. + console.warn( + 'Discarding failed page load for a feed no longer shown:', + e, + ) + return + } console.error('Failed to load more posts:', e) error = e } finally { + // Released unconditionally: `loading` belongs to the request, not the + // feed, and leaving it latched would block the new feed's first page. loading = false } diff --git a/src/lib/feature/post/form/PostForm.svelte b/src/lib/feature/post/form/PostForm.svelte index 38808521..9b16294a 100644 --- a/src/lib/feature/post/form/PostForm.svelte +++ b/src/lib/feature/post/form/PostForm.svelte @@ -18,7 +18,7 @@ TextArea, TextInput, } from 'mono-svelte' - import type { Snippet } from 'svelte' + import { untrack, type Snippet } from 'svelte' import { ChatBubbleBottomCenterText, Photo, @@ -34,7 +34,9 @@ let { init, title, onsubmit }: Props = $props() - let form = $state(init ?? new PostFormState()) + // `init` is by contract the initial form state only — the form owns its + // contents from here on, and re-seeding would discard the user's edits. + let form = $state(untrack(() => init) ?? new PostFormState()) let loading = $state(false) let uploadImage = $state(false) diff --git a/src/lib/feature/post/helpers.test.ts b/src/lib/feature/post/helpers.test.ts index 5f73b557..e40d8359 100644 --- a/src/lib/feature/post/helpers.test.ts +++ b/src/lib/feature/post/helpers.test.ts @@ -3,9 +3,7 @@ import type { CommunityRef, ExternalEmbed, ImageEmbed, - PostStats, PostView, - PostViewerState, RecordEmbed, VideoEmbed, } from '$lib/api/coves/types' @@ -15,7 +13,6 @@ import { bestImageURL, buildPostAtUri, commentLink, - computeVoteState, decodeCrosspostDraft, encodeCrosspostDraft, extractEmbedAlt, @@ -473,69 +470,6 @@ describe('extractEmbedAlt', () => { }) }) -// --------------------------------------------------------------------------- -// computeVoteState() -// --------------------------------------------------------------------------- - -describe('computeVoteState', () => { - const baseStats: PostStats = { - upvotes: 10, - downvotes: 2, - score: 8, - commentCount: 5, - } - const noVoteViewer: PostViewerState = { saved: false } - - it('like from no vote increments upvotes and sets score to upvotes', () => { - const result = computeVoteState(baseStats, noVoteViewer, 'up') - expect(result.stats.upvotes).toBe(11) - expect(result.stats.score).toBe(11) - expect(result.viewer.vote).toBe('up') - }) - - it('toggling off like decrements upvotes and sets score to upvotes', () => { - const upViewer: PostViewerState = { saved: false, vote: 'up' } - const result = computeVoteState(baseStats, upViewer, 'up') - expect(result.stats.upvotes).toBe(9) - expect(result.stats.score).toBe(9) - expect(result.viewer.vote).toBeUndefined() - }) - - it('handles undefined stats gracefully', () => { - const result = computeVoteState(undefined, undefined, 'up') - expect(result.stats.upvotes).toBe(1) - expect(result.stats.downvotes).toBe(0) - expect(result.stats.score).toBe(1) - expect(result.viewer.vote).toBe('up') - }) - - it('does not mutate the original stats', () => { - const originalStats = { ...baseStats } - const originalViewer = { ...noVoteViewer } - computeVoteState(originalStats, originalViewer, 'up') - expect(originalStats.upvotes).toBe(10) - expect(originalViewer.vote).toBeUndefined() - }) - - it('preserves saved state from viewer', () => { - const savedViewer: PostViewerState = { saved: true } - const result = computeVoteState(baseStats, savedViewer, 'up') - expect(result.viewer.saved).toBe(true) - }) - - it('clears voteUri when toggling off a vote', () => { - const upViewer: PostViewerState = { - saved: false, - vote: 'up', - voteUri: - 'at://did:plc:abc/social.coves.community.vote/rkey1' as import('$lib/api/coves/types').AtUri, - } - const result = computeVoteState(baseStats, upViewer, 'up') - expect(result.viewer.vote).toBeUndefined() - expect(result.viewer.voteUri).toBeUndefined() - }) -}) - // --------------------------------------------------------------------------- // buildPostAtUri() // --------------------------------------------------------------------------- diff --git a/src/lib/feature/post/helpers.ts b/src/lib/feature/post/helpers.ts index 704446b3..34c325d2 100644 --- a/src/lib/feature/post/helpers.ts +++ b/src/lib/feature/post/helpers.ts @@ -1,9 +1,4 @@ -import type { - AtUri, - PostEmbed, - PostStats, - PostViewerState, -} from '$lib/api/coves/types' +import type { AtUri, PostEmbed } from '$lib/api/coves/types' import { parseAtUri } from '$lib/api/coves/types' import { canParseUrl, @@ -326,54 +321,6 @@ export function extractEmbedAlt(embed?: PostEmbed): string | undefined { } } -// --------------------------------------------------------------------------- -// Vote state calculation -// --------------------------------------------------------------------------- - -export interface VoteState { - stats: PostStats - viewer: PostViewerState -} - -/** - * Computes the new vote state after a user likes or unlikes. - * - * Pure function: takes the current stats + viewer state and a vote direction, - * returns the new stats + viewer state without mutating the inputs. - * Only handles direction 'up' (like/unlike toggle). - */ -export function computeVoteState( - currentStats: PostStats | undefined, - currentViewer: PostViewerState | undefined, - direction: 'up', -): VoteState { - const stats = { - ...(currentStats ?? { - upvotes: 0, - downvotes: 0, - score: 0, - commentCount: 0, - }), - } - const viewer = { ...(currentViewer ?? { saved: false }) } - - const currentVote = currentViewer?.vote - - if (currentVote === 'up') { - // Unlike: toggle off existing like - stats.upvotes-- - viewer.vote = undefined - viewer.voteUri = undefined - } else { - // Like: add upvote - stats.upvotes++ - viewer.vote = direction - } - stats.score = stats.upvotes - - return { stats, viewer } -} - // --------------------------------------------------------------------------- // Crosspost query-param encoding // --------------------------------------------------------------------------- diff --git a/src/lib/feature/post/vote.test.ts b/src/lib/feature/post/vote.test.ts new file mode 100644 index 00000000..1bcffa76 --- /dev/null +++ b/src/lib/feature/post/vote.test.ts @@ -0,0 +1,134 @@ +import { describe, it, expect } from 'vitest' +import { nextVoteState, toggleUpvote, type VoteCounts } from './vote' + +// --------------------------------------------------------------------------- +// Optimistic vote state +// +// Migrated from the `computeVoteState` block in helpers.test.ts (find it with +// `git log -S 'computeVoteState'`). The score expectations there encoded the +// bug — `score = upvotes`, downvotes ignored — so the fixture is kept verbatim +// and only the oracles are corrected to the backend's invariant, +// `score = upvotes - downvotes` (Coves backend: tests/fixtures/fixtures.go:144, +// tests/e2e/journey_test.go:179). +// +// Every expectation is a literal number. Asserting `score === upvotes - +// downvotes` would be self-fulfilling: it holds for any implementation that is +// internally consistent, including one that moves the wrong counter. +// --------------------------------------------------------------------------- + +const baseCounts: VoteCounts = { + upvotes: 10, + downvotes: 2, + score: 8, +} + +describe('toggleUpvote', () => { + // V1 — was "like from no vote increments upvotes and sets score to upvotes", + // which expected score 11 against a fixture carrying two downvotes. + // Corrected to 9. + it('upvoting from no vote increments upvotes and the score', () => { + const result = toggleUpvote(baseCounts, undefined) + + expect(result.counts).toEqual({ upvotes: 11, downvotes: 2, score: 9 }) + expect(result.vote).toBe('up') + }) + + // V2 — was "toggling off like decrements upvotes and sets score to upvotes", + // which expected score 9. Corrected to 7. + it('toggling off an existing upvote decrements upvotes and the score', () => { + const result = toggleUpvote(baseCounts, 'up') + + expect(result.counts).toEqual({ upvotes: 9, downvotes: 2, score: 7 }) + expect(result.vote).toBeUndefined() + }) + + // V3 — the backend treats a direction switch as delete-then-create: the + // replacement is a new record under a new key, so the old downvote is gone + // (internal/core/votes/service_impl_test.go:343). The optimistic counts must + // release the downvote, not merely add an upvote alongside it. + it('switching from a downvote releases the downvote as it adds the upvote', () => { + const result = toggleUpvote(baseCounts, 'down') + + expect(result.counts).toEqual({ upvotes: 11, downvotes: 1, score: 10 }) + expect(result.vote).toBe('up') + }) + + // V4 — a round trip. Mutation testing showed this kills nothing V1-V3 miss + // (the ±2 mutant it was written for dies to V1, V2 and V3 alike); it is kept + // only because composing the function with itself is how callers use it. + it('upvoting and toggling straight back off restores the original counts', () => { + const applied = toggleUpvote(baseCounts, undefined) + const reverted = toggleUpvote(applied.counts, applied.vote) + + expect(reverted.counts).toEqual(baseCounts) + expect(reverted.vote).toBeUndefined() + }) + + // Counters are rendered straight to the user, so they must never go negative + // when the optimistic state disagrees with the server about what the viewer + // had voted — a stale `viewer.vote` against zeroed counts otherwise produces + // a downvote count of -1, which inflates the score above the upvote count. + it('clamps downvotes at zero when switching from a stale downvote', () => { + const result = toggleUpvote({ upvotes: 0, downvotes: 0, score: 0 }, 'down') + + expect(result.counts).toEqual({ upvotes: 1, downvotes: 0, score: 1 }) + expect(result.vote).toBe('up') + }) + + it('clamps upvotes at zero when toggling off a stale upvote', () => { + const result = toggleUpvote({ upvotes: 0, downvotes: 0, score: 0 }, 'up') + + expect(result.counts).toEqual({ upvotes: 0, downvotes: 0, score: 0 }) + expect(result.vote).toBeUndefined() + }) + + // V5 — ported from "does not mutate the original stats". Callers hold the + // pre-vote counts for rollback and re-read them when the request fails, so + // the input must survive every branch, not just the fresh-vote one. + it.each([undefined, 'up', 'down'] as const)( + 'does not mutate the counts it is given (current vote: %s)', + (currentVote) => { + const counts: VoteCounts = { ...baseCounts } + + toggleUpvote(counts, currentVote) + + expect(counts).toEqual({ upvotes: 10, downvotes: 2, score: 8 }) + }, + ) +}) + +// --------------------------------------------------------------------------- +// nextVoteState() +// +// The viewer half of a vote press, previously split between computeVoteState +// (PostVote's path, tested in helpers.test.ts) and inline code in CommentVote. +// The switch-from-downvote path — the one that carried the deleted record's +// URI forward — had no coverage in either home. +// --------------------------------------------------------------------------- + +describe('nextVoteState', () => { + it('drops the vote and its record URI when toggling off an upvote', () => { + expect(nextVoteState('up')).toEqual({ + vote: undefined, + voteUri: undefined, + }) + }) + + it('records an upvote when there was no previous vote', () => { + expect(nextVoteState(undefined)).toEqual({ + vote: 'up', + voteUri: undefined, + }) + }) + + // The switch is delete-then-create on the backend, so the downvote's record + // is already gone. Carrying its URI forward — what the components do today — + // leaves the optimistic state claiming a vote of 'up' backed by a record the + // backend has deleted, which is the URI a subsequent toggle-off would send. + it('does not carry the old downvote record URI into the switched upvote', () => { + const result = nextVoteState('down') + + expect(result.vote).toBe('up') + expect(result.voteUri).toBeUndefined() + }) +}) diff --git a/src/lib/feature/post/vote.ts b/src/lib/feature/post/vote.ts new file mode 100644 index 00000000..991b3aab --- /dev/null +++ b/src/lib/feature/post/vote.ts @@ -0,0 +1,109 @@ +// --------------------------------------------------------------------------- +// Optimistic vote state +// +// Shared by PostVote.svelte and CommentVote.svelte, whose stats types differ +// only in the counter they carry alongside (`commentCount` vs `replyCount`). +// This module owns the three vote counters and the viewer's vote; callers +// spread the result over their own typed base to keep the fields it does not +// know about. +// --------------------------------------------------------------------------- + +import type { AtUri, VoteCounts } from '$lib/api/coves/types' + +// Re-exported so vote logic imports its counter type from the module that +// operates on it. It is declared in the API types module because `PostStats` +// and `CommentStats` extend it there, and this module already depends on that +// one for `AtUri`. +export type { VoteCounts } + +/** + * Decrements a counter, refusing to go below zero. + * + * A negative counter is reachable whenever the viewer state we hold disagrees + * with the server about what was voted — a stale `viewer.vote` against zeroed + * counts. The counters are rendered verbatim, and a downvote count of -1 + * inflates the score above the upvote count, so the clamp prevents visible + * corruption. The warning is what keeps the clamp from also hiding the desync + * that caused it. + */ +function decrement(count: number, counter: string): number { + if (count <= 0) { + console.warn( + `[vote] refusing to decrement ${counter} below zero — optimistic state is out of sync with the server`, + { count }, + ) + return 0 + } + return count - 1 +} + +/** + * Applies an upvote press to `counts`, returning the optimistic counters. + * + * `score` is always recomputed as `upvotes - downvotes`, the backend's + * invariant. Stating it is deliberate: the implementation this replaced set + * `score = upvotes` and dropped downvotes from the display entirely. The + * invariant is pinned locally by `vote.test.ts`; upstream it is visible in the + * Coves backend's vote fixtures and end-to-end journey assertions. + * + * Switching direction releases the old downvote, because the backend treats a + * direction switch as delete-then-create — see the "Different direction - + * delete old vote first" branch in `internal/core/votes/service_impl.go`, and + * `TestCreateVote_DifferentDirectionReplacesUnderANewRKey`. Leaving + * `downvotes` untouched would leave the rendered count wrong by one until the + * next refetch. + * + * Caveat on that release: the same backend function has a KNOWN DEFECT where a + * direction switch does not roll the delete back if the subsequent create + * fails. "The old downvote no longer exists" therefore describes the happy + * path only; on that failure the subject is left with no vote at all rather + * than the original downvote. + * + * Pure: `counts` is never mutated. + */ +export function toggleUpvote( + counts: VoteCounts, + currentVote: 'up' | 'down' | undefined, +): { counts: VoteCounts; vote: 'up' | undefined } { + const isToggleOff = currentVote === 'up' + + const upvotes = isToggleOff + ? decrement(counts.upvotes, 'upvotes') + : counts.upvotes + 1 + const downvotes = + currentVote === 'down' + ? decrement(counts.downvotes, 'downvotes') + : counts.downvotes + + return { + counts: { upvotes, downvotes, score: upvotes - downvotes }, + vote: isToggleOff ? undefined : 'up', + } +} + +/** + * The viewer half of an upvote press: the vote it leaves behind, and the record + * URI that vote is backed by. + * + * `voteUri` is undefined on every branch, and that is the substance of this + * function rather than an oversight. Toggling off deletes the record; switching + * from a downvote deletes it too and creates a new one under a different rkey; + * a first vote never had one. In all three cases the URI the caller was holding + * is stale the moment the press is made, and the replacement is not knowable + * until the server answers. Carrying the old one forward — which both + * components did inline — leaves the optimistic state claiming an upvote backed + * by a record the backend has already deleted, and that is the URI a subsequent + * toggle-off would send. + * + * The caller's current URI is therefore not a parameter: there is no branch on + * which it could be returned, so taking it would only imply otherwise. + */ +export function nextVoteState(currentVote: 'up' | 'down' | undefined): { + vote: 'up' | undefined + voteUri: AtUri | undefined +} { + return { + vote: currentVote === 'up' ? undefined : 'up', + voteUri: undefined, + } +} diff --git a/src/lib/ui/icon/AnimatedHeart.svelte b/src/lib/ui/icon/AnimatedHeart.svelte index 4b1e4063..3d0be526 100644 --- a/src/lib/ui/icon/AnimatedHeart.svelte +++ b/src/lib/ui/icon/AnimatedHeart.svelte @@ -1,4 +1,6 @@ diff --git a/src/lib/ui/shared/popover/Popover.svelte b/src/lib/ui/shared/popover/Popover.svelte index 3b6b1b6f..493a3dc9 100644 --- a/src/lib/ui/shared/popover/Popover.svelte +++ b/src/lib/ui/shared/popover/Popover.svelte @@ -58,10 +58,13 @@ 'top-start': 'bottom', } + // Only `onComputed` is fixed for the lifetime of the component. The + // positioning options are handed to `floatingContent` as its action argument + // instead of being baked in here, so a change to any of them re-runs the + // action's update() with the new values. Baking them in would capture them at + // init, and createFloatingActions' autoUpdate loop would keep recomputing + // against that captured config on every scroll and resize. const [floatingRef, floatingContent] = createFloatingActions({ - strategy: strategy, - placement: placement, - middleware: middleware, onComputed: ({ placement }) => { if (popoverEl) { popoverEl.style.transformOrigin = origins[placement] @@ -145,7 +148,7 @@ easing: expoOut, }} class={['z-150', popoverClass]} - use:floatingContent + use:floatingContent={{ strategy, placement, middleware }} use:trapFocus bind:this={popoverEl} > diff --git a/src/routes/+layout.svelte b/src/routes/+layout.svelte index 10eb5ae8..1ce0968e 100644 --- a/src/routes/+layout.svelte +++ b/src/routes/+layout.svelte @@ -38,35 +38,8 @@ showSpinner: false, }) - /** - * Reads and clears the kelp_flash cookie for session expiration messages. - * This cookie is set by the server when session decryption fails. - */ - function handleFlashMessage() { - const cookies = document.cookie.split(';') - const flashCookie = cookies.find((c) => c.trim().startsWith('kelp_flash=')) - if (!flashCookie) return - - try { - const value = decodeURIComponent(flashCookie.split('=')[1]) - const flash = JSON.parse(value) as { type: string; message: string } - - if (flash.type === 'session_expired') { - toast({ content: $t('toast.sessionExpired'), type: 'warning' }) - } - } catch (e) { - console.warn('Failed to parse flash cookie:', e) - } - - // Clear the cookie regardless of success/failure - document.cookie = 'kelp_flash=; path=/; max-age=0' - } - onMount(() => { if (browser) { - // Handle flash messages from server (e.g., session expiration) - handleFlashMessage() - if (window.location.hash == 'main') { history.replaceState( null, @@ -113,8 +86,32 @@ profile.syncFromServer(page.data.session ?? undefined) }) + // Tell the user their session ended rather than letting them discover it by + // being silently logged out. hooks.server.ts deletes the stale cookie and + // sets locals.sessionExpired on a 401 from /api/me; +layout.server.ts + // forwards it as page.data.sessionExpired. + // + // The latch is REQUIRED, not defensive. The root layout's server load reads + // only `request` and `locals` — no params, no url, no depends() — so plain + // client-side navigations never re-run it: page.data.sessionExpired stays + // true (and this effect re-runs on each one) until something re-runs the + // load — an invalidateAll navigation (every sort/search change via + // searchParam()), a form action, or a full reload. Without the latch that + // is a toast on every navigation in between. + let notifiedSessionExpired = false + $effect(() => { + if (page.data.sessionExpired) { + if (!notifiedSessionExpired) { + notifiedSessionExpired = true + toast({ content: $t('toast.sessionExpired'), type: 'warning' }) + } + } else { + notifiedSessionExpired = false + } + }) + // Surface auth infrastructure failures from hooks.server.ts (mirrors the - // sessionExpired flash handling above): the backend couldn't be reached to + // sessionExpired handling above): the backend couldn't be reached to // validate the session, so the user may appear logged out even though their // session cookie is preserved. Warn once per outage rather than on every // navigation while the backend stays unreachable. diff --git a/src/routes/explore/+layout.svelte b/src/routes/explore/+layout.svelte index d1009ddb..030fff62 100644 --- a/src/routes/explore/+layout.svelte +++ b/src/routes/explore/+layout.svelte @@ -4,6 +4,7 @@ import { searchParam } from '$lib/app/util.svelte' import { Header, SearchBar } from '$lib/ui/layout' import { Option, Select } from 'mono-svelte' + import { untrack } from 'svelte' import { ChartBar, Fire, @@ -15,8 +16,43 @@ let { data, children } = $props() - let search = $state(page.data.query || '') - let sort = $state(data.sort) + // Navigating within /explore re-runs load() but reuses this layout, so a + // lone one-time seed would leave the controls showing whatever the URL said + // when the section was first entered. The URL stays the source of truth and + // the effects below re-sync after a navigation that actually changes it. + // Seeds are still needed because effects don't run during SSR — the + // server-rendered first paint needs values; init reads are never reactive, + // so untrack() marks them as deliberate one-time seeds and silences + // state_referenced_locally. + // + // The latches are REQUIRED, not an optimisation. `page.data` is $state.raw, + // which SvelteKit replaces wholesale on every navigation, and `data` arrives + // through a freshly built object each load — so these effects re-run on every + // navigation regardless of whether the value changed. SearchBar binds + // straight into `search`, so an unguarded re-sync would erase whatever the + // user had typed the moment the sort dropdown fired its `goto(..., + // { invalidateAll: true })`. Only a change in the URL-derived value may + // overwrite the local control. `lastQuery`/`lastSort` are plain `let`s, not + // $state, so updating them here cannot re-trigger the effect. + let search = $state(untrack(() => page.data.query || '')) + let sort = $state(untrack(() => data.sort)) + + let lastQuery = untrack(() => page.data.query || '') + let lastSort = untrack(() => data.sort) + + $effect(() => { + const query = page.data.query || '' + if (query === lastQuery) return + lastQuery = query + search = query + }) + + $effect(() => { + const routeSort = data.sort + if (routeSort === lastSort) return + lastSort = routeSort + sort = routeSort + }) diff --git a/src/routes/profile/[handle=handle]/+page.ts b/src/routes/profile/[handle=handle]/+page.ts index 04da818b..915c2b6f 100644 --- a/src/routes/profile/[handle=handle]/+page.ts +++ b/src/routes/profile/[handle=handle]/+page.ts @@ -1,50 +1,48 @@ import { error } from '@sveltejs/kit' import { coves } from '$lib/api/client.svelte' +import { XrpcError } from '$lib/api/coves/xrpc' import { isValidDID, isValidHandle } from '$lib/types/atproto' import { ReactiveState } from '$lib/app/util.svelte' import { feed } from '$lib/feature/feeds/feed.svelte' export async function load({ params, url, fetch, route }) { const cursor = url.searchParams.get('cursor') ?? undefined - const sort = url.searchParams.get('sort') ?? 'new' const feedData = await feed(route.id, async (p) => { if (!isValidHandle(p.actor) && !isValidDID(p.actor)) { error(400, 'Invalid user identifier') } const actor = p.actor + const api = coves({ func: fetch }) - try { - const [profileData, postsData, commentsData] = await Promise.all([ - coves({ func: fetch }).getProfile({ actor }), - coves({ func: fetch }).getActorPosts({ - actor, - limit: p.limit, - cursor: p.cursor, - }), - coves({ func: fetch }).getActorComments({ - actor, - limit: p.limit, - cursor: p.cursor, - }), - ]) + const [profileData, postsData, commentsData] = await Promise.all([ + api.getProfile({ actor }).catch((e: unknown) => { + // Scoped to this call deliberately: of the three, only getProfile + // answers "does this account exist?". A 404 from the posts or comments + // call is an infrastructure fault — a stale AppView, a proxy misroute — + // and telling the viewer the account is gone would turn an outage into + // a deleted-account story. Those propagate untouched. + // + // Bare i18n key, not prose: `errorMessage` in $lib/app/error.ts only + // translates messages matching /^[\w-]+$/. + if (e instanceof XrpcError && e.status === 404) { + error(404, 'couldnt_find_person') + } + throw e + }), + api.getActorPosts({ actor, limit: p.limit, cursor: p.cursor }), + api.getActorComments({ actor, limit: p.limit, cursor: p.cursor }), + ]) - return { - profile: profileData, - posts: postsData, - comments: commentsData, - } - } catch (err) { - if (err instanceof Error && err.message.includes('not found')) { - error(404, 'couldnt_find_person') - } - error(500, 'Failed to load profile') + return { + profile: profileData, + posts: postsData, + comments: commentsData, } }).load({ actor: params.handle, limit: 20, cursor, - sort, }) return { diff --git a/src/routes/profile/[handle=handle]/UserActions.svelte b/src/routes/profile/[handle=handle]/UserActions.svelte index 61797c15..8cdc1ac0 100644 --- a/src/routes/profile/[handle=handle]/UserActions.svelte +++ b/src/routes/profile/[handle=handle]/UserActions.svelte @@ -1,9 +1,12 @@ diff --git a/src/routes/profile/[handle=handle]/page.test.ts b/src/routes/profile/[handle=handle]/page.test.ts new file mode 100644 index 00000000..0f8a7f98 --- /dev/null +++ b/src/routes/profile/[handle=handle]/page.test.ts @@ -0,0 +1,222 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +// --------------------------------------------------------------------------- +// Mocks +// +// The loader is exercised for its error translation only: what an upstream XRPC +// failure turns into by the time it reaches SvelteKit's router. `$app/environment` +// is deliberately NOT mocked — with `browser` false, `feed()` hands back a fresh +// Feed per load, so no cached success can shadow a rejecting mock. +// --------------------------------------------------------------------------- + +const mockCovesMethods = vi.hoisted(() => ({ + getProfile: vi.fn(), + getActorPosts: vi.fn(), + getActorComments: vi.fn(), +})) + +// The factory itself is a spy so the loader's client construction is +// observable — see the fetch pass-through assertion in the happy-path test. +const mockCoves = vi.hoisted(() => vi.fn(() => mockCovesMethods)) + +vi.mock('$lib/api/client.svelte', () => ({ + coves: mockCoves, +})) + +// `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 } }, +})) + +import { isHttpError } from '@sveltejs/kit' +import { XrpcError } from '$lib/api/coves/xrpc' +import { load } from './+page' + +// The loader only destructures { params, url, fetch, 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 = ''): Parameters[0] { + return { + params: { handle }, + url: new URL(`https://coves.test/profile/${handle}${query}`), + // A distinct spy, not globalThis.fetch: the pass-through assertion below + // must be able to tell SvelteKit's per-request fetch from the global one. + fetch: vi.fn(), + route: { id: '/profile/[handle=handle]' }, + } as unknown as Parameters[0] +} + +/** Resolves with whatever the promise rejected with; fails if it resolves. */ +async function rejection(promise: Promise): Promise { + return promise.then( + (value) => { + throw new Error( + `expected load to reject, but it resolved with ${JSON.stringify(value)}`, + ) + }, + (err: unknown) => err, + ) +} + +const profileFixture = { + did: 'did:plc:alice', + handle: 'alice.coves.social', +} +const postsFixture = { posts: [] } +const commentsFixture = { comments: [] } + +describe('profile loader', () => { + let consoleError: ReturnType + + beforeEach(() => { + // `Feed.load` console.errors every rejection before rethrowing it. + consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + + mockCoves.mockClear() + mockCovesMethods.getProfile.mockReset().mockResolvedValue(profileFixture) + mockCovesMethods.getActorPosts.mockReset().mockResolvedValue(postsFixture) + mockCovesMethods.getActorComments + .mockReset() + .mockResolvedValue(commentsFixture) + }) + + afterEach(() => { + consoleError.mockRestore() + }) + + // Every other test here asserts a rejection, so a loader that threw + // unconditionally would fail exactly one of them. This is the one. + it('returns the profile, posts and comments for a valid actor', async () => { + const args = makeArgs('alice.coves.social', '?cursor=page2') + const result = await load(args) + + expect(result.data.value).toEqual({ + profile: profileFixture, + posts: postsFixture, + comments: commentsFixture, + }) + // SvelteKit's per-request fetch must reach the client factory — it + // carries SSR cookie/credential forwarding, and no other assertion + // notices if the loader silently drops it for the global fetch. + expect(mockCoves).toHaveBeenCalledWith({ func: args.fetch }) + expect(mockCovesMethods.getProfile).toHaveBeenCalledWith({ + actor: 'alice.coves.social', + }) + expect(mockCovesMethods.getActorPosts).toHaveBeenCalledWith({ + actor: 'alice.coves.social', + limit: 20, + cursor: 'page2', + }) + expect(mockCovesMethods.getActorComments).toHaveBeenCalledWith({ + actor: 'alice.coves.social', + limit: 20, + cursor: 'page2', + }) + }) + + // B1 — an upstream 404 must reach the router as a 404 whose message is the + // i18n key `couldnt_find_person`. The key shape is load-bearing: `errorMessage` + // in src/lib/app/error.ts only translates messages matching /^[\w-]+$/, so a + // prose message would silently ship untranslated to every locale. + // + // Parameterised over the 404 shapes the client can actually produce, because + // the status is what decides routing — never the wording of `message`. The + // last row is what `XrpcClient#parseError` synthesises when a 404 response + // body is not the XRPC error JSON (e.g. a proxy-level miss). + // + // Scoped to `getProfile`: only that call answers "does this account exist?". + it.each([ + ['ProfileNotFound', 'user not found'], + ['NotFound', 'Profile not found'], + ['UnknownError', 'XRPC request failed with status 404'], + ])( + 'turns an upstream 404 (%s: "%s") into a 404 keyed couldnt_find_person', + async (errorName, message) => { + mockCovesMethods.getProfile.mockRejectedValue( + new XrpcError(404, errorName, message), + ) + + const thrown = await rejection(load(makeArgs('ghost.coves.social'))) + + expect(isHttpError(thrown)).toBe(true) + expect(thrown).toMatchObject({ + status: 404, + body: { message: 'couldnt_find_person' }, + }) + }, + ) + + // B2 — a non-404 upstream failure is not the loader's to reinterpret: the + // original XrpcError must reach the caller with its own status and error name + // intact, rather than every failure being flattened into one server error. + it.each([ + [500, 'InternalServerError', 'boom'], + [401, 'AuthenticationRequired', 'Invalid token'], + [502, 'UpstreamFailure', 'PDS unreachable'], + ])( + 'propagates the original XrpcError for a %i upstream failure', + async (status, errorName, message) => { + const upstream = new XrpcError(status, errorName, message) + mockCovesMethods.getProfile.mockRejectedValue(upstream) + + const thrown = await rejection(load(makeArgs('alice.coves.social'))) + + expect(thrown).toBe(upstream) + expect(isHttpError(thrown)).toBe(false) + }, + ) + + // A failure that never reached XRPC — a DNS miss, an aborted request, a + // programming error inside the init closure — is likewise not the loader's to + // reinterpret. Without this, an `instanceof XrpcError` check could be widened + // to catch everything and no test would notice. + it('propagates a rejection that is not an XrpcError at all', async () => { + const upstream = new TypeError('fetch failed') + mockCovesMethods.getProfile.mockRejectedValue(upstream) + + const thrown = await rejection(load(makeArgs('alice.coves.social'))) + + expect(thrown).toBe(upstream) + expect(isHttpError(thrown)).toBe(false) + }) + + // B3 — not characterization: this is the only test that kills a blanket + // `error(500, 'Failed to load profile')` fallback, because the 400 it expects + // is an HttpError rather than an XrpcError and so takes a different path out + // of the catch. Deleting it reopens that hole. + it('rejects an actor that is neither a handle nor a DID with a 400', async () => { + const thrown = await rejection(load(makeArgs('not-an-identifier'))) + + expect(thrown).toMatchObject({ + status: 400, + body: { message: 'Invalid user identifier' }, + }) + expect(mockCovesMethods.getProfile).not.toHaveBeenCalled() + }) + + // B4 — the three upstream calls share one `Promise.all`, so any of them can + // be the rejecter, but only `getProfile` speaks to whether the account + // exists. A stale AppView or a proxy misroute 404ing the posts or comments + // call is an infrastructure fault; rendering "that user doesn't exist" for it + // tells the viewer a deleted-account story about a service outage. + it.each([ + ['getActorPosts', mockCovesMethods.getActorPosts] as const, + ['getActorComments', mockCovesMethods.getActorComments] as const, + ])( + 'propagates a 404 from %s rather than calling the actor missing', + async (_name, method) => { + const upstream = new XrpcError( + 404, + 'UnknownError', + 'XRPC request failed with status 404', + ) + method.mockRejectedValue(upstream) + + const thrown = await rejection(load(makeArgs('alice.coves.social'))) + + expect(thrown).toBe(upstream) + expect(isHttpError(thrown)).toBe(false) + }, + ) +}) diff --git a/tsconfig.json b/tsconfig.json index 44b02043..66e19956 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -9,7 +9,7 @@ "skipLibCheck": true, "sourceMap": true, "strict": true, - "types": ["vite-plugin-pwa/client", "node"] + "types": ["node"] }, // Must copy include from base config since TypeScript doesn't merge arrays "include": [ -- 2.51.2