perf(timeline): a thread cost a round-trip per reply, and six copies of "who is reading" - #824
Merged
Merged
Conversation
getReplies recursed: one query AND one enrichEventsForDisplay per node. Enrichment is several requests — profiles, projects, the reader's id, the three reaction tables — so a thread cost roughly six round-trips per reply. I introduced the reader's-id and reaction queries in #811 without noticing they sat inside a per-node recursion. Measured in production by opening one three-reply post: eight /auth/v1/user calls alone. Two changes, both structural: - getReplies fetches the tree a LEVEL at a time (one query per depth, regardless of width) and enriches the whole flat list ONCE, then assembles parent -> children from a map. - getCurrentUserId caches the in-flight PROMISE, so concurrent callers collapse onto one /auth/v1/user instead of racing to make several. A failure is never cached - caching "nobody is signed in" would outlive the blip and render the timeline as signed-out. The tests assert the COST, not just the tree. The recursive version built a perfectly correct tree, which is exactly why nothing caught it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dpTLxh5GJWeWTF1UEvcD5
The first version of this mock returned rows by a level counter, ignoring what was asked for. That made "one query per level" and "one query per node" indistinguishable: a mutation swapping .in(parents) for .eq(parents[0]) stayed green, so the cost assertion was decorative. The mock now filters on parent_event_id, and a sibling-branch case pins that both branches are fetched in one query AND that neither loses its children. Both mutations are red against it and green after restore. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dpTLxh5GJWeWTF1UEvcD5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two related performance defects on the timeline, both structural, both mine.
1. Opening a thread cost a round-trip per reply
getRepliesrecursed: one query AND oneenrichEventsForDisplayper node. Enrichment is itself several requests — profiles, projects, the reader's id, and the three reaction tables — so a thread cost roughly six round-trips per reply.This is my regression from #811: I added the reader's-id lookup and the reaction-state queries to enrichment without noticing enrichment sat inside a per-node recursion.
Measured in production by opening one three-reply post: eight
/auth/v1/usercalls for a single page.Fix:
getReplieswalks the tree a level at a time — one query per depth regardless of width — enriches the whole flat list once, then assembles parent→children from a map. A reply whose parent vanished mid-read simply does not attach, instead of orphaning its branch.2.
getCurrentUserIdwas defined six times, each uncachedAcross timeline queries, timeline processors, groups, loans, projects, and the auth layer. Every copy calls
supabase.auth.getUser(), which is a network call — it validates the token against/auth/v1/user. So one page asked the server who the reader was over and over, and the cache in fix 1 covered only four of the six importers.On the timeline that lookup also sat on the critical path. Cold load, measured:
The identity lookup ran after the feed returned, and the reaction queries then waited on it — though it depends on neither.
Fix:
getUserFeedwarms the id alongside the feed request instead of after it, and asks for the total count concurrently rather than after enrichment.check:one-current-userfails the build if a seventh copy appears. Wired intoverify.A test caught a real behaviour change
getUser()catches its own errors and reports them inerrorrather than throwing — so "could not ask" arrived looking almost exactly 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.Why the tests assert cost, not correctness
The recursive version built a perfectly correct tree. That is exactly why nothing caught it. So the tests pin the number of queries and enrichment passes.
The first version of the reply-tree mock returned rows by a level counter, ignoring what was asked for — which made "one query per level" and "one query per node" indistinguishable. Caught by mutation: swapping
.in(parents)for.eq(parents[0])stayed green. The mock now filters onparent_event_id.Mutation-proved — each red, then green after restore:
getCurrentUserIddefinitionThe gate was also confirmed reachable through
npm run, not only when run by hand.Full unit suite: 261 suites, 2524 passed.
Not fixed, deliberately
/api/ratesandprojectseach appear twice in the waterfall. I checked both: the second/api/ratesis the browser's ownstale-while-revalidaterevalidation (cache-control: public, max-age=30, stale-while-revalidate=300), not our code. Not a defect, so not touched.