From 981b55e90dbcc12f387b1a52cc7e056ccfaa49cd Mon Sep 17 00:00:00 2001 From: Georgy Butaev <41178744+g-but@users.noreply.github.com> Date: Fri, 28 Aug 2026 23:34:03 +0200 Subject: [PATCH 1/2] perf(auth): six copies of "who is reading", each an uncached round-trip getCurrentUserId was defined six times across services - timeline queries, timeline processors, groups, loans, projects, and the auth layer. Every copy called supabase.auth.getUser(), which is a NETWORK call: it validates the token against /auth/v1/user. So a single page asked the server who the reader was over and over, and the cache I added yesterday covered only four of the six importers. On the timeline that lookup also sat on the critical path. Measured on a cold load: 2429 -> 3061 get_user_timeline_feed 632ms 3105 -> 3351 auth/v1/user 246ms <- depends on nothing 3367 -> 3595 reactions x3 228ms <- waits on the one above The identity lookup ran after the feed returned, and then the reaction queries waited on it in turn, though it depends on neither. Changes: - One definition, in the auth layer, caching the in-flight promise so concurrent callers collapse onto one request. Safe as module state only because this module talks exclusively to the browser client; a per-request server client would hand one request's user to the next, and the one caller that passes its own client (groups) keeps its uncached path. - The cache is dropped on any auth state change, registered lazily on first use rather than at import - this module is imported very widely and an import-time subscription is a side effect every importer pays for. - getUserFeed warms the id alongside the feed request instead of after it, and asks for the total count concurrently rather than after enrichment. - check:one-current-user fails the build if a seventh copy appears. Wired into verify. A test caught a real behaviour change while writing this: getUser() catches its own errors and reports them in `error` rather than throwing, so "could not ask" arrived looking like "nobody is signed in" and would have been cached as signed-out for the rest of the page. Fixed the code, not the test. Mutation-proved: a seventh definition -> red; the owner losing its definition -> red; both green after restore; and the gate is reached through npm, not just when run by hand. Full suite: 261 suites, 2524 passed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012dpTLxh5GJWeWTF1UEvcD5 --- .../services/current-user-id-cache.test.ts | 46 ++++++++-- package.json | 3 +- scripts/check-one-current-user.mjs | 72 +++++++++++++++ src/services/groups/utils/helpers.ts | 14 ++- src/services/loans/utils/auth.ts | 16 +--- src/services/projects/support/helpers.ts | 24 +---- src/services/supabase/auth/session.ts | 90 +++++++++++++++++-- .../timeline/processors/social-shared.ts | 57 ++---------- src/services/timeline/queries/helpers.ts | 18 +--- src/services/timeline/queries/userFeeds.ts | 43 ++++++--- 10 files changed, 255 insertions(+), 128 deletions(-) create mode 100644 scripts/check-one-current-user.mjs diff --git a/__tests__/unit/services/current-user-id-cache.test.ts b/__tests__/unit/services/current-user-id-cache.test.ts index 19fa3b4a3..8ce1e0d72 100644 --- a/__tests__/unit/services/current-user-id-cache.test.ts +++ b/__tests__/unit/services/current-user-id-cache.test.ts @@ -12,12 +12,23 @@ * onto one request rather than racing to make several. */ -import { getCurrentUserId, __resetCurrentUserIdCache } from '@/services/timeline/processors/social-shared'; +import { + getCurrentUserId, + __resetCurrentUserIdCache, +} from '@/services/supabase/auth/session'; +// The timeline re-exports it rather than defining its own; both names must be +// the same function, or the cache would only cover half the callers. +import { getCurrentUserId as timelineGetCurrentUserId } from '@/services/timeline/processors/social-shared'; const getUser = jest.fn(); jest.mock('@/lib/supabase/browser', () => ({ __esModule: true, - default: { auth: { getUser: (...a: unknown[]) => getUser(...a) } }, + default: { + auth: { + getUser: (...a: unknown[]) => getUser(...a), + onAuthStateChange: () => ({ data: { subscription: { unsubscribe: () => {} } } }), + }, + }, })); describe('getCurrentUserId', () => { @@ -46,18 +57,43 @@ describe('getCurrentUserId', () => { expect(getUser).toHaveBeenCalledTimes(1); }); - it('does not cache a failure as "signed out"', async () => { + it('does not cache a thrown failure as "signed out"', async () => { __resetCurrentUserIdCache(); getUser.mockRejectedValueOnce(new Error('network blip')); expect(await getCurrentUserId()).toBeNull(); - // A blip must not outlive itself and render the whole timeline as - // signed-out for the rest of the page. + // A blip must not outlive itself and render the whole app as signed-out + // for the rest of the page. getUser.mockResolvedValue({ data: { user: { id: 'u1' } } }); expect(await getCurrentUserId()).toBe('u1'); }); + it('does not cache a REPORTED failure as "signed out" either', async () => { + // The auth layer catches its own errors and reports them in `error` rather + // than throwing, so "could not ask" arrives looking almost exactly like + // "nobody is signed in". Only the error field tells them apart. + __resetCurrentUserIdCache(); + getUser.mockResolvedValueOnce({ + data: { user: null }, + error: { message: 'fetch failed' }, + }); + + expect(await getCurrentUserId()).toBeNull(); + + getUser.mockResolvedValue({ data: { user: { id: 'u1' } } }); + expect(await getCurrentUserId()).toBe('u1'); + }); + + it('is the SAME function the timeline uses, so one cache covers both', async () => { + expect(timelineGetCurrentUserId).toBe(getCurrentUserId); + + await getCurrentUserId(); + await timelineGetCurrentUserId(); + + expect(getUser).toHaveBeenCalledTimes(1); + }); + it('caches a genuine signed-out answer', async () => { __resetCurrentUserIdCache(); getUser.mockResolvedValue({ data: { user: null } }); diff --git a/package.json b/package.json index d47f4c6dc..1c3804dbd 100644 --- a/package.json +++ b/package.json @@ -37,10 +37,11 @@ "check:schema-columns": "node scripts/check-schema-columns.mjs", "check:currency-units": "node scripts/check-currency-units.mjs", "check:rpc-exists": "node scripts/check-rpc-exists.mjs", + "check:one-current-user": "node scripts/check-one-current-user.mjs", "check:client-ip": "node scripts/check-client-ip.mjs", "check:ai-models": "node scripts/check-ai-models.mjs", "check:mdx": "node scripts/check-mdx.mjs", - "verify": "npm run ci:docs && npm run check:accent-ink && npm run type-check && npm run type-check:scripts && npm run check:sizes && npm run audit:routes && npm run lint && npm run check:duplication && npm run check:dead-fields && npm run check:migration-versions && npm run check:schema-columns && npm run check:currency-units && npm run check:rpc-exists && npm run check:client-ip && npm run check:mdx && npm run test:unit -- --watchAll=false", + "verify": "npm run ci:docs && npm run check:accent-ink && npm run type-check && npm run type-check:scripts && npm run check:sizes && npm run audit:routes && npm run lint && npm run check:duplication && npm run check:dead-fields && npm run check:migration-versions && npm run check:schema-columns && npm run check:currency-units && npm run check:rpc-exists && npm run check:one-current-user && npm run check:client-ip && npm run check:mdx && npm run test:unit -- --watchAll=false", "audit:schema": "node scripts/db/audit-schema-drift.mjs", "audit:routes": "node scripts/audit-routes.mjs", "gen:types": "bash scripts/db/gen-types.sh", diff --git a/scripts/check-one-current-user.mjs b/scripts/check-one-current-user.mjs new file mode 100644 index 000000000..c9c76fa2b --- /dev/null +++ b/scripts/check-one-current-user.mjs @@ -0,0 +1,72 @@ +#!/usr/bin/env node +/** + * "Who is reading?" gets exactly one answer, from one place. + * + * Six copies of `getCurrentUserId` had accumulated across services — timeline + * queries, timeline processors, groups, loans, projects, and the auth layer — + * each one an uncached network call to /auth/v1/user. A single timeline load + * asked the server who the reader was repeatedly, and on the critical path: + * the lookup ran after the feed returned and gated the reaction queries. + * + * They were collapsed onto src/services/supabase/auth/session.ts, which caches + * the in-flight promise. This gate exists so the seventh copy is a red build + * rather than another silent round-trip: a duplicate is easy to reintroduce, + * costs nothing visible, and nothing else would ever catch it. + * + * Definitions are what this counts. Re-exports (`export { getCurrentUserId } + * from ...`) are the intended way to expose it from a service's own module. + */ + +import { readFileSync } from 'node:fs'; +import { execSync } from 'node:child_process'; + +const OWNER = 'src/services/supabase/auth/session.ts'; + +/** + * A caller may legitimately pass its own Supabase client — a per-request server + * client, where a module-level cache would hand one request's user to the next. + * Those wrappers take a client parameter and delegate when there isn't one. + */ +const DELEGATING_WRAPPERS = new Set(['src/services/groups/utils/helpers.ts']); + +const files = execSync('git ls-files "src/**/*.ts" "src/**/*.tsx"', { encoding: 'utf8' }) + .split('\n') + .filter(Boolean); + +const defines = []; +for (const file of files) { + const body = readFileSync(file, 'utf8'); + // A definition, not a re-export or an import. + if (/export\s+(async\s+)?function\s+getCurrentUserId\b/.test(body)) { + defines.push(file); + } +} + +const unexpected = defines.filter(f => f !== OWNER && !DELEGATING_WRAPPERS.has(f)); + +if (!defines.includes(OWNER)) { + console.error(`✗ ${OWNER} no longer defines getCurrentUserId.`); + console.error(' That file is the single definition. Move it back, or update this gate'); + console.error(' deliberately — do not let the definition drift somewhere unnoticed.'); + process.exit(1); +} + +if (unexpected.length > 0) { + console.error('✗ getCurrentUserId is defined in more than one place:'); + for (const f of unexpected) { + console.error(` - ${f}`); + } + console.error(''); + console.error(`Import or re-export it from ${OWNER} instead:`); + console.error(" export { getCurrentUserId } from '@/services/supabase/auth/session';"); + console.error(''); + console.error('Every copy is an uncached round-trip to /auth/v1/user. Six of them once'); + console.error('shared one page load. If your caller must pass its own Supabase client,'); + console.error('add it to DELEGATING_WRAPPERS in this script and delegate when it has none.'); + process.exit(1); +} + +console.log( + `check:one-current-user passed — getCurrentUserId defined once (${OWNER})` + + (DELEGATING_WRAPPERS.size ? `, ${DELEGATING_WRAPPERS.size} delegating wrapper(s)` : '') +); diff --git a/src/services/groups/utils/helpers.ts b/src/services/groups/utils/helpers.ts index 5eaa7dea4..1138549bf 100644 --- a/src/services/groups/utils/helpers.ts +++ b/src/services/groups/utils/helpers.ts @@ -13,17 +13,25 @@ import { logger } from '@/utils/logger'; import { DATABASE_TABLES } from '@/config/database-tables'; import { slugify } from '@/utils/string'; import type { AnySupabaseClient } from '@/lib/supabase/types'; +import { getCurrentUserId as getSharedCurrentUserId } from '@/services/supabase/auth/session'; import { fromTable } from '../db-helpers'; /** - * Get current authenticated user ID + * Get current authenticated user ID. + * + * With no client this is the shared, cached answer from the auth layer. With an + * explicit client it asks that client directly and caches nothing: a caller + * passing its own client is usually passing a per-request server client, and a + * module-level cache there would hand one request's user to the next. */ export async function getCurrentUserId(client?: AnySupabaseClient): Promise { + if (!client) { + return getSharedCurrentUserId(); + } try { - const supabaseClient = client || supabase; const { data: { user }, - } = await supabaseClient.auth.getUser(); + } = await client.auth.getUser(); return user?.id || null; } catch (error) { logger.error('Error getting current user ID', error, 'Groups'); diff --git a/src/services/loans/utils/auth.ts b/src/services/loans/utils/auth.ts index 3ee2b5a65..38e47a315 100644 --- a/src/services/loans/utils/auth.ts +++ b/src/services/loans/utils/auth.ts @@ -6,18 +6,6 @@ * Last Modified Summary: Extracted from loans/index.ts for modularity */ -import supabase from '@/lib/supabase/browser'; -/** - * Get current authenticated user ID - */ -export async function getCurrentUserId(): Promise { - try { - const { - data: { user }, - } = await supabase.auth.getUser(); - return user?.id || null; - } catch { - return null; - } -} +// Defined once, in the auth layer, where it is cached. See session.ts. +export { getCurrentUserId } from '@/services/supabase/auth/session'; diff --git a/src/services/projects/support/helpers.ts b/src/services/projects/support/helpers.ts index 88afd723e..88d04d12a 100644 --- a/src/services/projects/support/helpers.ts +++ b/src/services/projects/support/helpers.ts @@ -8,29 +8,9 @@ * Last Modified Summary: Created project support helper functions */ -import supabase from '@/lib/supabase/browser'; -import { logger } from '@/utils/logger'; -/** - * Get current user ID - */ -export async function getCurrentUserId(): Promise { - try { - const { - data: { user }, - error, - } = await supabase.auth.getUser(); - - if (error || !user) { - return null; - } - - return user.id; - } catch (error) { - logger.error('Error getting current user ID', error, 'ProjectSupport'); - return null; - } -} +// Defined once, in the auth layer, where it is cached. See session.ts. +export { getCurrentUserId } from '@/services/supabase/auth/session'; // Re-export formatSats from SSOT to avoid duplication // Use useDisplayCurrency hook in components for user-preferred currency diff --git a/src/services/supabase/auth/session.ts b/src/services/supabase/auth/session.ts index 180ab2e2f..6d67888e9 100644 --- a/src/services/supabase/auth/session.ts +++ b/src/services/supabase/auth/session.ts @@ -71,11 +71,91 @@ export async function isAuthenticated(): Promise { } } +/** + * Who is reading — the ONE definition, cached for as long as it stays true. + * + * `supabase.auth.getUser()` is a network call: it validates the token against + * `/auth/v1/user`. There were six independent copies of this function across + * services (timeline, groups, loans, projects, this file), each uncached, so a + * single page could ask the server who the reader was a dozen times over. On + * the timeline that lookup also sat on the critical path: it ran after the feed + * returned and gated the reaction queries, though it depends on neither. + * + * Cached as the in-flight PROMISE, not the value, so concurrent callers + * collapse onto one request instead of racing to make several. A failure is + * never cached — caching "nobody is signed in" would outlive the blip that + * caused it and quietly render the app as signed-out. + * + * Safe as module state ONLY because this module talks exclusively to the + * browser client. A server client is per-request, and a module-level identity + * cache there would hand one request's user to the next. + */ +let cachedUserId: string | null | undefined; +let userIdInFlight: Promise | null = null; +let watchingAuth = false; + +/** + * Drop the cached id whenever Supabase says the session changed. + * + * Sign-in and sign-out normally replace the page, but "normally" is not a + * guarantee — token refreshes and same-document auth changes happen too, and an + * id must never outlive the session it came from. + * + * Registered on first use rather than at import. This module is imported very + * widely, and a subscription that runs at import time is a side effect every + * importer pays for — including every test that stubs the client. + */ +function watchAuthChanges(): void { + if (watchingAuth || typeof window === 'undefined') { + return; + } + watchingAuth = true; + supabase.auth.onAuthStateChange(() => { + __resetCurrentUserIdCache(); + }); +} + export async function getCurrentUserId(): Promise { - try { - const { user } = await getUser(); - return user?.id || null; - } catch { - return null; + watchAuthChanges(); + if (cachedUserId !== undefined) { + return cachedUserId; + } + if (!userIdInFlight) { + userIdInFlight = (async () => { + try { + const { user, error } = await getUser(); + if (error) { + // getUser() swallows the throw and reports the failure here instead, + // so this branch is the only thing separating "signed out" from + // "could not ask". Caching the two the same way would turn one + // network blip into a signed-out session for the rest of the page. + return null; + } + cachedUserId = user?.id || null; + return cachedUserId; + } catch { + return null; + } finally { + userIdInFlight = null; + } + })(); } + return userIdInFlight; +} + +/** + * Start learning the reader's id without waiting for the answer. + * + * For callers that know they will need it later and can overlap the round-trip + * with work that does not depend on it — fetching a feed, say. Returns nothing, + * so it cannot be mistaken for the id itself. + */ +export function warmCurrentUserId(): void { + void getCurrentUserId(); +} + +/** Test seam, and what the auth listener calls when the reader changes. */ +export function __resetCurrentUserIdCache(): void { + cachedUserId = undefined; + userIdInFlight = null; } diff --git a/src/services/timeline/processors/social-shared.ts b/src/services/timeline/processors/social-shared.ts index 17c88c7c5..4139804fd 100644 --- a/src/services/timeline/processors/social-shared.ts +++ b/src/services/timeline/processors/social-shared.ts @@ -4,58 +4,15 @@ */ import supabase from '@/lib/supabase/browser'; -import { logger } from '@/utils/logger'; // TIMELINE_LIKES, TIMELINE_DISLIKES, TIMELINE_COMMENTS are not in the generated DB schema, // and custom RPCs (like/unlike/comment) are also absent — cast required. export const db = supabase as any; -/** - * Who is reading, cached for the page. - * - * `supabase.auth.getUser()` is a NETWORK call — it validates the token against - * `/auth/v1/user`. This is called once per enrichment pass, and enrichment runs - * once per node while a reply tree is built, so opening a thread fired one - * round-trip per reply just to re-learn the same id. Measured on a three-reply - * thread: eight `/auth/v1/user` calls for one page. - * - * Cached as the in-flight PROMISE, not the value, so concurrent callers — and - * enrichment is deliberately concurrent — collapse onto one request instead of - * racing to make several. Only a resolved id is kept; a failure is not cached, - * because caching "nobody is signed in" would outlive the blip that caused it - * and quietly render the whole timeline as signed-out. - * - * The id cannot change without a sign-in or sign-out, and both replace the - * page, so a page-lifetime cache is the correct scope. - */ -let cachedUserId: string | null | undefined; -let userIdInFlight: Promise | null = null; - -export async function getCurrentUserId(): Promise { - if (cachedUserId !== undefined) { - return cachedUserId; - } - if (!userIdInFlight) { - userIdInFlight = (async () => { - try { - const { - data: { user }, - } = await supabase.auth.getUser(); - cachedUserId = user?.id || null; - return cachedUserId; - } catch (error) { - logger.error('Error getting current user ID', error, 'Timeline'); - return null; - } finally { - userIdInFlight = null; - } - })(); - } - return userIdInFlight; -} - -/** Test seam, and the hook a sign-out would use if this ever needs clearing. */ -export function __resetCurrentUserIdCache(): void { - cachedUserId = undefined; - userIdInFlight = null; -} +// Who is reading is an AUTH question, not a timeline one, and there is exactly +// one answer per page. Re-exported rather than redefined: five copies of this +// function used to exist across services, each uncached, each a round-trip. +export { + getCurrentUserId, + __resetCurrentUserIdCache, +} from '@/services/supabase/auth/session'; diff --git a/src/services/timeline/queries/helpers.ts b/src/services/timeline/queries/helpers.ts index 60923791c..53a907c38 100644 --- a/src/services/timeline/queries/helpers.ts +++ b/src/services/timeline/queries/helpers.ts @@ -8,9 +8,7 @@ * Last Modified Summary: Extracted from feeds.ts */ -import supabase from '@/lib/supabase/browser'; import { ENTITY_REGISTRY } from '@/config/entity-registry'; -import { logger } from '@/utils/logger'; import type { TimelineDisplayEvent, TimelineEventDb, TimelineActorType } from '@/types/timeline'; import type { Database } from '@/types/database'; import { @@ -22,20 +20,8 @@ import { isEventRecent, } from '@/services/timeline/formatters'; -/** - * Get current user ID helper - */ -export async function getCurrentUserId(): Promise { - try { - const { - data: { user }, - } = await supabase.auth.getUser(); - return user?.id || null; - } catch (error) { - logger.error('Error getting current user ID', error, 'Timeline'); - return null; - } -} +// Defined once, in the auth layer, where it is cached. See session.ts. +export { getCurrentUserId } from '@/services/supabase/auth/session'; /** * Helper to transform enriched view events to display events diff --git a/src/services/timeline/queries/userFeeds.ts b/src/services/timeline/queries/userFeeds.ts index f2e01c24f..847d7d0ae 100644 --- a/src/services/timeline/queries/userFeeds.ts +++ b/src/services/timeline/queries/userFeeds.ts @@ -23,6 +23,7 @@ import type { } from '@/types/timeline'; import { DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE } from './constants'; import { getCurrentUserId, transformEnrichedEventToDisplay } from './helpers'; +import { warmCurrentUserId } from '@/services/supabase/auth/session'; import { getDateRangeFilter, buildDefaultFilters } from '@/services/timeline/formatters/filters'; import { enrichEventsForDisplay } from '@/services/timeline/processors/enrichment'; import { attachReactionState } from '@/services/timeline/processors/reaction-state'; @@ -40,6 +41,14 @@ export async function getUserFeed( const limit = Math.min(pagination?.limit || DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE); const offset = (page - 1) * limit; + // Enrichment needs to know who is reading, to mark which posts this user + // already reacted to. That lookup is a round-trip and it depends on nothing + // here, yet it used to start only after the feed came back — and then the + // reaction queries waited on it in turn. Measured on a cold timeline load: + // feed 2429-3061ms, THEN /auth/v1/user 3105-3351, THEN reactions 3367-3595. + // Starting it now overlaps it with the feed instead of stacking behind it. + warmCurrentUserId(); + // Build filter conditions let query = callRpc(supabase, 'get_user_timeline_feed', { @@ -62,9 +71,30 @@ export async function getUserFeed( query = query.in('visibility', filters.visibility); } + // How many posts exist in total (for pagination) is a separate question + // from what the first page contains, and neither answer needs the other. + // Asking now rather than after enrichment overlaps the two round-trips. + // The RPC resolves user→actor internally. + const countQuery = callRpc( + supabase, + 'get_user_timeline_feed', + { + p_user_id: userId, + p_limit: 0, + p_offset: 0, + }, + { count: 'exact', head: true } + ); + const { data: events, error } = await query; if (error) { + // The count is already in flight; let it settle so it cannot reject + // unhandled after we leave. + void countQuery.then( + () => undefined, + () => undefined + ); logger.error('Failed to fetch timeline feed', error, 'Timeline'); throw error; } @@ -72,18 +102,7 @@ export async function getUserFeed( // Transform to display events const displayEvents = await enrichEventsForDisplay(events || []); - // Total count: use the RPC with count option (it resolves user→actor internally) - - const { count } = await callRpc( - supabase, - 'get_user_timeline_feed', - { - p_user_id: userId, - p_limit: 0, - p_offset: 0, - }, - { count: 'exact', head: true } - ); + const { count } = await countQuery; const totalEvents = count || displayEvents.length; From 88241ae7a73c0604ed53818529c94b5726636793 Mon Sep 17 00:00:00 2001 From: Georgy Butaev <41178744+g-but@users.noreply.github.com> Date: Fri, 28 Aug 2026 23:52:06 +0200 Subject: [PATCH 2/2] perf(timeline): warm the reader id at the door every feed read goes through My previous commit warmed the reader's id inside getUserFeed. The timeline page calls getEnrichedUserFeed. So the fix shipped to production and did nothing: /auth/v1/user still started at 3552ms, after the feed returned at 3520, with the three reaction queries queued behind it. Moved to the timeline service facade, which every read goes through - including getEventById, getReplies, searchPosts and getThreadPosts, which have the same ordering problem on thread pages. One rule in one place rather than a call repeated inside each query function, and no longer dependent on me picking the right one. The test asserts the ORDER, not that the warm happens: a test that only checked "it was called" would pass with the call left at the end, which is the bug. It runs over every read the facade exposes and fails if a new one is added without warming. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012dpTLxh5GJWeWTF1UEvcD5 --- .../services/timeline-warms-reader.test.ts | 92 +++++++++++++++++++ src/services/timeline/index.ts | 44 ++++++--- src/services/timeline/queries/userFeeds.ts | 9 -- 3 files changed, 125 insertions(+), 20 deletions(-) create mode 100644 __tests__/unit/services/timeline-warms-reader.test.ts diff --git a/__tests__/unit/services/timeline-warms-reader.test.ts b/__tests__/unit/services/timeline-warms-reader.test.ts new file mode 100644 index 000000000..e5c1b4a32 --- /dev/null +++ b/__tests__/unit/services/timeline-warms-reader.test.ts @@ -0,0 +1,92 @@ +/** + * Every timeline read learns who is reading BEFORE it asks for the posts. + * + * Enrichment needs the reader's id to mark which posts they already reacted + * to, and that id is a round-trip to /auth/v1/user. It used to be requested + * only once the feed came back, with the three reaction queries then queued + * behind it — three serial waves where two could overlap. Measured on a cold + * timeline load: feed 3147-3520ms, then /auth/v1/user 3552-3784, then + * reactions 3811-4076. + * + * The ordering is the whole point, so that is what these assert. A test that + * only checked "the warm happens" would pass with the call left at the end, + * which is the bug. + * + * The first version of this fix warmed inside getUserFeed — a function the + * timeline page does not call. It went to production doing nothing. Hence the + * table below: every read the facade exposes, not the one I happened to open. + */ + +const order: string[] = []; + +const warmCurrentUserId = jest.fn(() => { + order.push('warm'); +}); + +jest.mock('@/services/supabase/auth/session', () => ({ + warmCurrentUserId: () => warmCurrentUserId(), + getCurrentUserId: jest.fn(async () => 'u1'), +})); + +const record = (name: string) => + jest.fn(async () => { + order.push(name); + return { success: true, events: [], posts: [], replies: [], pagination: {}, total: 0 }; + }); + +const queries = { + getUserFeed: record('getUserFeed'), + getProjectFeed: record('getProjectFeed'), + getProfileFeed: record('getProfileFeed'), + getFollowedUsersFeed: record('getFollowedUsersFeed'), + getCommunityFeed: record('getCommunityFeed'), + getEnrichedUserFeed: record('getEnrichedUserFeed'), + getEnrichedFollowingFeed: record('getEnrichedFollowingFeed'), + getEventById: record('getEventById'), + getReplies: record('getReplies'), + searchPosts: record('searchPosts'), + getThreadPosts: record('getThreadPosts'), +}; + +jest.mock('@/services/timeline/queries', () => queries); + +import { timelineService } from '@/services/timeline'; + +/** Each read, and how the facade exposes it. */ +const READS: Array<[keyof typeof queries, () => Promise]> = [ + ['getUserFeed', () => timelineService.getUserFeed('u1')], + ['getProjectFeed', () => timelineService.getProjectFeed('p1')], + ['getProfileFeed', () => timelineService.getProfileFeed('pr1')], + ['getFollowedUsersFeed', () => timelineService.getFollowedUsersFeed('u1')], + ['getCommunityFeed', () => timelineService.getCommunityFeed()], + ['getEnrichedUserFeed', () => timelineService.getEnrichedUserFeed('u1')], + ['getEnrichedFollowingFeed', () => timelineService.getEnrichedFollowingFeed('u1')], + ['getEventById', () => timelineService.getEventById('e1')], + ['getReplies', () => timelineService.getReplies('e1')], + ['searchPosts', () => timelineService.searchPosts('cat')], + ['getThreadPosts', () => timelineService.getThreadPosts('t1')], +]; + +describe('timeline reads warm the reader id first', () => { + beforeEach(() => { + order.length = 0; + warmCurrentUserId.mockClear(); + }); + + it.each(READS)('%s warms before it queries', async (name, call) => { + await call(); + + expect(order).toEqual(['warm', name]); + }); + + it('covers every read the facade exposes', () => { + // If a new read is added to the service without warming, this catches it + // even when nobody thinks to add a case above. + const exposed = Object.getOwnPropertyNames( + Object.getPrototypeOf(timelineService) + ).filter(m => /^(get|search)/.test(m) && m !== 'getEventCounts' && m !== 'getEventComments' && m !== 'getCommentReplies'); + const covered = new Set(READS.map(([name]) => name as string)); + + expect([...exposed].filter(m => !covered.has(m))).toEqual([]); + }); +}); diff --git a/src/services/timeline/index.ts b/src/services/timeline/index.ts index 7c0706b4b..b161a7df2 100644 --- a/src/services/timeline/index.ts +++ b/src/services/timeline/index.ts @@ -39,6 +39,26 @@ import { getThreadPosts, } from './queries'; +import { warmCurrentUserId } from '@/services/supabase/auth/session'; + +/** + * Start learning who is reading before the feed query, not after it. + * + * Every feed read below ends in enrichment, and enrichment needs the reader's + * id to mark which posts they already reacted to. That lookup is a round-trip + * to /auth/v1/user, it depends on nothing in the feed, and it used to start + * only once the feed came back — with the reaction queries then queued behind + * it. Measured on a cold timeline load: feed 3147-3520ms, THEN /auth/v1/user + * 3552-3784, THEN reactions 3811-4076. + * + * Applied here, at the one door every feed read goes through, rather than + * repeated inside each query function. + */ +function withWarmReader(run: () => Promise): Promise { + warmCurrentUserId(); + return run(); +} + // Import mutation functions import { createEventWithVisibility, @@ -150,7 +170,7 @@ class TimelineService { filters?: Partial, pagination?: Partial ): Promise { - return getUserFeed(userId, filters, pagination); + return withWarmReader(() => getUserFeed(userId, filters, pagination)); } /** @@ -161,7 +181,7 @@ class TimelineService { filters?: Partial, pagination?: Partial ): Promise { - return getProjectFeed(projectId, filters, pagination); + return withWarmReader(() => getProjectFeed(projectId, filters, pagination)); } /** @@ -172,7 +192,7 @@ class TimelineService { filters?: Partial, pagination?: Partial ): Promise { - return getProfileFeed(profileId, filters, pagination); + return withWarmReader(() => getProfileFeed(profileId, filters, pagination)); } /** @@ -183,7 +203,7 @@ class TimelineService { pagination?: Partial ): Promise { // Note: getFollowedUsersFeed gets currentUserId internally - return getFollowedUsersFeed(undefined, pagination); + return withWarmReader(() => getFollowedUsersFeed(undefined, pagination)); } /** @@ -194,7 +214,7 @@ class TimelineService { filters?: Partial, pagination?: Partial ): Promise { - return getCommunityFeed(filters, pagination); + return withWarmReader(() => getCommunityFeed(filters, pagination)); } /** @@ -205,7 +225,9 @@ class TimelineService { filters?: Partial, pagination?: Partial ): Promise { - return getEnrichedUserFeed(userId, filters, pagination, getDemoTimelineEvents); + return withWarmReader(() => + getEnrichedUserFeed(userId, filters, pagination, getDemoTimelineEvents) + ); } /** Home feed — posts from people/projects the user follows (+ own), no public firehose. */ @@ -214,7 +236,7 @@ class TimelineService { filters?: Partial, pagination?: Partial ): Promise { - return getEnrichedFollowingFeed(userId, filters, pagination); + return withWarmReader(() => getEnrichedFollowingFeed(userId, filters, pagination)); } /** @@ -223,7 +245,7 @@ class TimelineService { async getEventById( eventId: string ): Promise<{ success: boolean; event?: TimelineDisplayEvent; error?: string }> { - return getEventById(eventId); + return withWarmReader(() => getEventById(eventId)); } /** @@ -234,7 +256,7 @@ class TimelineService { eventId: string, limit: number = 50 ): Promise<{ success: boolean; replies?: TimelineDisplayEvent[]; error?: string }> { - return getReplies(eventId, limit); + return withWarmReader(() => getReplies(eventId, limit)); } /** @@ -253,14 +275,14 @@ class TimelineService { total?: number; error?: string; }> { - return searchPosts(query, options); + return withWarmReader(() => searchPosts(query, options)); } /** * Get all posts in a thread */ async getThreadPosts(threadId: string): Promise { - const result = await getThreadPosts(threadId); + const result = await withWarmReader(() => getThreadPosts(threadId)); return { success: result.success, posts: result.posts || [], diff --git a/src/services/timeline/queries/userFeeds.ts b/src/services/timeline/queries/userFeeds.ts index 847d7d0ae..3805e6c2a 100644 --- a/src/services/timeline/queries/userFeeds.ts +++ b/src/services/timeline/queries/userFeeds.ts @@ -23,7 +23,6 @@ import type { } from '@/types/timeline'; import { DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE } from './constants'; import { getCurrentUserId, transformEnrichedEventToDisplay } from './helpers'; -import { warmCurrentUserId } from '@/services/supabase/auth/session'; import { getDateRangeFilter, buildDefaultFilters } from '@/services/timeline/formatters/filters'; import { enrichEventsForDisplay } from '@/services/timeline/processors/enrichment'; import { attachReactionState } from '@/services/timeline/processors/reaction-state'; @@ -41,14 +40,6 @@ export async function getUserFeed( const limit = Math.min(pagination?.limit || DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE); const offset = (page - 1) * limit; - // Enrichment needs to know who is reading, to mark which posts this user - // already reacted to. That lookup is a round-trip and it depends on nothing - // here, yet it used to start only after the feed came back — and then the - // reaction queries waited on it in turn. Measured on a cold timeline load: - // feed 2429-3061ms, THEN /auth/v1/user 3105-3351, THEN reactions 3367-3595. - // Starting it now overlaps it with the feed instead of stacking behind it. - warmCurrentUserId(); - // Build filter conditions let query = callRpc(supabase, 'get_user_timeline_feed', {