perf: cut the client-side RPC floods, and repair four desktop e2e tests - #29466
Open
chrisnojima wants to merge 7 commits into
Open
perf: cut the client-side RPC floods, and repair four desktop e2e tests#29466chrisnojima wants to merge 7 commits into
chrisnojima wants to merge 7 commits into
Conversation
The Go service log is the only place the client's real network behaviour is visible - the JS side shows intent, the log shows what actually went out - but a flood rotates the default log away in under two minutes, so reading it starts with capturing it properly. scripts/ covers both halves: clean-logs.sh and start-service.sh capture a run against a service built from the checkout, and rpc-report.py, rpc-cost.py, rpc-why.py and rpc-diff.py read it by volume, by time, by attribution and against a previous run. SKILL.md is mostly the mistakes. Rank by cost before count - the loudest line in this log was 15,767 calls worth 0.16s while the real expense sat 30th - and the default cost threshold hides exactly the cheap chatty rows you need to prove a flood is harmless. CLAUDE.md: the base-branch rule said "previous commit" where it meant the base branch, and validation now requires lint:all, since plain lint is eslint only and does not catch react-compiler bailouts.
Every consumer of a team's channels held its own cache, so the stale window and the single-flight lived on a per-instance object and none of them could see each other's in-flight request. Each issued its own getTLFConversationsLocal, which localizes every channel in the team: measured at 7 calls for one team inside 1.5s, and on a big team that is enough to trip the server's chat rate limit. The channel list, the all-channel-metas set, the loaded team and the #general lookup now share one module-level cache each. That exposes two things a per-instance cache hid, both fixed here: Channel create and delete fire no teamChangedByID, and consumers used to refetch by remounting. Sharing a cache means a remount inside the stale window serves the pre-change list, so those screens drop the entry explicitly through registerTeamChannelsInvalidator. A disabled "shadow" instance - one returning the context value rather than loading - resets its cache to loadedAt 0, which with a shared map clobbers the real loader's data. Shadows get a private throwaway map via forceLocalCache. useCachedResource also looped on unstable initialData: a fresh-cache hit built a new state object, which re-rendered, which re-ran the effect. And a load whose result never landed left loadedAt at 0, making the resource permanently stale so every effect run reissued the RPC the instant the previous settled. Both are covered by tests that hang against the pre-fix code. The settings tab was keyed on server data that arrives in stages, so the subtree remounted as each piece landed - 13 GetTeamRetentionLocal in one second.
Opening a profile identifies the user once per surface that happens to be mounted - the profile screen, the tracker popup, the hover card - and each identify is hundreds of milliseconds and several HTTP calls. identify-session holds a single session for a profile view and every surface reads from it, so the work happens once and the later surfaces get the result rather than repeating it. Two related wastes on the same screen: a profile re-identified on every mount even when it had just been checked, and the teams list annotated every featured team up front to fill a hover popup that usually never opens. The annotation is now loadOnDemand - the single biggest win in this pass.
userEmojis resolves two attachment URLs per emoji, and the suggestor refetched the whole set every time a ":" was typed: 16,512 GetURL calls in one run on an account with ~950 custom emoji. The set is now cached across consumers and skipped entirely on a prefetch, which takes the call count to a handful. The inbox search indexing affordance was gated on indexPercent > 0, which is false at exactly 0% - the state a freshly rebuilt or brand new index is in. Inbox-wide search has no fallback while the index is empty, so this rendered an empty result list with nothing to explain it. indexPercent is now undefined until the service reports one, so 0 renders "Indexing..." like any other incomplete value, and it resets per search instead of carrying the previous query's number.
All four were genuine test bugs, each confirmed from the Playwright trace rather than guessed. crypto recipients: the "Search people" input sits inside a pointerEvents="none" wrapper, so the click could never land - the trace showed clickable-box2 intercepting pointer events until timeout. Added a testID on the wrapping ClickableBox. bot install preview: two bugs. The row centre lands on the "by <owner>" username link, so the click opened that user's profile - a different profile on each run, which is what gave it away. And the label regex could never match a bot already installed unrestricted in the conversation, whose footer renders only "Uninstall". Now clicks the avatar column and asserts on a modal testID. teams members tab: lastSelectedTabs remembers the tab per team for the app's lifetime, so an earlier test left it on Channels. Selects Members explicitly. retention warning: reopens the dropdown when the popup is swallowed while the settings tab settles. Per-test timeout 15s -> 30s; several flows chain three 5s waits and could not fit inside the old budget.
Sixteen module-level caches held user data across sign-out - team member lists, the team list and role map, inbox search results, attachment handoffs, profile and tab selections, and revealed markdown spoilers. Module scope outlives the session, so the next user could be served the previous one's data. That is a privacy bug rather than a perf one. All now clear through registerExternalResetter. Two subscription bugs on the same sweep. An inline useSafeFocusEffect callback re-subscribed on every render, so one focus fired 39 gregor dismissCategory calls. And a stale engine unsubscribe could drop a live listener that had replaced it, covered now by a test. Also returns react-compiler bailouts to zero, which the whole-props closure dependency in a few of these hooks had broken.
A mutation and the reload it triggers routinely land in the same millisecond, so comparing the in-flight request's start timestamp against the forced load's request timestamp read them as equal and the forced load joined the very request it exists to supersede: pre-change data, stamped with loadedAt, pinned for the whole stale window. Order the two with a monotonic counter instead, and skip the invalidate entirely when nothing is in flight. The test suite's flush() awaited only microtasks, so React work scheduled outside act() - which goes through its MessageChannel, a macrotask - was never reached and the forced-reload test committed or not depending on runtime interleaving. Flush with a real timer.
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.
Base of a two-PR stack. This one is the JS side plus tooling; the Go service changes sit on top in a follow-up PR. They are independent — nothing in
protocol/changed, so there is no generated-type coupling, and neither side's build reads the other's tree.Started as four failing desktop e2e tests. The rate-limit error they surfaced turned out to be real client bugs, so this grew.
The e2e failures (4 tests)
All four were genuine test bugs, each confirmed from the Playwright trace rather than guessed:
Search peopleinput sits inside apointerEvents="none"wrapper, so the click could never land; the trace showedclickable-box2 ... intercepts pointer eventslooping until timeout. Added a testID on the wrappingClickableBox.^(Install|Edit settings|Review)$can never match a bot already installed unrestricted in the conversation — that footer renders only "Uninstall". Now clicks the avatar column and asserts on a modal testID.lastSelectedTabsremembers the tab per team for the app's lifetime, so an earlier test left it on Channels. Selects Members explicitly.Per-test timeout 15s → 30s; several flows chain three 5s waits and could not fit.
The floods
Analysis of the Go service log during a run — it rotated 128MB twice in under two minutes. The client-side half of each is fixed here; the service-side half is in the stacked PR.
RefreshParticipantsAttachmentHTTPSrv: GetURLuserEmojisresolves 2 attachment URLs per emoji, and the suggestor refetched the whole set on each:getAnnotatedTeamuseCachedResourceGetTeamRetentionLocalgregor.dismissCategoryuseSafeFocusEffectcallback re-subscribing every rendergetAnnotatedTeamwas the subtle one, two defects composing: a fresh-cache hit built a new state object and forced a re-render that re-ran the effect that called it; and a load whose result never landed leftloadedAtat 0, making the resource permanently stale so every effect run reissued the RPC the instant the previous settled. Both are now covered by tests that hang the pre-fix code.Sharing one cache per team (rather than one per consumer) exposed two things the per-instance caches had hidden, both fixed here: channel create/delete fire no
teamChangedByIDand so now drop the entry explicitly, and a disabled "shadow" hook instance would reset the shared cache out from under the real loader, so shadows get a private map viaforceLocalCache.Sign-out
Separately, 16 module-level caches held user data across sign-out — team member lists, the team list and role map, inbox search results, attachment handoffs, profile and tab selections, and revealed markdown spoilers, which is a privacy bug rather than a perf one. All now clear via
registerExternalResetter.Tooling
A
keybase-rpc-log-analysisskill: scripts to capture a clean service log against a locally built service, and to read it by volume, by cost, by attribution, and against a previous run. The write-up is mostly the mistakes — rank by cost before count (the loudest line in this log was 15,767 calls worth 0.16s, while the real expense sat 30th by count).Verification
yarn lint:all(eslint + react-compiler bailouts + tsc) passes with 0 bailouts, and the full jest suite is green (79 suites / 360 tests). The desktop e2e suite was run against a service built from this checkout: 192 passed; the only failures are the twofiles-*tests, which need a kbfs mount the capture setup does not provide.