From bcb7f844af0e044feaf3c562adc9a1d688024112 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Sun, 26 Jul 2026 15:00:31 -0400 Subject: [PATCH 1/7] skill: RPC log analysis for the keybase service 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. --- CLAUDE.md | 4 +- skill/keybase-rpc-log-analysis/SKILL.md | 145 ++++++++++++++++++ .../scripts/clean-logs.sh | 45 ++++++ .../keybase-rpc-log-analysis/scripts/kblog.py | 87 +++++++++++ .../scripts/rpc-cost.py | 101 ++++++++++++ .../scripts/rpc-diff.py | 64 ++++++++ .../scripts/rpc-report.py | 116 ++++++++++++++ .../scripts/rpc-why.py | 137 +++++++++++++++++ .../scripts/start-service.sh | 84 ++++++++++ 9 files changed, 781 insertions(+), 2 deletions(-) create mode 100644 skill/keybase-rpc-log-analysis/SKILL.md create mode 100755 skill/keybase-rpc-log-analysis/scripts/clean-logs.sh create mode 100755 skill/keybase-rpc-log-analysis/scripts/kblog.py create mode 100755 skill/keybase-rpc-log-analysis/scripts/rpc-cost.py create mode 100755 skill/keybase-rpc-log-analysis/scripts/rpc-diff.py create mode 100755 skill/keybase-rpc-log-analysis/scripts/rpc-report.py create mode 100755 skill/keybase-rpc-log-analysis/scripts/rpc-why.py create mode 100755 skill/keybase-rpc-log-analysis/scripts/start-service.sh diff --git a/CLAUDE.md b/CLAUDE.md index b68b2bb4ba95..8cdb98e6d4ab 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2,7 +2,7 @@ - No `Co-Authored-By` in commits. Ever. - Never interact with the Electron app or iOS simulator (screenshots, driving UI, debug ports) without asking first. The user drives and takes screenshots. - Use `--no-ext-diff` with `git diff` (and `git show`/`git log -p`) so external diff tools don't hijack output. -- "Was working before" = base branch, not previous commit. Base branch is almost always `nojima/HOTPOT-next-670-clean-2` (not `master`). Always run `gh pr view --json baseRefName` to confirm before any `git diff` or `git log` comparison. +- "Was working before" = base branch, not previous commit. Base is normally `master`. Always run `gh pr view --json baseRefName` to confirm before any `git diff` or `git log` comparison. - Never use `npm`. Always `yarn`. - Never silently drop features/behavior — ask first, present options. - In tests/stories, use `testuser` / `testuser-mac` as placeholder usernames — never real usernames like `chrisnojima`. @@ -23,4 +23,4 @@ Repo root is `client/`. TS source lives in `shared/`. Always use absolute paths - Plans created by superpowers skills go into `plans/` at the repo root. ## Validation -After TS changes (from `shared/`): `yarn lint` then `yarn tsc`. When debugging visually, skip until fix is confirmed. Never delete the ESLint cache. +After TS changes (from `shared/`): `yarn lint:all` (= `yarn lint` && `yarn lint:bailouts` && `yarn tsc`). Plain `yarn lint` is eslint only and does NOT catch react-compiler bailouts — no compiler rule is wired into `eslint.config.mjs`, so bailouts only surface via `lint:bailouts`. Repo baseline is 0 bailouts; keep it there. When debugging visually, skip until fix is confirmed. Never delete the ESLint cache. diff --git a/skill/keybase-rpc-log-analysis/SKILL.md b/skill/keybase-rpc-log-analysis/SKILL.md new file mode 100644 index 000000000000..6b1864801fbf --- /dev/null +++ b/skill/keybase-rpc-log-analysis/SKILL.md @@ -0,0 +1,145 @@ +--- +name: keybase-rpc-log-analysis +description: Use when hunting redundant, duplicated or looping RPCs in the Keybase client — after a rate-limit error, a service log that rotates every few minutes, a slow or hot app, or to prove a caching fix actually reduced calls. Covers capturing a clean service log against a locally built service and reading it. +--- + +# Keybase RPC log analysis + +The Go service logs every RPC it serves and every call it makes. That 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. A quiet minute is ~45 lines; a +minute with a flood is 120,000. + +## Capture a clean run + +```bash +skill/keybase-rpc-log-analysis/scripts/clean-logs.sh --archive # dry run without a flag +skill/keybase-rpc-log-analysis/scripts/start-service.sh # own terminal, foreground +cd shared && yarn desktop:start:hot:e2e # another terminal +cd shared && yarn test:e2e:desktop # or drive the app by hand +``` + +`start-service.sh` builds from this checkout, stops the running service, and logs +with `-d --log-file` to `/tmp/kb-analysis/service.log`. Its own file matters: the +default log is shared with everything else and a flood rotates the evidence away +in under two minutes. + +`--log-file` **still rotates at 128MB** — a full e2e run produces ~220MB. Nothing +is lost, but the analysis must include the siblings, so always pass them all: + +```bash +python3 skill/keybase-rpc-log-analysis/scripts/rpc-report.py \ + /tmp/kb-analysis/service.log-* /tmp/kb-analysis/service.log +``` + +The service forks kbfs from its own bin directory, and a plain `go install` of +the service does not build it. Without kbfs the Files tab is empty and git +fails with `KBFS client not found`, which fails every `files-*` and `git-*` e2e +test for reasons unrelated to the change under test. `start-service.sh` warns. + +Ask before stopping the service or launching the app — both are the user's. + +## Read it + +| Question | Command | +|---|---| +| What did this run do? | `rpc-report.py [--start 2026-07-24T17:23] [--end ...]` | +| What was actually slow? | `rpc-cost.py [--min-mean-ms 50]` | +| Why is X called so much? | `rpc-why.py --match 'AttachmentHTTPSrv: GetURL'` | +| Did my fix work? | `rpc-diff.py before.log after.log` | + +**Run `rpc-cost.py` early.** Count and cost answer different questions, and the +loudest line in the log is usually not the expensive one. In one capture the top +line by count was an in-memory map lookup — 15,767 calls for 0.16s total, not +worth touching — while the real cost was 469 team loads at 292ms each, 186 +seconds, sitting 30th by count. + +To *prove* a loud flood is free rather than just absent from the table, pass +`--min-mean-ms 0 --top 400`: the default threshold hides cheap chatty ops, which +is exactly the row you need. `AttachmentHTTPSrv: GetURL` at 4,848 calls does not +appear at all by default, and shows as 0.04s total once it does. + +Analysing an already-rotated log is the same — pass +`~/Library/Logs/keybase.service.log--` directly. The filename carries +its own window, so `--start/--end` are only for narrowing further. + +Read `rpc-report.py`'s sections in order: + +- **REMOTE** — service→server. Only these burn server rate limits. Start here. +- **APP** — app→service. One of these usually explains a REMOTE row. +- **BURSTS** — same call and subject, 3+ times in one second. Each row is a + missing cache or a remounting component. +- **FLOODS** — most repeated lines, ids and numbers collapsed. + +A BURSTS row with a bare name and no `(subject)` means the subject could not be +resolved: many RPCs log no arguments. Those rows are same-*name* only — treat +them as a lead, not as proof the same conversation was hit twice. + +`rpc-why.py` answers attribution. **ROOT** (the app RPC that opened the trace) is +usually the answer; **DRIVERS** (nearest preceding call) points at the specific +line. **BATCHES** needs the span that logs the item count, which is normally the +local one, not the remote it wraps — match `HybridConversationSource: GetMessages`, +not `getMessagesRemote`. The script says so when it finds no counts. + +## What a bug looks like + +- **Same args, same second, N times** → no dedupe, or a keyed component remounting + as its props arrive in stages. +- **Fires again the instant the previous lands** → a feedback loop, not user + action. Look for a cache that never records success, or an effect whose deps + change on every render. +- **Batch sizes almost all 1** → the caller loops where it could fetch once. +- **A per-item cost inside a loop** → e.g. a gregor state read per emoji. + +## Mistakes + +- **Guessing the trigger from neighbouring lines.** A burst surrounded by + startup-looking work is not necessarily startup. Correlate against something + independent: the mtimes of `shared/tests/results/test-results/*/test-finished-1.png` + give you a timestamped list of which test ran when, and `app.log` records + renderer reloads. One burst-per-profile-open and one burst-per-bootstrap look + identical in the service log until you check. +- **Attributing to the child call.** The line directly above a flood is often + something the flood itself invoked. Trust ROOT over DRIVERS. +- **Reading a fix as failed because the totals did not move.** If the app's + request volume changed between runs, a working cache can show flat server + counts. Count what the cache actually did — a hit rate, a log line — before + concluding anything. One cache here looked useless in one capture and turned + out to be a 79% reduction once the volume feeding it was fixed. +- **Fixing what you have not measured.** Two of the loudest floods in this + log — 15,767 username lookups and 7,284 chain-storage misses — were worth + 0.16s and 0.24s. Both were correctly left alone. `rpc-cost.py` first. +- **Blaming the biggest number.** Service-side background work — the search + indexer especially — can dwarf the real bug. An empty ROOT means nothing asked + for it. Check whether it recurs on a timer before spending time on it. +- **Reporting a burst as same-subject when it is not.** See the BURSTS caveat + above. If it matters, prove the subject repeats before claiming it. +- **Using plain `grep`.** It is wrapped in this environment and truncates. Use + python, as the scripts do. +- **Comparing unlike runs.** `rpc-diff.py` is only meaningful if both runs did + the same thing. + +## Shapes seen before + +Each of these was a real bug, and each is what its section looks like: + +- An app RPC in the thousands where the screen has tens of items — a per-item + prime re-run on every list reload. +- A local call in the low single digits dragging thousands of lines behind it — a + per-item cost inside a server-side loop. +- FLOODS dominated by one span whose BATCHES are almost all size 1. +- An APP row whose count is a multiple of the number of times a screen was + opened — a component keyed on data that arrives in stages, remounting. +- **A list row eagerly loading what only its hover popup needs.** The single + biggest win here was one `loadOnDemand` flag: a profile rendered a row per + team and each row annotated its team up front, for a popup nobody opened. +- **A hook that subscribes to a broadcast per component instance.** One + notification then does N times the work, and if the work itself emits that + notification it compounds. Subscribe once at module scope and fan out. +- **The same expensive load repeated because each surface holds its own copy.** + Two or three components asking for the same user or team at the same instant + is the common case, not the rare one. + +Where the money actually is in this client: identify/proof checks and team +loads. Both are hundreds of milliseconds each and both fan out to several HTTP +calls. Chat localization is cheap per call but runs over every channel. diff --git a/skill/keybase-rpc-log-analysis/scripts/clean-logs.sh b/skill/keybase-rpc-log-analysis/scripts/clean-logs.sh new file mode 100755 index 000000000000..47a30b7f2450 --- /dev/null +++ b/skill/keybase-rpc-log-analysis/scripts/clean-logs.sh @@ -0,0 +1,45 @@ +#!/bin/bash +# Clear keybase logs so the next run is the only thing in them. +# +# ./clean-logs.sh # list what would move, change nothing +# ./clean-logs.sh --archive # move them to ~/Library/Logs/keybase-archive-/ +# ./clean-logs.sh --delete # remove them +# +# Default is a dry run: an unattended --delete on the wrong machine is not +# recoverable, and the logs are often the only record of a run. +set -euo pipefail + +LOGDIR="$HOME/Library/Logs" +MODE="${1:-}" + +shopt -s nullglob +FILES=("$LOGDIR"/keybase*.log "$LOGDIR"/keybase*.log-* "$LOGDIR"/Keybase.app.log) +shopt -u nullglob + +if [ ${#FILES[@]} -eq 0 ]; then + echo "no keybase logs in $LOGDIR" + exit 0 +fi + +TOTAL=$(du -ch "${FILES[@]}" 2>/dev/null | tail -1 | cut -f1) +echo "${#FILES[@]} files, $TOTAL:" +for f in "${FILES[@]}"; do + printf ' %8s %s\n' "$(du -h "$f" | cut -f1)" "$(basename "$f")" +done + +case "$MODE" in + --archive) + DEST="$LOGDIR/keybase-archive-$(date +%Y%m%dT%H%M%S)" + mkdir -p "$DEST" + mv "${FILES[@]}" "$DEST/" + echo "moved to $DEST" + ;; + --delete) + rm -f "${FILES[@]}" + echo "deleted" + ;; + *) + echo + echo "dry run. pass --archive to move them aside, or --delete to remove them." + ;; +esac diff --git a/skill/keybase-rpc-log-analysis/scripts/kblog.py b/skill/keybase-rpc-log-analysis/scripts/kblog.py new file mode 100755 index 000000000000..5afb98f20423 --- /dev/null +++ b/skill/keybase-rpc-log-analysis/scripts/kblog.py @@ -0,0 +1,87 @@ +"""Shared parsing for keybase service logs. + +Line shape: + 2026-07-24T17:19:03.576384-04:00 ▶ [DEBU keybase utils.go:537] 286366 ++Chat: | Body [tags:k=v,...] +""" + +import re + +TS_LEN = 19 # 2026-07-24T17:19:03 +BODY_RE = re.compile(r"\]\s+\w+\s+(.*)") +TAGS_RE = re.compile(r"\[tags:.*") +TIME_RE = re.compile(r"\[time=[^\]]*\]") +HEXID_RE = re.compile(r"[0-9a-f]{16,}") +# an id explicitly labelled as a conversation +CONVID_RE = re.compile(r"conv(?:ID)?[:= (]+([0-9a-f]{16,})", re.I) +# the logged-in user's id, which appears on a great many lines. It has to be +# excluded when guessing what a call was about, or every call looks like it +# shares a subject. +UID_RE = re.compile(r"uid:\s*([0-9a-f]{16,})") + + +def subject_of(line, uid=None): + """Best guess at what a line is about: a labelled conv id, else the first id + that is not the logged-in user.""" + stripped = TAGS_RE.sub("", line) + m = CONVID_RE.search(stripped) + if m: + return m.group(1) + for candidate in HEXID_RE.findall(stripped): + if candidate != uid: + return candidate + return None +NUM_RE = re.compile(r"\b\d+\b") +TRACE_RE = re.compile(r"chat-trace=(\w+)") + +# app -> service: a local RPC the client asked for +SERVER_RE = re.compile(r"\+ Server: (\w+)(\([^)]*\))?") +# service -> keybase servers: these are what burn the server-side rate limits +REMOTE_RE = re.compile(r"\+ RemoteClient: ([\w.]+)") +# raw HTTP to the API host +HTTP_RE = re.compile(r"(GET|POST) (https?://[^\s?]+)") + + +def body(line): + m = BODY_RE.search(line) + return (m.group(1) if m else line).rstrip() + + +def normalize(line, width=95): + """Collapse ids, numbers and trailing metadata so repeated lines group.""" + s = body(line) + s = TAGS_RE.sub("", s) + s = TIME_RE.sub("", s) + s = HEXID_RE.sub("", s) + s = NUM_RE.sub("", s) + return s.strip()[:width] + + +def stamp(line, precision=19): + """precision: 19=second, 16=minute, 13=hour.""" + return line[:precision] if len(line) >= precision else None + + +def iter_lines(paths, start=None, end=None): + """Yield lines from each path, optionally filtered to a timestamp window. + + start/end are prefixes, e.g. '2026-07-24T17:23'. + """ + for path in paths: + with open(path, errors="ignore") as f: + for line in f: + if start or end: + ts = line[:TS_LEN] + if not ts.startswith("2"): + continue + if start and ts < start: + continue + # end is a prefix, so compare only that many chars: a bare + # '...T19:12' must include all of 19:12, not exclude it + if end and ts[: len(end)] > end: + continue + yield line + + +def trace(line): + m = TRACE_RE.search(line) + return m.group(1) if m else None diff --git a/skill/keybase-rpc-log-analysis/scripts/rpc-cost.py b/skill/keybase-rpc-log-analysis/scripts/rpc-cost.py new file mode 100755 index 000000000000..84a56100009d --- /dev/null +++ b/skill/keybase-rpc-log-analysis/scripts/rpc-cost.py @@ -0,0 +1,101 @@ +#!/usr/bin/env python3 +"""Rank operations by the time they actually took, not by how often they appear. + + python3 rpc-cost.py ... [--top 20] [--min-mean-ms 0] + +Count and cost are different questions, and the loudest thing in the log is +usually not the expensive one. In one capture the top line by count was an +in-memory map lookup running 15,767 times for 0.16s total, while the real cost +was 469 team loads at 292ms each - 186 seconds, and only the 30th most common +line. + +Reads the [time=...] that the Go tracing helpers put on every span exit, so it +only sees operations that are traced. Times overlap across goroutines, so the +totals are service time, not wall clock. +""" + +import argparse +import collections +import re + +import kblog + +# "- Namespace: Op(args) -> ok [time=1.234ms]", optionally behind a "++Chat: " +# prefix. Keep the whole "Namespace: Op": stopping the name at the first colon +# collapsed every "Server: X" and "RemoteClient: X" into one row, which hid the +# per-operation breakdown this script exists to produce. A colon only ends the +# name when it is not followed by a space. +EXIT_RE = re.compile( + r"\]\s+\w+\s+(?:\+\+\w+:\s+)?-\s+((?:[^\s(:]|:(?= )|(?<=:) )+?)\s*(?:\(| ->).*\[time=([^\]]+)\]" +) +# Go durations are compound once they pass a minute ("1m28.867s", "1h2m3s"). +# Matching only a single number+unit silently dropped exactly the slowest spans. +DUR_RE = re.compile(r"(?:([\d.]+)h)?(?:([\d.]+)m(?!s))?(?:([\d.]+)(ms|s|µs|ns))?$") +UNIT_MS = {"ms": 1.0, "s": 1000.0, "µs": 0.001, "ns": 1e-6} + + +def parse_duration_ms(text): + """Return milliseconds for a Go duration string, or None if unparseable.""" + m = DUR_RE.match(text.strip()) + if not m: + return None + hours, minutes, value, unit = m.groups() + total = 0.0 + if hours: + total += float(hours) * 3_600_000 + if minutes: + total += float(minutes) * 60_000 + if value: + total += float(value) * UNIT_MS[unit] + return total if (hours or minutes or value) else None + + +def main(): + ap = argparse.ArgumentParser() + ap.add_argument("logs", nargs="+") + ap.add_argument("--start") + ap.add_argument("--end") + ap.add_argument("--top", type=int, default=20) + ap.add_argument("--min-mean-ms", type=float, default=0.0, help="hide cheap chatty ops") + args = ap.parse_args() + + total = collections.Counter() + calls = collections.Counter() + worst = {} + + for line in kblog.iter_lines(args.logs, args.start, args.end): + m = EXIT_RE.search(line) + if not m: + continue + # "Namespace: Op" is the useful grouping; anything past that is an + # argument ("localizerPipeline: localizeConversation: TLF: foo") and + # would split one operation into a row per argument value. + name = ": ".join(m.group(1).strip().split(": ")[:2])[:45] + ms = parse_duration_ms(m.group(2)) + if ms is None: + continue + total[name] += ms + calls[name] += 1 + if ms > worst.get(name, 0): + worst[name] = ms + + if not calls: + print("no traced spans with [time=] found") + return + + print(f"{sum(calls.values())} traced spans, {sum(total.values()) / 1000:.1f}s of service time") + print("(times overlap across goroutines - this is service time, not wall clock)\n") + print(f"{'total_s':>9} {'calls':>7} {'mean_ms':>9} {'max_ms':>9} operation") + shown = 0 + for name, ms in total.most_common(): + mean = ms / calls[name] + if mean < args.min_mean_ms: + continue + print(f"{ms / 1000:9.2f} {calls[name]:7d} {mean:9.2f} {worst[name]:9.1f} {name}") + shown += 1 + if shown >= args.top: + break + + +if __name__ == "__main__": + main() diff --git a/skill/keybase-rpc-log-analysis/scripts/rpc-diff.py b/skill/keybase-rpc-log-analysis/scripts/rpc-diff.py new file mode 100755 index 000000000000..e2509f5f762f --- /dev/null +++ b/skill/keybase-rpc-log-analysis/scripts/rpc-diff.py @@ -0,0 +1,64 @@ +#!/usr/bin/env python3 +"""Compare RPC counts between two runs, to prove a fix landed. + + python3 rpc-diff.py before.log after.log [--top 30] + +Counts app->service and service->server calls in each and prints the change. +Only meaningful if both runs did the same thing - e.g. the same e2e suite. +""" + +import argparse +import collections + +import kblog + + +def counts(path): + app, remote, http = collections.Counter(), collections.Counter(), collections.Counter() + for line in kblog.iter_lines([path]): + m = kblog.SERVER_RE.search(line) + if m: + app[m.group(1)] += 1 + m = kblog.REMOTE_RE.search(line) + if m: + remote[m.group(1)] += 1 + # HTTP matters as much as the RPC counts: a lot of server work - team + # loads, proof checks, merkle lookups - shows up only here. + m = kblog.HTTP_RE.search(line) + if m: + http[m.group(2)[:70]] += 1 + return app, remote, http + + +def show(title, before, after, top): + keys = set(before) | set(after) + rows = [] + for k in keys: + b, a = before.get(k, 0), after.get(k, 0) + rows.append((a - b, b, a, k)) + rows.sort(key=lambda r: abs(r[0]), reverse=True) + print(f"\n== {title} before {sum(before.values())} -> after {sum(after.values())}") + print(f"{'delta':>8} {'before':>8} {'after':>8} call") + for delta, b, a, k in rows[:top]: + if delta == 0: + continue + mark = " <-- gone" if a == 0 and b else (" <-- new" if b == 0 else "") + print(f"{delta:+8d} {b:8d} {a:8d} {k}{mark}") + + +def main(): + ap = argparse.ArgumentParser() + ap.add_argument("before") + ap.add_argument("after") + ap.add_argument("--top", type=int, default=30) + args = ap.parse_args() + + ba, br, bh = counts(args.before) + aa, ar, ah = counts(args.after) + show("REMOTE service -> keybase servers", br, ar, args.top) + show("APP app -> service", ba, aa, args.top) + show("HTTP", bh, ah, args.top) + + +if __name__ == "__main__": + main() diff --git a/skill/keybase-rpc-log-analysis/scripts/rpc-report.py b/skill/keybase-rpc-log-analysis/scripts/rpc-report.py new file mode 100755 index 000000000000..956f6bb61dcb --- /dev/null +++ b/skill/keybase-rpc-log-analysis/scripts/rpc-report.py @@ -0,0 +1,116 @@ +#!/usr/bin/env python3 +"""Summarize RPC traffic in a keybase service log. + + python3 rpc-report.py ... [--start 2026-07-24T17:23] [--end ...] [--top N] + +Sections, in the order worth reading: + REMOTE service -> keybase servers. These burn the server rate limits. + APP app -> service. One of these usually explains a REMOTE row. + HTTP raw API calls. + BURSTS same RPC, same args, >=3 times inside one second. Missing dedupe. + FLOODS most repeated log lines overall, after collapsing ids/numbers. +""" + +import argparse +import collections +import sys + +import kblog + + +def main(): + ap = argparse.ArgumentParser() + ap.add_argument("logs", nargs="+") + ap.add_argument("--start", help="timestamp prefix, e.g. 2026-07-24T17:23") + ap.add_argument("--end") + ap.add_argument("--top", type=int, default=25) + ap.add_argument("--burst-min", type=int, default=3, help="calls in one second to flag") + args = ap.parse_args() + + app = collections.Counter() + remote = collections.Counter() + http = collections.Counter() + floods = collections.Counter() + per_second = collections.defaultdict(collections.Counter) # sec -> (rpc, arg) -> n + # Many RPCs log no arguments, so "same call twice in a second" would not prove + # it was the same conversation. Recover the subject by correlating on the + # chat-trace: the first id that shows up on the trace is what the call was for. + pending = {} # trace -> (second, rpc name) + uid = None # learned from the log, then excluded when guessing subjects + total = 0 + first = last = None + + for line in kblog.iter_lines(args.logs, args.start, args.end): + total += 1 + ts = kblog.stamp(line) + if ts and ts.startswith("2"): + # min/max, not first/last seen: rotated files are often concatenated + # or globbed out of order, and stream order would report a range of + # zero width + first = ts if first is None or ts < first else first + last = ts if last is None or ts > last else last + floods[kblog.normalize(line)] += 1 + + if uid is None: + u = kblog.UID_RE.search(line) + if u: + uid = u.group(1) + + tr = kblog.trace(line) + m = kblog.SERVER_RE.search(line) + if m: + name, arg = m.group(1), (m.group(2) or "") + app[name] += 1 + if ts and arg: + per_second[ts][(name, arg[:60])] += 1 + elif ts and tr: + pending[tr] = (ts, name) + elif ts: + per_second[ts][(name, "")] += 1 + elif tr and tr in pending: + subject = kblog.subject_of(line, uid) + if subject: + sec, name = pending.pop(tr) + per_second[sec][(name, f"({subject[:16]})")] += 1 + m = kblog.REMOTE_RE.search(line) + if m: + remote[m.group(1)] += 1 + m = kblog.HTTP_RE.search(line) + if m: + http[m.group(2)[:80]] += 1 + + if not total: + sys.exit("no lines matched (check --start/--end)") + + print(f"{total} lines {first} .. {last}") + + def section(title, counter, note=""): + if not counter: + return + print(f"\n== {title} (total {sum(counter.values())}) {note}") + for k, v in counter.most_common(args.top): + print(f"{v:8d} {k}") + + section("REMOTE service -> keybase servers", remote, "<- rate-limited") + section("APP app -> service", app) + section("HTTP", http) + + bursts = collections.Counter() + for sec, calls in per_second.items(): + for (name, arg), n in calls.items(): + if n >= args.burst_min: + bursts[(name, arg)] = max(bursts[(name, arg)], n) + if bursts: + print(f"\n== BURSTS same call+subject >={args.burst_min}x in one second") + print(" subject in () is the call's own argument, or the first id seen on") + print(" its chat-trace when the call logs none. Bare name = neither.") + for (name, arg), n in bursts.most_common(args.top): + print(f"{n:8d}x {name}{arg}") + + print(f"\n== FLOODS most repeated lines") + for k, v in floods.most_common(args.top): + print(f"{v:8d} {k}") + + +if __name__ == "__main__": + main() diff --git a/skill/keybase-rpc-log-analysis/scripts/rpc-why.py b/skill/keybase-rpc-log-analysis/scripts/rpc-why.py new file mode 100755 index 000000000000..0056c017992c --- /dev/null +++ b/skill/keybase-rpc-log-analysis/scripts/rpc-why.py @@ -0,0 +1,137 @@ +#!/usr/bin/env python3 +"""Explain a flood: who drives it, how it is paced, how big its batches are. + + python3 rpc-why.py ... --match 'refreshParticipantsRemote' [--start ...] [--end ...] + +DRIVERS the '+ X' logged immediately before each hit on the same chat-trace. + This is what turns 'GetURL is called 8000 times' into 'userEmojis calls it'. +CADENCE hits per second. A flat rate that never stops is a loop; spikes are mounts. +GAPS ms between the end of one hit and the start of the next. Consistently + ~0 means the caller re-fires the instant the previous lands - a feedback + loop, not a user action. +BATCHES distribution of any 'msgIDs: N' / 'convs: N' count on the line. A pile of + 1s means the caller is not batching. +""" + +import argparse +import collections +import re + +import kblog + +COUNT_RE = re.compile(r"(msgIDs|convs|len|msgs): (\d+)") +# the +/-/| marker sits right after the log's sequence number, optionally behind +# a '++Chat: ' prefix. + enters a span, - leaves it, | is a note inside one. +MARKER_RE = re.compile(r"\]\s+\w+\s+(?:\+\+\w+:\s+)?([+\-|])\s+(.*)") + + +def main(): + ap = argparse.ArgumentParser() + ap.add_argument("logs", nargs="+") + ap.add_argument("--match", required=True, help="substring to explain") + ap.add_argument("--start") + ap.add_argument("--end") + ap.add_argument("--top", type=int, default=10) + args = ap.parse_args() + + drivers = collections.Counter() + roots = collections.Counter() + per_second = collections.Counter() + batches = collections.Counter() + last_enter = {} # trace -> most recent span entered outside our match + root_rpc = {} # trace -> first 'Server: X' seen, i.e. what the app asked for + inside = collections.Counter() # trace -> depth inside our match + hits = 0 + + for line in kblog.iter_lines(args.logs, args.start, args.end): + m = MARKER_RE.search(line) + if not m: + continue + marker, text = m.group(1), kblog.TAGS_RE.sub("", m.group(2)).strip() + # Lines with no chat-trace tag are unrelated to each other. Sharing one + # bucket for them latched every untraced hit onto whichever "+ Server:" + # line happened to come first in the file and reported it as the root - + # a confident, wrong answer, in the section SKILL.md says to trust most. + tr = kblog.trace(line) + untraced = tr is None + mine = args.match in text + + srv = kblog.SERVER_RE.search(line) + if srv and not untraced and tr not in root_rpc: + root_rpc[tr] = srv.group(1) + + if mine and marker == "+": + hits += 1 + ts = kblog.stamp(line) + if ts: + per_second[ts] += 1 + # Every hit gets attributed to its trace's originating RPC. Do NOT gate + # this on nesting depth: the service fans out concurrent goroutines + # under a single chat-trace, so most hits are "inside" an earlier one + # and gating would attribute only a handful of thousands of calls. + if untraced: + roots[""] += 1 + else: + roots[root_rpc.get(tr, "")] += 1 + # DRIVERS stays gated - the nearest preceding span is only meaningful + # for a hit that is not nested inside another of our own + if not untraced and not inside[tr]: + drivers[last_enter.get(tr, "")] += 1 + c = COUNT_RE.search(text) + if c: + batches[int(c.group(2))] += 1 + inside[tr] += 1 + elif mine and marker == "-": + inside[tr] = max(0, inside[tr] - 1) + elif marker == "+" and not inside[tr]: + # a span entered outside our match is a candidate driver; spans entered + # while inside it are its own children + last_enter[tr] = text[:55] + + if not hits: + print(f"no lines matched {args.match!r}") + return + + print(f"{hits} hits on {args.match!r}") + + print("\n== ROOT app RPC that opened the trace this ran on") + for k, v in roots.most_common(args.top): + print(f"{v:8d} {k}") + # Both buckets mean "nothing asked for this": a hit whose trace never saw a + # server RPC, and a hit with no trace at all. The DRIVERS placeholder is not + # one of them - it is only ever counted into drivers, so reading it back out + # of roots always added zero. + unrooted = roots.get("", 0) + roots.get( + "", 0 + ) + if unrooted > hits * 0.5: + print(" most hits have no app RPC above them: this is service-side background") + print(" work, not something the client asked for. Look at DRIVERS instead, and") + print(" grep the trace tags for CHTBKG= to confirm.") + + print("\n== DRIVERS call logged just before, same chat-trace") + for k, v in drivers.most_common(args.top): + print(f"{v:8d} {k}") + + print("\n== CADENCE busiest seconds") + for k, v in per_second.most_common(args.top): + print(f"{v:8d} {k}") + spread = len(per_second) + print(f" spread over {spread} distinct seconds, mean {hits / max(spread, 1):.1f}/s") + + if batches: + print("\n== BATCHES count on the line") + for size, n in batches.most_common(args.top): + print(f"{n:8d}x size {size}") + ones = batches.get(1, 0) + if ones > sum(batches.values()) * 0.5: + print(" >half are size 1 - the caller is not batching") + else: + print("\n== BATCHES none: these lines carry no item count") + print(" Remote calls usually do not log one. To see batch sizes, match the") + print(" local span that wraps this call instead - e.g. match") + print(" 'HybridConversationSource: GetMessages' rather than 'getMessagesRemote'.") + + +if __name__ == "__main__": + main() diff --git a/skill/keybase-rpc-log-analysis/scripts/start-service.sh b/skill/keybase-rpc-log-analysis/scripts/start-service.sh new file mode 100755 index 000000000000..fddd29c1b841 --- /dev/null +++ b/skill/keybase-rpc-log-analysis/scripts/start-service.sh @@ -0,0 +1,84 @@ +#!/bin/bash +# Build the service from this checkout and run it in the foreground with debug +# logging going to its own file. +# +# ./start-service.sh [logfile] # default /tmp/kb-analysis/service.log +# +# Runs against the REAL home directory, so the account, teams and conversations +# the e2e suite needs are all there. That means it replaces whatever service is +# currently running - it stops the existing one first. It does not log you out. +# +# Run this in its own terminal and leave it in the foreground; ^C stops it. Then +# start the app (yarn desktop:start:hot:e2e) so it attaches to this service +# rather than auto-forking the installed one. +set -euo pipefail + +LOGFILE="${1:-/tmp/kb-analysis/service.log}" +mkdir -p "$(dirname "$LOGFILE")" + +# -P resolves symlinks before collapsing "..": this script is normally reached +# through .claude/skills -> skill, and a logical cd would land in .claude/ +REPO="$(cd -P "$(dirname "${BASH_SOURCE[0]}")/../../.." && pwd)" +cd "$REPO/go" + +echo "building from $REPO/go" +go install github.com/keybase/client/go/keybase + +# go install honours GOBIN when it is set and only falls back to GOPATH/bin +# otherwise, so asking for GOPATH unconditionally looks in the wrong place on +# any setup that sets GOBIN. +GOBIN_DIR="$(go env GOBIN)" +BIN="${GOBIN_DIR:-$(go env GOPATH)/bin}/keybase" +[ -x "$BIN" ] || { echo "no binary at $BIN"; exit 1; } +echo "built $("$BIN" version -S 2>/dev/null || echo '?')" + +# The service does not provide the filesystem itself; it forks kbfs from the same +# bin directory. Without it the Files tab is empty and `keybase git` fails with +# "KBFS client not found", which fails every files-* and git-* e2e test for +# reasons that have nothing to do with the code under test. +# +# The name matters: on darwin the service looks for a binary called "kbfs" +# (install_darwin.go), but the package that builds it is kbfsfuse, so a plain +# go install leaves you with kbfsfuse and the service still finds nothing. Hence +# the link in the hint below. +if [ ! -x "$(dirname "$BIN")/kbfs" ]; then + echo + echo "WARNING: no kbfs binary next to $BIN." + echo " Files and git e2e tests will fail with 'KBFS client not found'." + echo " To include them:" + echo " go install github.com/keybase/client/go/kbfs/kbfsfuse" + echo " ln -s $(dirname "$BIN")/kbfsfuse $(dirname "$BIN")/kbfs" + echo " Otherwise ignore those failures - they are not regressions." + echo +fi + +# The installed service and this one cannot share the socket. --include takes a +# comma separated component list; an unknown flag here just prints usage and +# leaves the old service running, which then quietly wins the socket. +# pgrep -x matches the process NAME. Do not use pgrep -f here: the pattern would +# also match any shell whose command line happens to contain it, including the +# one running this check. +if pgrep -x keybase >/dev/null 2>&1; then + echo "stopping the running service" + "$BIN" ctl stop --include service || true + for _ in $(seq 10); do + pgrep -x keybase >/dev/null 2>&1 || break + sleep 1 + done + if pgrep -x keybase >/dev/null 2>&1; then + echo "service still running; kill it before rerunning" + exit 1 + fi +fi + +: > "$LOGFILE" +echo "logging to $LOGFILE" +echo +echo "NOTE: a newly built binary has a new code signature, so macOS will prompt" +echo " for keychain access on first run. Until someone clicks Allow the service" +echo " cannot finish bootstrapping, the app sits on its splash screen, and every" +echo " test fails for a reason that has nothing to do with the code. Watch for the" +echo " prompt before walking away." +echo +echo "leave this running; start the app in another terminal" +exec "$BIN" -d --log-file="$LOGFILE" service From 0d3a128f87efb3ee0ec3f49ebe3c8f72004ec73f Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Sun, 26 Jul 2026 15:00:42 -0400 Subject: [PATCH 2/7] perf(teams): share the team caches instead of one copy per consumer 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. --- shared/chat/conversation/team-hooks.tsx | 8 + shared/chat/create-channel/hooks.tsx | 2 + shared/teams/channel/create-channels.tsx | 2 + shared/teams/channel/index.tsx | 13 +- shared/teams/common/channel-hooks.tsx | 84 +++++-- shared/teams/common/general-conv.tsx | 59 +++++ .../common/team-channels-invalidation.tsx | 23 ++ .../teams/common/use-loaded-team-channels.tsx | 84 +++++-- .../teams/confirm-modals/delete-channel.tsx | 4 +- shared/teams/team/index.tsx | 50 +++- shared/teams/team/member/edit-channel.tsx | 7 + shared/teams/team/rows/index.tsx | 46 +--- .../team/settings-tab/default-channels.tsx | 139 ++++------- shared/teams/team/settings-tab/index.tsx | 94 ++++--- .../team/settings-tab/retention/index.tsx | 138 +++++----- .../team/settings-tab/retention/warning.tsx | 5 +- shared/teams/team/use-loaded-team.test.tsx | 101 ++++++++ shared/teams/team/use-loaded-team.tsx | 23 +- shared/teams/use-cached-resource.test.tsx | 236 ++++++++++++++++++ shared/teams/use-cached-resource.tsx | 136 +++++++--- shared/teams/use-teams-list.tsx | 8 + 21 files changed, 925 insertions(+), 337 deletions(-) create mode 100644 shared/teams/common/general-conv.tsx create mode 100644 shared/teams/common/team-channels-invalidation.tsx create mode 100644 shared/teams/team/use-loaded-team.test.tsx create mode 100644 shared/teams/use-cached-resource.test.tsx diff --git a/shared/chat/conversation/team-hooks.tsx b/shared/chat/conversation/team-hooks.tsx index 872359104947..55545ded9a6d 100644 --- a/shared/chat/conversation/team-hooks.tsx +++ b/shared/chat/conversation/team-hooks.tsx @@ -14,6 +14,7 @@ import { } from '@/teams/use-cached-resource' import {updateChosenChannelsTeamnames, useChosenChannelsTeamnames} from './manage-channels-badge' import {useThreadMeta} from './thread-context' +import {registerExternalResetter} from '@/util/zustand' type ChatTeamState = { allowPromote: boolean @@ -67,6 +68,13 @@ const chatTeamCacheMap: TeamCacheMap = new Map() const chatTeamMembersCacheMap: TeamCacheMap = new Map() const chatTeamReloadStaleMs = 5 * 60_000 +// module scope outlives sign-out, so the next user would be served the previous +// user's team data and member lists +registerExternalResetter('chat-team-hooks-caches', () => { + chatTeamCacheMap.clear() + chatTeamMembersCacheMap.clear() +}) + // A disabled "shadow" instance (one that returns the context value instead of // its own) must NOT share the loader's cache: with enabled=false // useCachedResource resets the cache, which would clobber the loader's data. diff --git a/shared/chat/create-channel/hooks.tsx b/shared/chat/create-channel/hooks.tsx index 679b8bd37602..cbcd86fdf318 100644 --- a/shared/chat/create-channel/hooks.tsx +++ b/shared/chat/create-channel/hooks.tsx @@ -4,6 +4,7 @@ import * as T from '@/constants/types' import {RPCError} from '@/util/errors' import upperFirst from 'lodash/upperFirst' import type {Props} from './index.shared' +import {invalidateTeamChannels} from '@/teams/common/team-channels-invalidation' import {useChatTeam} from '../conversation/team-hooks' export default (p: Props) => { @@ -57,6 +58,7 @@ export default (p: Props) => { C.waitingKeyTeamsCreateChannel(teamID) ) } + invalidateTeamChannels(teamID) onBack() if (navToChatOnSuccess) { previewConversation({channelname, conversationIDKey, reason: 'newChannel', teamname}) diff --git a/shared/teams/channel/create-channels.tsx b/shared/teams/channel/create-channels.tsx index 2c01b532af9e..c4ef5d5cc912 100644 --- a/shared/teams/channel/create-channels.tsx +++ b/shared/teams/channel/create-channels.tsx @@ -4,6 +4,7 @@ import * as Kb from '@/common-adapters' import * as T from '@/constants/types' import {RPCError} from '@/util/errors' import {CreateChannelsModal} from '../new-team/wizard/create-channels' +import {invalidateTeamChannels} from '../common/team-channels-invalidation' import {useLoadedTeam} from '../team/use-loaded-team' type Props = {teamID: T.Teams.TeamID} @@ -34,6 +35,7 @@ const submitChannels = async ( C.waitingKeyTeamsCreateChannel(teamID) ) } + invalidateTeamChannels(teamID) setSuccess(true) C.Router2.clearModals() } catch (error_) { diff --git a/shared/teams/channel/index.tsx b/shared/teams/channel/index.tsx index e8d8ff55e164..bfc4fff9606b 100644 --- a/shared/teams/channel/index.tsx +++ b/shared/teams/channel/index.tsx @@ -23,6 +23,7 @@ import {LoadedTeamChannelsProvider} from '../common/use-loaded-team-channels' import {useUsersState} from '@/stores/users' import {LoadedTeamProvider, useLoadedTeam} from '../team/use-loaded-team' import {unboxRows, useInboxMetadataState} from '@/chat/inbox/metadata' +import {registerExternalResetter} from '@/util/zustand' export type OwnProps = { teamID: T.Teams.TeamID @@ -75,18 +76,24 @@ const useLoadDataForChannelPage = ( } // keep track during session -const lastSelectedTabs: {[T: string]: TabKey} = {} +const lastSelectedTabs = new Map() const defaultTab: TabKey = 'members' +// module scope outlives sign-out; keyed by conversation, so the next user would +// inherit the previous user's tab choices +registerExternalResetter('teams-channel-index', () => { + lastSelectedTabs.clear() +}) + const useTabsState = ( conversationIDKey: T.Chat.ConversationIDKey, providedTab?: TabKey ): [TabKey, (t: TabKey) => void] => { - const defaultSelectedTab = lastSelectedTabs[conversationIDKey] ?? providedTab ?? defaultTab + const defaultSelectedTab = lastSelectedTabs.get(conversationIDKey) ?? providedTab ?? defaultTab const [selectedTab, setSelectedTab] = React.useState(defaultSelectedTab) React.useEffect(() => { - lastSelectedTabs[conversationIDKey] = selectedTab + lastSelectedTabs.set(conversationIDKey, selectedTab) }, [conversationIDKey, selectedTab]) const prevConvIDRef = React.useRef(conversationIDKey) diff --git a/shared/teams/common/channel-hooks.tsx b/shared/teams/common/channel-hooks.tsx index a28bdd7f1c02..71a6456aab57 100644 --- a/shared/teams/common/channel-hooks.tsx +++ b/shared/teams/common/channel-hooks.tsx @@ -5,12 +5,19 @@ import * as React from 'react' import logger from '@/logger' import {ensureError} from '@/util/errors' import {useLoadedTeam} from '../team/use-loaded-team' -import {createCachedResourceCache, type CachedResourceCache, useCachedResource} from '../use-cached-resource' +import { + createCachedResourceCache, + type CachedResourceCache, + getCachedResourceCache, + useCachedResource, +} from '../use-cached-resource' import { teamChannelsRPCParams, useLoadedTeamChannels, useReloadOnTeamChannelChanges, } from './use-loaded-team-channels' +import {registerExternalResetter} from '@/util/zustand' +import {registerTeamChannelsInvalidator} from './team-channels-invalidation' type ChannelMetasData = { channelMetas: Map @@ -19,6 +26,40 @@ type ChannelMetasData = { const allChannelMetasReloadStaleMs = 5_000 +const emptyChannelMetas = new Map() +const emptyChannelParticipants = new Map() +const emptyChannelMetasData: ChannelMetasData = { + channelMetas: emptyChannelMetas, + channelParticipants: emptyChannelParticipants, +} + +// One cache per team, shared by every consumer: getTLFConversations makes the +// service localize (and remotely refresh participants for) every channel in the +// team, so a per-instance cache turns each extra mount into a full refetch — +// enough of them trips the server's chat rate limit on a big team. +const allChannelMetasCaches = new Map< + T.Teams.TeamID, + CachedResourceCache +>() + +// module scope outlives sign-out, so the next user would be served the previous +// user's channel metas for any team they happen to share +registerExternalResetter('teams-all-channel-metas-caches', () => { + allChannelMetasCaches.clear() +}) + +const allChannelMetasInvalidationListeners = new Set<(teamID: T.Teams.TeamID) => void>() + +// This is the second shared cache of a team's channels, and channel create and +// delete fire no teamChangedByID, so it needs the same explicit drop as +// use-loaded-team-channels. Without it a create/delete leaves every +// useAllChannelMetas consumer - the browse-channels modal, the channels widget, +// the delete confirm itself - serving the pre-change list for the stale window. +registerTeamChannelsInvalidator((teamID: T.Teams.TeamID) => { + allChannelMetasCaches.get(teamID)?.invalidate(teamID) + allChannelMetasInvalidationListeners.forEach(listener => listener(teamID)) +}) + // Filter bots out using team role info, isolate to only when related state changes export const useChannelParticipants = ( teamID: T.Teams.TeamID, @@ -54,21 +95,20 @@ export const useAllChannelMetas = ( teamMeta: {teamname}, } = useLoadedTeam(teamID) const enabled = !dontCallRPC && !!teamname - const emptyChannelMetas = React.useMemo( - () => new Map(), - [] - ) - const emptyChannelParticipants = React.useMemo( - () => new Map(), - [] + const initialData = emptyChannelMetasData + // a dontCallRPC instance is disabled, and useCachedResource resets the cache it + // holds when disabled — give those their own throwaway cache so they can't wipe + // the shared one out from under a real loader + const [localCache] = React.useState>(() => + createCachedResourceCache(initialData, teamID) ) - const initialData = React.useMemo( - () => ({channelMetas: emptyChannelMetas, channelParticipants: emptyChannelParticipants}), - [emptyChannelMetas, emptyChannelParticipants] - ) - const [channelMetasCache] = React.useState>( - () => createCachedResourceCache(initialData, teamID) + const sharedCache = React.useMemo( + () => getCachedResourceCache(allChannelMetasCaches, initialData, teamID), + [initialData, teamID] ) + // gate on `enabled`, not just dontCallRPC: an instance whose teamname has not + // resolved yet is also disabled, and would otherwise reset the shared cache + const channelMetasCache = enabled ? sharedCache : localCache const {data, loading, reload, clear} = useCachedResource({ cache: channelMetasCache, cacheKey: teamID, @@ -111,6 +151,22 @@ export const useAllChannelMetas = ( useReloadOnTeamChannelChanges(teamID, enabled, reload, () => clear(teamID)) + // a mounted list must pick up a create/delete too, not just the next mount + React.useEffect(() => { + if (!enabled) { + return + } + const listener = (invalidatedTeamID: T.Teams.TeamID) => { + if (invalidatedTeamID === teamID) { + void reload() + } + } + allChannelMetasInvalidationListeners.add(listener) + return () => { + allChannelMetasInvalidationListeners.delete(listener) + } + }, [enabled, teamID, reload]) + return { channelMetas: data.channelMetas, channelParticipants: data.channelParticipants, diff --git a/shared/teams/common/general-conv.tsx b/shared/teams/common/general-conv.tsx new file mode 100644 index 000000000000..c7c2ffeb1159 --- /dev/null +++ b/shared/teams/common/general-conv.tsx @@ -0,0 +1,59 @@ +import * as React from 'react' +import * as T from '@/constants/types' +import * as Meta from '@/constants/chat/meta' +import {metasReceived} from '@/chat/inbox/metadata' +import {registerExternalResetter} from '@/util/zustand' +import { + type CachedResourceCache, + createCachedResourceCache, + getCachedResourceCache, + useCachedResource, +} from '../use-cached-resource' + +type GeneralConvData = T.Chat.ConversationIDKey | undefined + +const noGeneralConv: GeneralConvData = undefined + +// A team's #general conversation does not move, but two screens ask for it - the +// team rows and the bot install modal - and each used to hold the answer in its +// own state, so every mount was another findGeneralConvFromTeamID. Share one +// cache per team, and let it live a while since the answer is effectively static. +const generalConvCaches = new Map>() +const generalConvStaleMs = 5 * 60_000 + +registerExternalResetter('teams-general-conv-caches', () => { + generalConvCaches.clear() +}) + +export const useGeneralConvIDKey = (teamID?: T.Teams.TeamID, enabled = true) => { + const validTeamID = teamID && teamID !== T.Teams.noTeamID ? teamID : undefined + const on = enabled && !!validTeamID + const cacheKey = validTeamID ?? T.Teams.noTeamID + // a disabled instance resets whatever cache it holds, so keep it off the shared one + const [localCache] = React.useState>(() => + createCachedResourceCache(noGeneralConv, cacheKey) + ) + const sharedCache = React.useMemo( + () => getCachedResourceCache(generalConvCaches, noGeneralConv, cacheKey), + [cacheKey] + ) + const {data} = useCachedResource({ + cache: on ? sharedCache : localCache, + cacheKey, + enabled: on, + initialData: noGeneralConv, + load: async () => { + const conv = await T.RPCChat.localFindGeneralConvFromTeamIDRpcPromise({ + teamID: validTeamID ?? T.Teams.noTeamID, + }) + const meta = Meta.inboxUIItemToConversationMeta(conv) + if (!meta) { + return noGeneralConv + } + metasReceived([meta]) + return meta.conversationIDKey + }, + staleMs: generalConvStaleMs, + }) + return data +} diff --git a/shared/teams/common/team-channels-invalidation.tsx b/shared/teams/common/team-channels-invalidation.tsx new file mode 100644 index 000000000000..81d40eefb6ec --- /dev/null +++ b/shared/teams/common/team-channels-invalidation.tsx @@ -0,0 +1,23 @@ +import type * as T from '@/constants/types' + +type Invalidator = (teamID: T.Teams.TeamID) => void + +// Creating or deleting a channel fires no teamChangedByID, so the team channel +// cache has to be dropped by hand. This indirection exists so the chat-side +// create-channel screen can trigger it without importing anything from teams: +// teams/common sits in a require cycle (channel-hooks -> @/constants -> +// constants/router -> router-v2/routes -> chat/routes -> teams/common), and a +// chat -> teams import there has broken team rendering on iOS before. Keep this +// module a leaf: type-only imports. +const invalidators = new Set() + +export const registerTeamChannelsInvalidator = (invalidator: Invalidator) => { + invalidators.add(invalidator) + return () => { + invalidators.delete(invalidator) + } +} + +export const invalidateTeamChannels = (teamID: T.Teams.TeamID) => { + invalidators.forEach(invalidator => invalidator(teamID)) +} diff --git a/shared/teams/common/use-loaded-team-channels.tsx b/shared/teams/common/use-loaded-team-channels.tsx index d56861a201e1..679806ce71ca 100644 --- a/shared/teams/common/use-loaded-team-channels.tsx +++ b/shared/teams/common/use-loaded-team-channels.tsx @@ -4,6 +4,8 @@ import * as T from '@/constants/types' import {useEngineActionListener} from '@/engine/action-listener' import logger from '@/logger' import * as React from 'react' +import {registerExternalResetter} from '@/util/zustand' +import {registerTeamChannelsInvalidator} from './team-channels-invalidation' import {useLoadedTeam} from '../team/use-loaded-team' import {type CachedResourceCache, getCachedResourceCache, useCachedResource} from '../use-cached-resource' @@ -25,7 +27,6 @@ type LoadedTeamChannelsCacheMap = Map< > const LoadedTeamChannelsContext = React.createContext(null) -const LoadedTeamChannelsCacheContext = React.createContext(null) const loadedTeamChannelsReloadStaleMs = 5_000 const emptyChannels: ReadonlyMap = new Map() @@ -39,20 +40,37 @@ const emptyLoadedTeamChannelsData: LoadedTeamChannelsData = { channels: emptyChannels, } +// One map for every consumer. The stale window and the single-flight both live on +// the cache object, so callers holding separate maps cannot see each other's +// in-flight request and each issue their own getTLFConversationsLocal - which +// localizes every channel in the team. Measured at 7 calls for one team inside +// 1.5s before this was shared. +const loadedTeamChannelsCache: LoadedTeamChannelsCacheMap = new Map() +const loadedTeamChannelsInvalidationListeners = new Set<(teamID: T.Teams.TeamID | undefined) => void>() + +// module scope outlives sign-out and this is per-user team data +registerExternalResetter('loaded-team-channels-cache', () => { + loadedTeamChannelsCache.forEach(cache => cache.reset(emptyLoadedTeamChannelsData, undefined)) + loadedTeamChannelsCache.clear() +}) + +// While every consumer held a private cache a remount happened to refetch, which +// is what the channel list relied on after a create. Sharing one cache means a +// remount inside the stale window serves the pre-change channels instead, so the +// create/delete screens have to drop this explicitly. +registerTeamChannelsInvalidator((teamID: T.Teams.TeamID) => { + const key = loadableTeamID(teamID) + loadedTeamChannelsCache.get(key)?.invalidate(key) + loadedTeamChannelsInvalidationListeners.forEach(listener => listener(key)) +}) + // forceLocalCache: a disabled "shadow" instance (one that returns the context // value instead of its own) must NOT share the loader's cache map. With enabled=false // useCachedResource resets the cache (loadedAt=0), which would clobber the loader's // loaded data. Give shadows a private throwaway map so their resets are harmless. -const useLoadedTeamChannelsCacheMap = ( - providedCacheMap?: LoadedTeamChannelsCacheMap, - forceLocalCache = false -) => { - const contextCacheMap = React.useContext(LoadedTeamChannelsCacheContext) +const useLoadedTeamChannelsCacheMap = (forceLocalCache: boolean) => { const [localCacheMap] = React.useState(() => new Map()) - if (forceLocalCache) { - return localCacheMap - } - return providedCacheMap ?? contextCacheMap ?? localCacheMap + return forceLocalCache ? localCacheMap : loadedTeamChannelsCache } export const teamChannelsRPCParams = (teamname: string) => ({ @@ -90,7 +108,6 @@ const useLoadedTeamChannelsRaw = ( teamID: T.Teams.TeamID, providedTeamname?: string, enabled = true, - providedCacheMap?: LoadedTeamChannelsCacheMap, forceLocalCache = false ): LoadedTeamChannels => { const validTeamID = loadableTeamID(teamID) @@ -98,7 +115,12 @@ const useLoadedTeamChannelsRaw = ( teamMeta: {teamname: loadedTeamname}, } = useLoadedTeam(teamID, enabled) const teamnameToLoad = providedTeamname || loadedTeamname - const cacheMap = useLoadedTeamChannelsCacheMap(providedCacheMap, forceLocalCache) + // useCachedResource resets whatever cache it holds while disabled, so a + // disabled instance must never hold the shared one — including the ordinary + // consumer whose teamname has not resolved yet, which would otherwise wipe a + // real loader's data mid-flight. Gate the cache on exactly the load condition. + const canLoad = enabled && !!validTeamID && !!teamnameToLoad + const cacheMap = useLoadedTeamChannelsCacheMap(forceLocalCache || !canLoad) const cache = React.useMemo( () => getCachedResourceCache(cacheMap, emptyLoadedTeamChannelsData, validTeamID), [cacheMap, validTeamID] @@ -106,7 +128,7 @@ const useLoadedTeamChannelsRaw = ( const {data, loading, reload, clear} = useCachedResource({ cache, cacheKey: validTeamID, - enabled: enabled && !!validTeamID && !!teamnameToLoad, + enabled: canLoad, initialData: emptyLoadedTeamChannelsData, load: async () => { if (!teamnameToLoad) { @@ -149,21 +171,39 @@ const useLoadedTeamChannelsRaw = ( useReloadOnTeamChannelChanges(validTeamID, enabled, reload, () => clear(validTeamID)) - return {...data, loading, reload} + // a mounted list must pick up an invalidation too, not just the next mount + React.useEffect(() => { + if (!enabled || !validTeamID) { + return + } + const listener = (invalidatedTeamID: T.Teams.TeamID | undefined) => { + if (invalidatedTeamID === validTeamID) { + void reload() + } + } + loadedTeamChannelsInvalidationListeners.add(listener) + return () => { + loadedTeamChannelsInvalidationListeners.delete(listener) + } + }, [enabled, validTeamID, reload]) + + const {channelParticipants, channels} = data + return React.useMemo( + () => ({channelParticipants, channels, loading, reload}), + [channelParticipants, channels, loading, reload] + ) } export const LoadedTeamChannelsProvider = ( props: React.PropsWithChildren<{teamID: T.Teams.TeamID; teamname?: string}> ) => { const {children, teamID, teamname} = props - const [cacheMap] = React.useState(() => new Map()) - const loadedTeamChannels = useLoadedTeamChannelsRaw(teamID, teamname, true, cacheMap) - const value = {...loadedTeamChannels, teamID} - return ( - - {children} - + const loadedTeamChannels = useLoadedTeamChannelsRaw(teamID, teamname, true) + const value = React.useMemo( + () => ({...loadedTeamChannels, teamID}), + [loadedTeamChannels, teamID] ) + return {children} } export const useLoadedTeamChannels = ( @@ -172,7 +212,7 @@ export const useLoadedTeamChannels = ( ): LoadedTeamChannels => { const context = React.useContext(LoadedTeamChannelsContext) const useContextValue = context?.teamID === teamID - const raw = useLoadedTeamChannelsRaw(teamID, teamname, !useContextValue, undefined, useContextValue) + const raw = useLoadedTeamChannelsRaw(teamID, teamname, !useContextValue, useContextValue) return useContextValue ? context : raw } diff --git a/shared/teams/confirm-modals/delete-channel.tsx b/shared/teams/confirm-modals/delete-channel.tsx index 95ce9cf167c5..fea2f71887b7 100644 --- a/shared/teams/confirm-modals/delete-channel.tsx +++ b/shared/teams/confirm-modals/delete-channel.tsx @@ -5,6 +5,7 @@ import * as T from '@/constants/types' import {useNavigation} from '@react-navigation/native' import {pluralize} from '@/util/string' import setRouteParamsIfPresent from './set-route-params-if-present' +import {invalidateTeamChannels} from '@/teams/common/team-channels-invalidation' import {useAllChannelMetas} from '@/teams/common/channel-hooks' type Props = { @@ -77,11 +78,12 @@ const DeleteChannel = (props: Props) => { for (const channelID of channelIDs) { await deleteChannel(channelID) } + invalidateTeamChannels(teamID) setRouteParamsIfPresent(navigation, 'team', {selectedChannels: undefined}) C.Router2.clearModals() } C.ignorePromise(f()) - }, [channelIDs, deleteChannel, navigation]) + }, [channelIDs, deleteChannel, navigation, teamID]) return ( () +const primedParticipantConvs = new Set() const defaultTab: T.Teams.TabKey = 'members' +// module scope outlives sign-out; both are keyed by team/conversation, so the +// next user would inherit the previous user's tab choices and priming state +registerExternalResetter('teams-team-index', () => { + lastSelectedTabs.clear() + primedParticipantConvs.clear() +}) + const getSettingsErrorWaitingKeys = (teamID: T.Teams.TeamID) => [ C.waitingKeyTeamsLoadWelcomeMessage(teamID), @@ -127,29 +136,50 @@ const TeamBody = (props: Props) => { const {loading: loadingTeam, teamDetails, teamMeta, yourOperations} = useLoadedTeam(teamID) - C.Router2.useSafeFocusEffect(() => { - return () => teamSeen(teamID) - }) - C.Router2.useSafeFocusEffect(() => { - return () => { - if (props.justFinishedAddWizard) { - clearJustFinishedAddWizard() + // useSafeFocusEffect re-runs (and so runs its cleanup) whenever the callback + // identity changes, so an inline callback here would fire the gregor dismiss + // RPC on every render of this screen + const justFinishedAddWizard = props.justFinishedAddWizard + C.Router2.useSafeFocusEffect( + React.useCallback(() => { + return () => teamSeen(teamID) + }, [teamID]) + ) + C.Router2.useSafeFocusEffect( + React.useCallback(() => { + return () => { + if (justFinishedAddWizard) { + clearJustFinishedAddWizard() + } } - } - }) + }, [justFinishedAddWizard, clearJustFinishedAddWizard]) + ) const {channels, loading: loadingChannels} = useLoadedTeamChannels(teamID, teamMeta.teamname) // getTLFConversations leaves team channel participants empty; ask the service to // refresh them, which pushes ChatParticipantsInfo into useInboxMetadataState (read // by the channel rows). Without this the member counts render 0. + // + // This only primes them: later membership changes arrive on their own as + // ChatParticipantsInfo. `channels` gets a new identity on every reload of the + // channel list (stale refresh, team change, refocus), and each request costs a + // remote round trip, so a big team re-primed on every reload was enough to trip + // the server's chat rate limit. Ask once per conversation per session. const refreshParticipants = C.useRPC(T.RPCChat.localRefreshParticipantsRpcPromise) React.useEffect(() => { for (const conversationIDKey of channels.keys()) { + if (primedParticipantConvs.has(conversationIDKey)) { + continue + } + primedParticipantConvs.add(conversationIDKey) refreshParticipants( [{convID: T.Chat.keyToConversationID(conversationIDKey)}], () => {}, - () => {} + () => { + // let a later render retry a conversation whose refresh failed + primedParticipantConvs.delete(conversationIDKey) + } ) } }, [channels, refreshParticipants]) diff --git a/shared/teams/team/member/edit-channel.tsx b/shared/teams/team/member/edit-channel.tsx index a2862a134664..7550f45a74db 100644 --- a/shared/teams/team/member/edit-channel.tsx +++ b/shared/teams/team/member/edit-channel.tsx @@ -3,6 +3,7 @@ import * as Kb from '@/common-adapters' import * as React from 'react' import * as T from '@/constants/types' import {useSafeNavigation} from '@/util/safe-navigation' +import {invalidateTeamChannels} from '@/teams/common/team-channels-invalidation' import {useLoadedTeam} from '../use-loaded-team' type Props = { @@ -73,6 +74,12 @@ const EditChannel = (props: Props) => { ] Promise.all(ps) .then(() => { + // renaming a channel or editing its description fires no + // teamChangedByID, so the shared channel caches would keep serving the + // old name and description for their stale window + if (ps.length) { + invalidateTeamChannels(teamID) + } nav.safeNavigateUp() }) .catch(() => {}) diff --git a/shared/teams/team/rows/index.tsx b/shared/teams/team/rows/index.tsx index 9ee62233584f..2ba8fd01f28f 100644 --- a/shared/teams/team/rows/index.tsx +++ b/shared/teams/team/rows/index.tsx @@ -1,8 +1,8 @@ import * as C from '@/constants' -import * as Meta from '@/constants/chat/meta' import * as T from '@/constants/types' import type * as Kb from '@/common-adapters' import * as React from 'react' +import {useGeneralConvIDKey} from '@/teams/common/general-conv' import EmptyRow from './empty-row' import LoadingRow from './loading' import MemberRow from './member-row' @@ -23,7 +23,6 @@ import {getOrderedMemberArray, sortInvites, getOrderedBotsArray} from './helpers import {useEmojiState} from '../../emojis/use-emoji' import {useCurrentUserState} from '@/stores/current-user' import {useTeamsListMap} from '@/teams/use-teams-list' -import {metasReceived} from '@/chat/inbox/metadata' export type Item = | {type: 'members-loading'} @@ -287,49 +286,8 @@ export const useSubteamsSections = ( return sections } -const useGeneralConversationIDKey = (teamID?: T.Teams.TeamID) => { - const [conversationIDKeyResult, setConversationIDKeyResult] = React.useState< - {conversationIDKey: T.Chat.ConversationIDKey; teamID: T.Teams.TeamID} | undefined - >() - const findGeneralConvIDFromTeamID = C.useRPC(T.RPCChat.localFindGeneralConvFromTeamIDRpcPromise) - const requestIDRef = React.useRef(0) - const conversationIDKey = - conversationIDKeyResult && conversationIDKeyResult.teamID === teamID - ? conversationIDKeyResult.conversationIDKey - : undefined - - React.useEffect(() => { - if (conversationIDKey || !teamID) { - return - } - requestIDRef.current += 1 - const requestID = requestIDRef.current - findGeneralConvIDFromTeamID( - [{teamID}], - conv => { - if (requestIDRef.current !== requestID) { - return - } - const meta = Meta.inboxUIItemToConversationMeta(conv) - if (!meta) { - return - } - metasReceived([meta]) - setConversationIDKeyResult({conversationIDKey: meta.conversationIDKey, teamID}) - }, - () => {} - ) - return () => { - if (requestIDRef.current === requestID) { - requestIDRef.current += 1 - } - } - }, [conversationIDKey, findGeneralConvIDFromTeamID, teamID]) - return conversationIDKey -} - export const useEmojiSections = (teamID: T.Teams.TeamID, shouldActuallyLoad: boolean): Array
=> { - const convID = useGeneralConversationIDKey(teamID) + const convID = useGeneralConvIDKey(teamID) const getUserEmoji = C.useRPC(T.RPCChat.localUserEmojisRpcPromise) const [customEmoji, setCustomEmoji] = React.useState([]) const [filter, setFilter] = React.useState('') diff --git a/shared/teams/team/settings-tab/default-channels.tsx b/shared/teams/team/settings-tab/default-channels.tsx index c4d9f71079db..a7361abaf2f1 100644 --- a/shared/teams/team/settings-tab/default-channels.tsx +++ b/shared/teams/team/settings-tab/default-channels.tsx @@ -2,107 +2,68 @@ import * as C from '@/constants' import * as React from 'react' import * as Kb from '@/common-adapters' import * as T from '@/constants/types' -import type {RPCError} from '@/util/errors' -import {produce} from 'immer' +import logger from '@/logger' +import {registerExternalResetter} from '@/util/zustand' import {ChannelsWidget} from '@/teams/common' import {useLoadedTeam} from '../use-loaded-team' +import {type CachedResourceCache, getCachedResourceCache, useCachedResource} from '../../use-cached-resource' type Props = { teamID: T.Teams.TeamID } -export const useDefaultChannels = (teamID: T.Teams.TeamID) => { - type DefaultChannelsState = { - defaultChannels: Array - error?: RPCError - loadedTeamID?: T.Teams.TeamID - waiting: boolean - } +type DefaultChannelsData = ReadonlyArray - const getDefaultChannelsRPC = C.useRPC(T.RPCChat.localGetDefaultTeamChannelsLocalRpcPromise) - const [state, setState] = React.useState({ - defaultChannels: [], - error: undefined, - loadedTeamID: teamID, - waiting: true, - }) - const requestVersionRef = React.useRef(0) - const requestTeamIDRef = React.useRef(teamID) +const defaultChannelsStaleMs = 5_000 +const emptyDefaultChannels: DefaultChannelsData = [] - // no synchronous setState here so the mount effect below can call it directly; - // initial/mismatched-team state already shows waiting - const load = React.useCallback(() => { - const requestVersion = ++requestVersionRef.current - getDefaultChannelsRPC( - [{teamID}], - result => { - if (requestVersion !== requestVersionRef.current) { - return - } - setState({ - defaultChannels: [ - {channelname: 'general', conversationIDKey: 'unused'}, - ...(result.convs || []).map(conv => ({ - channelname: conv.channel, - conversationIDKey: conv.convID, - })), - ], - error: undefined, - loadedTeamID: teamID, - waiting: false, - }) - }, - err => { - if (requestVersion !== requestVersionRef.current) { - return - } - setState( - produce(draft => { - draft.error = err - draft.loadedTeamID = teamID - draft.waiting = false - }) - ) - } - ) - }, [getDefaultChannelsRPC, teamID]) +// One cache per team, shared by every consumer: each load is a remote +// chat.1.remote.getDefaultTeamChannels round trip, so a per-instance cache turns +// every extra mount of the settings tab into another hit on the chat rate limit. +const defaultChannelsCaches = new Map< + T.Teams.TeamID, + CachedResourceCache +>() - const reloadDefaultChannels = React.useCallback(() => { - setState( - produce(draft => { - draft.error = undefined - draft.waiting = true - }) - ) - load() - }, [load]) +// module scope outlives sign-out, so the next user would inherit this user's channels +registerExternalResetter('teams-default-channels-caches', () => { + defaultChannelsCaches.clear() +}) - React.useEffect(() => { - if (requestTeamIDRef.current !== teamID) { - requestTeamIDRef.current = teamID - requestVersionRef.current++ - } - }, [teamID]) - - React.useEffect(() => { - load() - }, [load]) +// resolve rather than reject on failure: consumers render an empty list (and no +// spinner) on error, and a cached failure keeps a broken team from re-requesting +// on every render. Module scope, not inline in the hook: a try/catch wrapping a +// value block is a react-compiler bailout. +const loadDefaultChannels = async (teamID: T.Teams.TeamID): Promise => { + try { + const {convs} = await T.RPCChat.localGetDefaultTeamChannelsLocalRpcPromise({teamID}) + return [ + {channelname: 'general', conversationIDKey: 'unused'}, + ...(convs ?? []).map(conv => ({channelname: conv.channel, conversationIDKey: conv.convID})), + ] + } catch (error) { + logger.warn(`Failed to load default channels for ${teamID}`, error) + return emptyDefaultChannels + } +} - const visibleState = - state.loadedTeamID === teamID - ? state - : { - defaultChannels: [] as Array, - error: undefined, - loadedTeamID: teamID, - waiting: true, - } +export const useDefaultChannels = (teamID: T.Teams.TeamID) => { + const cache = React.useMemo( + () => getCachedResourceCache(defaultChannelsCaches, emptyDefaultChannels, teamID), + [teamID] + ) + const {data, loaded, loading, reload} = useCachedResource({ + cache, + cacheKey: teamID, + initialData: emptyDefaultChannels, + load: async () => await loadDefaultChannels(teamID), + staleMs: defaultChannelsStaleMs, + }) return { - defaultChannels: visibleState.defaultChannels, - defaultChannelsWaiting: visibleState.waiting, - error: visibleState.error, - reloadDefaultChannels, + defaultChannels: data, + defaultChannelsWaiting: loading || !loaded, + reloadDefaultChannels: reload, } } @@ -125,7 +86,7 @@ const DefaultChannels = (props: Props) => { [{convs, teamID}], () => { setWaiting(false) - reloadDefaultChannels() + void reloadDefaultChannels() }, error => { setWaiting(false) @@ -145,7 +106,7 @@ const DefaultChannels = (props: Props) => { [{convs, teamID}], () => { setWaiting(false) - reloadDefaultChannels() + void reloadDefaultChannels() }, error => { setWaiting(false) diff --git a/shared/teams/team/settings-tab/index.tsx b/shared/teams/team/settings-tab/index.tsx index 52ba4e63821d..efae4ed62f5d 100644 --- a/shared/teams/team/settings-tab/index.tsx +++ b/shared/teams/team/settings-tab/index.tsx @@ -8,7 +8,6 @@ import {pluralize} from '@/util/string' import type {RPCError} from '@/util/errors' import RetentionPicker from './retention' import DefaultChannels from './default-channels' -import isEqual from 'lodash/isEqual' import {useLoadedTeam} from '../use-loaded-team' import {useIsBigTeam} from '../../common/use-loaded-team-channels' import {useSettingsTabState} from './use-settings' @@ -138,7 +137,6 @@ const OpenTeam = (props: { Anyone will be able to join immediately. Users will join as { const {error, savePublicity, isBigTeam, teamID, yourOperations, teamname, showOpenTeamWarning} = p const {canShowcase, allowOpenTrigger} = p - const [newPublicityAnyMember, setNewPublicityAnyMember] = React.useState(p.publicityAnyMember) - const [newPublicityTeam, setNewPublicityTeam] = React.useState(p.publicityTeam) - const [newIgnoreAccessRequests, setNewIgnoreAccessRequests] = React.useState(p.ignoreAccessRequests) - const [newPublicityMember, setNewPublicityMember] = React.useState(p.publicityMember) + const serverSettings = { + ignoreAccessRequests: p.ignoreAccessRequests, + openTeam: p.openTeam, + openTeamRole: p.openTeamRole, + publicityAnyMember: p.publicityAnyMember, + publicityMember: p.publicityMember, + publicityTeam: p.publicityTeam, + } + // resync the checkboxes whenever the server-side values change. Doing this in + // render instead of with a key on this component matters: a key would remount the + // whole subtree, and RetentionPicker / DefaultChannels each load over RPC on mount. + const serverKey = `${p.ignoreAccessRequests}:${p.openTeam}:${p.openTeamRole}:${p.publicityAnyMember}:${p.publicityMember}:${p.publicityTeam}` + const [local, setLocal] = React.useState({...serverSettings, serverKey}) const [isRolePickerOpen, setIsRolePickerOpen] = React.useState(false) - const [newOpenTeam, setNewOpenTeam] = React.useState(p.openTeam) - const [newOpenTeamRole, setNewOpenTeamRole] = React.useState(p.openTeamRole) const lastAllowOpenTriggerRef = React.useRef(allowOpenTrigger) - React.useEffect(() => { - if (lastAllowOpenTriggerRef.current !== allowOpenTrigger) { - lastAllowOpenTriggerRef.current = allowOpenTrigger - setNewOpenTeam(o => !o) - } - }, [allowOpenTrigger]) + if (local.serverKey !== serverKey) { + setLocal({...serverSettings, serverKey}) + } - const lastSave = React.useRef({ + const { ignoreAccessRequests: newIgnoreAccessRequests, openTeam: newOpenTeam, openTeamRole: newOpenTeamRole, publicityAnyMember: newPublicityAnyMember, publicityMember: newPublicityMember, publicityTeam: newPublicityTeam, - }) - React.useEffect(() => { - const next = { - ignoreAccessRequests: newIgnoreAccessRequests, - openTeam: newOpenTeam, - openTeamRole: newOpenTeamRole, - publicityAnyMember: newPublicityAnyMember, - publicityMember: newPublicityMember, - publicityTeam: newPublicityTeam, - } - if (!isEqual(next, lastSave.current)) { - lastSave.current = next + serverKey: localServerKey, + } = local + + const applySettings = React.useCallback( + (changes: Partial) => { + const next: T.Teams.PublicitySettings = { + ignoreAccessRequests: newIgnoreAccessRequests, + openTeam: newOpenTeam, + openTeamRole: newOpenTeamRole, + publicityAnyMember: newPublicityAnyMember, + publicityMember: newPublicityMember, + publicityTeam: newPublicityTeam, + ...changes, + } + setLocal({...next, serverKey: localServerKey}) savePublicity(next) + }, + [ + localServerKey, + newIgnoreAccessRequests, + newOpenTeam, + newOpenTeamRole, + newPublicityAnyMember, + newPublicityMember, + newPublicityTeam, + savePublicity, + ] + ) + + React.useEffect(() => { + if (lastAllowOpenTriggerRef.current !== allowOpenTrigger) { + lastAllowOpenTriggerRef.current = allowOpenTrigger + applySettings({openTeam: !newOpenTeam}) } - }, [savePublicity, newIgnoreAccessRequests, newOpenTeam, newOpenTeamRole, newPublicityAnyMember, newPublicityMember, newPublicityTeam]) + }, [allowOpenTrigger, applySettings, newOpenTeam]) return ( @@ -237,7 +259,7 @@ const Settings = (p: Props) => { yourOperationsJoinTeam={yourOperations.joinTeam} canShowcase={canShowcase} newPublicityMember={newPublicityMember} - setNewPublicityMember={setNewPublicityMember} + setNewPublicityMember={publicityMember => applySettings({publicityMember})} /> {(yourOperations.changeOpenTeam || yourOperations.setTeamShowcase || @@ -249,11 +271,14 @@ const Settings = (p: Props) => { {yourOperations.setPublicityAny ? ( applySettings({publicityAnyMember})} /> ) : null} {yourOperations.setTeamShowcase ? ( - + applySettings({publicityTeam})} + /> ) : null} {yourOperations.changeOpenTeam ? ( { onCancelRolePicker={() => setIsRolePickerOpen(false)} onConfirmRolePicker={(role: T.Teams.TeamRoleType) => { setIsRolePickerOpen(false) - setNewOpenTeamRole(role) + applySettings({openTeamRole: role}) }} onOpenRolePicker={() => setIsRolePickerOpen(true)} /> @@ -272,7 +297,7 @@ const Settings = (p: Props) => { {!newOpenTeam && yourOperations.changeTarsDisabled ? ( applySettings({ignoreAccessRequests})} /> ) : null} @@ -431,11 +456,8 @@ const SettingsTabContainer = (ownProps: OwnProps) => { ] ) - const settingsKey = `${ignoreAccessRequests}:${openTeam}:${openTeamRole}:${publicityAnyMember}:${publicityMember}:${publicityTeam}` - return ( boolean, - setTeamRetentionPolicy: (policy?: T.RPCChat.RetentionPolicy | null) => void -) => { - try { - const servicePolicy = await T.RPCChat.localGetTeamRetentionLocalRpcPromise( - {teamID}, - C.waitingKeyTeamsLoadRetentionPolicy(teamID) - ) - if (!shouldApply()) { - return - } - setTeamRetentionPolicy(servicePolicy) - } catch { - if (!shouldApply()) { - return - } - setTeamRetentionPolicy(undefined) - } -} +type TeamRetentionData = T.Retention.RetentionPolicy | undefined -const useLoadedTeamRetentionPolicy = (teamID: T.Teams.TeamID) => { - type TeamRetentionState = { - loadedTeamID?: T.Teams.TeamID - teamPolicy?: T.Retention.RetentionPolicy - } +const teamRetentionStaleMs = 5_000 +const noTeamRetentionPolicy: TeamRetentionData = undefined - const [state, setState] = React.useState({loadedTeamID: teamID, teamPolicy: undefined}) - const requestVersionRef = React.useRef(0) - const requestTeamIDRef = React.useRef(teamID) - - const setTeamRetentionPolicy = React.useCallback((policy?: T.RPCChat.RetentionPolicy | null) => { - const nextPolicy = Teams.serviceRetentionPolicyToRetentionPolicy(policy) - setState({ - loadedTeamID: teamID, - teamPolicy: nextPolicy.type === 'inherit' ? Teams.retentionPolicies.policyRetain : nextPolicy, - }) - }, [teamID]) - - const reload = React.useCallback(async () => { - const requestVersion = ++requestVersionRef.current - await loadTeamRetentionPolicy( - teamID, - () => requestVersion === requestVersionRef.current, - setTeamRetentionPolicy - ) - }, [setTeamRetentionPolicy, teamID]) +// One cache per team, shared by every consumer (settings tab and every chat info +// panel for the team): a burst of remounts would otherwise be a burst of +// GetTeamRetentionLocal calls with identical arguments. +const teamRetentionCaches = new Map>() - React.useEffect(() => { - if (requestTeamIDRef.current !== teamID) { - requestTeamIDRef.current = teamID - requestVersionRef.current++ - } - }, [teamID]) +// module scope outlives sign-out, so the next user would inherit this user's policies +registerExternalResetter('teams-retention-caches', () => { + teamRetentionCaches.clear() +}) - React.useEffect(() => { - let canceled = false - const requestVersion = ++requestVersionRef.current - void loadTeamRetentionPolicy( - teamID, - () => !canceled && requestVersion === requestVersionRef.current, - setTeamRetentionPolicy - ) - return () => { - canceled = true - } - }, [setTeamRetentionPolicy, teamID]) - - C.Router2.useSafeFocusEffect( - React.useCallback(() => { - void reload() - }, [reload]) +const useLoadedTeamRetentionPolicy = (teamID: T.Teams.TeamID) => { + const enabled = !!teamID && teamID !== T.Teams.noTeamID + // an adhoc conversation has no team, and useCachedResource resets the cache it + // holds when disabled — give those instances their own throwaway cache so they + // can't wipe the shared one out from under a real loader + const [localCache] = React.useState>(() => + createCachedResourceCache(noTeamRetentionPolicy, teamID) + ) + const sharedCache = React.useMemo( + () => getCachedResourceCache(teamRetentionCaches, noTeamRetentionPolicy, teamID), + [teamID] ) + const {data: teamPolicy, reload} = useCachedResource({ + cache: enabled ? sharedCache : localCache, + cacheKey: teamID, + enabled, + initialData: noTeamRetentionPolicy, + // resolve rather than reject on failure: the picker falls back to the default + // "retain" policy on error instead of spinning forever + load: async () => { + let servicePolicy: T.RPCChat.RetentionPolicy | undefined | null + try { + servicePolicy = await T.RPCChat.localGetTeamRetentionLocalRpcPromise( + {teamID}, + C.waitingKeyTeamsLoadRetentionPolicy(teamID) + ) + } catch (error) { + logger.warn(`Failed to load team retention policy for ${teamID}`, error) + } + const policy = Teams.serviceRetentionPolicyToRetentionPolicy(servicePolicy) + return policy.type === 'inherit' ? Teams.retentionPolicies.policyRetain : policy + }, + staleMs: teamRetentionStaleMs, + }) + // the notification carries the new policy, but reload() is what actually + // invalidates the shared cache for the other consumers useEngineActionListener('chat.1.NotifyChat.ChatSetTeamRetention', action => { - if (action.payload.params.teamID !== teamID) { - return - } - const first = action.payload.params.convs?.[0] - if (!first?.teamRetention) { + if (action.payload.params.teamID === teamID) { void reload() - return } - setTeamRetentionPolicy(first.teamRetention) }) - const visibleState = - state.loadedTeamID === teamID ? state : {loadedTeamID: teamID, teamPolicy: undefined} - return { - loading: !visibleState.teamPolicy, - teamPolicy: visibleState.teamPolicy, + loading: !teamPolicy, + teamPolicy, } } diff --git a/shared/teams/team/settings-tab/retention/warning.tsx b/shared/teams/team/settings-tab/retention/warning.tsx index 2cd46ed9c451..d21a39c8db69 100644 --- a/shared/teams/team/settings-tab/retention/warning.tsx +++ b/shared/teams/team/settings-tab/retention/warning.tsx @@ -1,5 +1,6 @@ import * as Kb from '@/common-adapters' import * as C from '@/constants' +import * as React from 'react' import type * as T from '@/constants/types' import type {RetentionEntityType} from '.' import ConfirmWarning from '../confirm-warning' @@ -87,12 +88,12 @@ const RetentionWarningContainer = (ownProps: OwnProps) => { const updateConfirm = useConfirm(s => s.dispatch.updateConfirm) C.Router2.useSafeFocusEffect( - () => { + React.useCallback(() => { openModal() return () => { closeModal() } - } + }, [openModal, closeModal]) ) const onConfirm = () => { diff --git a/shared/teams/team/use-loaded-team.test.tsx b/shared/teams/team/use-loaded-team.test.tsx new file mode 100644 index 000000000000..66bdaaaed044 --- /dev/null +++ b/shared/teams/team/use-loaded-team.test.tsx @@ -0,0 +1,101 @@ +/** @jest-environment jsdom */ +/// +import {expect, jest, test} from '@jest/globals' +import {act, render} from '@testing-library/react' +import * as T from '@/constants/types' +import {useCurrentUserState} from '@/stores/current-user' +import {useConfigState} from '@/stores/config' +import {LoadedTeamsListProvider} from '../use-teams-list' +import {LoadedTeamChannelsProvider, useLoadedTeamChannels} from '../common/use-loaded-team-channels' +import {LoadedTeamProvider, useLoadedTeam} from './use-loaded-team' + +const teamID = 'tid1' as T.Teams.TeamID + +const annotated = { + invites: [], + joinRequests: [], + members: [], + name: 'testteam', + settings: {joinAs: T.RPCGen.TeamRole.reader, open: false}, + showcase: {anyMemberShowcase: false, description: '', isShowcased: false}, + tarsDisabled: false, + transitiveSubteamsUnverified: {entries: []}, +} as unknown as T.RPCGen.AnnotatedTeam + +let annotatedCalls = 0 +jest.spyOn(T.RPCGen, 'teamsGetAnnotatedTeamRpcPromise').mockImplementation(async () => { + annotatedCalls++ + await Promise.resolve() + return annotated +}) +jest.spyOn(T.RPCChat, 'localGetTLFConversationsLocalRpcPromise').mockImplementation(async () => { + await Promise.resolve() + return {convs: [], offline: false} as never +}) +let listCalls = 0 +jest.spyOn(T.RPCGen, 'teamsTeamListUnverifiedRpcPromise').mockImplementation(async () => { + listCalls++ + await Promise.resolve() + return { + teams: [ + { + fqName: 'testteam', + isOpenTeam: false, + memberCount: 1, + role: T.RPCGen.TeamRole.owner, + teamID, + username: 'testuser', + }, + ], + } as never +}) +jest.spyOn(T.RPCGen, 'teamsGetTeamRoleMapRpcPromise').mockImplementation(async () => { + await Promise.resolve() + return {teams: {}, version: 1} as never +}) + +let bodyRenders = 0 +const Body = () => { + // counts real renders: compiling this away is exactly what the test measures + 'use no memo' + bodyRenders++ + const {teamMeta} = useLoadedTeam(teamID) + const {channels} = useLoadedTeamChannels(teamID) + return ( +
+ {teamMeta.teamname} + {channels.size} +
+ ) +} + +const WithChannels = () => { + const {teamMeta} = useLoadedTeam(teamID) + return ( + + + + ) +} + +// The team screen mounts one real loader plus a shadow instance per consumer; +// a teams-list load landing must not re-issue getAnnotatedTeam. +test('team screen loads getAnnotatedTeam once', async () => { + useCurrentUserState.setState({username: 'testuser'}) + useConfigState.setState({loggedIn: true}) + render( + + + + + + ) + for (let i = 0; i < 60; i++) { + await act(async () => { + await Promise.resolve() + }) + } + expect(listCalls).toBe(1) + expect(bodyRenders).toBeLessThan(10) + expect(annotatedCalls).toBe(1) +}) diff --git a/shared/teams/team/use-loaded-team.tsx b/shared/teams/team/use-loaded-team.tsx index 84011b0ff685..888a57922cc9 100644 --- a/shared/teams/team/use-loaded-team.tsx +++ b/shared/teams/team/use-loaded-team.tsx @@ -96,12 +96,15 @@ const useLoadedTeamRaw = ( ) // Seed from the teams-list cache so the header (teamname, avatar, member count) // renders immediately instead of waiting for getAnnotatedTeam to round-trip. + // key the memo on this team's meta, not on the map: the map gets a new + // identity on every teams-list reload, and a fresh initialData object churns + // the whole useCachedResource state/effect chain for no reason const teamsListMap = useTeamsListMap() + const listMeta = validTeamID ? teamsListMap.get(validTeamID) : undefined const initialData = React.useMemo(() => { const data = emptyLoadedTeamData(validTeamID) - const listMeta = validTeamID ? teamsListMap.get(validTeamID) : undefined return listMeta ? {...data, teamMeta: listMeta} : data - }, [validTeamID, teamsListMap]) + }, [validTeamID, listMeta]) const {data, loaded, loading, reload, clear} = useCachedResource({ cache, cacheKey: validTeamID, @@ -123,7 +126,13 @@ const useLoadedTeamRaw = ( }, staleMs: loadedTeamReloadStaleMs, }) - const roleAndDetails = roleAndDetailsFromMap(roleMap, validTeamID ?? T.Teams.noTeamID) + // builds a fresh object whenever the team is in the map, so without this the + // memos below (and the context value built from them) never hit and every + // consumer re-renders on every render of the provider + const roleAndDetails = React.useMemo( + () => roleAndDetailsFromMap(roleMap, validTeamID ?? T.Teams.noTeamID), + [roleMap, validTeamID] + ) const teamMeta = React.useMemo( () => ({ ...data.teamMeta, @@ -160,14 +169,18 @@ const useLoadedTeamRaw = ( } }, subscribeToUpdates) - return {...data, loaded, loading, reload, teamMeta, yourOperations} + const teamDetails = data.teamDetails + return React.useMemo( + () => ({loaded, loading, reload, teamDetails, teamMeta, yourOperations}), + [loaded, loading, reload, teamDetails, teamMeta, yourOperations] + ) } export const LoadedTeamProvider = (props: React.PropsWithChildren<{teamID: T.Teams.TeamID}>) => { const {children, teamID} = props const [cacheMap] = React.useState(() => new Map()) const loadedTeam = useLoadedTeamRaw(teamID, true, cacheMap) - const value = {...loadedTeam, teamID} + const value = React.useMemo(() => ({...loadedTeam, teamID}), [loadedTeam, teamID]) return ( {children} diff --git a/shared/teams/use-cached-resource.test.tsx b/shared/teams/use-cached-resource.test.tsx new file mode 100644 index 000000000000..c546c4225108 --- /dev/null +++ b/shared/teams/use-cached-resource.test.tsx @@ -0,0 +1,236 @@ +/** @jest-environment jsdom */ +/// +import {expect, jest, test} from '@jest/globals' +import {act, render} from '@testing-library/react' +import {createCachedResourceCache, useCachedResource} from './use-cached-resource' + +const flush = async (turns = 40) => { + for (let i = 0; i < turns; i++) { + await act(async () => { + await Promise.resolve() + }) + } +} + +type Data = {v: number} + +// A caller that rebuilds initialData every render (seeding it from another +// store) must not put useCachedResource into a render loop. +test('unstable initialData does not loop', async () => { + const cache = createCachedResourceCache({v: 0}, 'k') + let calls = 0 + let renders = 0 + const load = jest.fn(async () => { + calls++ + await Promise.resolve() + return {v: calls} + }) + const Comp = () => { + // counts real renders: compiling this away is exactly what the test measures + 'use no memo' + renders++ + const {data} = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) + return
{data.v}
+ } + render() + await flush() + expect(calls).toBe(1) + expect(renders).toBeLessThan(10) +}) + +// A load that rejects leaves loadedAt at 0, i.e. permanently stale. Without a +// backoff every re-render re-issued the request the instant the previous one +// settled, which hammered both the service and the server. +test('a failed load backs off instead of retrying on every render', async () => { + const cache = createCachedResourceCache({v: 0}, 'k') + let calls = 0 + let loadIfStale: (() => Promise) | undefined + const load = jest.fn(async () => { + calls++ + await Promise.resolve() + throw new Error('nope') + }) + const Comp = () => { + // drives loadIfStale from the test; the mount effect only ever fires once, + // so without this the assertion below holds even with no backoff at all + 'use no memo' + const resource = useCachedResource({ + cache, + cacheKey: 'k', + initialData: {v: 0}, + load, + onError: () => {}, + staleMs: 5000, + }) + loadIfStale = resource.loadIfStale + return
{resource.data.v}
+ } + render() + await flush() + expect(calls).toBe(1) + + // inside the backoff window: further attempts must not reach `load` + await act(async () => { + await loadIfStale?.() + await loadIfStale?.() + }) + await flush() + expect(calls).toBe(1) + + // past it: the next attempt goes through + const realNow = Date.now + Date.now = () => realNow() + 5_001 + try { + await act(async () => { + await loadIfStale?.() + }) + await flush() + } finally { + Date.now = realNow + } + expect(calls).toBe(2) +}) + +test('reload bypasses the failure backoff', async () => { + const cache = createCachedResourceCache({v: 0}, 'k') + let calls = 0 + let reload: (() => Promise) | undefined + const load = jest.fn(async () => { + calls++ + await Promise.resolve() + throw new Error('nope') + }) + const Comp = () => { + // hoists reload out to the test body; the compiler rejects the assignment + 'use no memo' + const resource = useCachedResource({ + cache, + cacheKey: 'k', + initialData: {v: 0}, + load, + onError: () => {}, + staleMs: 5000, + }) + reload = resource.reload + return
{resource.data.v}
+ } + render() + await flush() + expect(calls).toBe(1) + await act(async () => { + await reload?.() + }) + expect(calls).toBe(2) +}) + +test('a successful load is served from cache while fresh', async () => { + const cache = createCachedResourceCache({v: 0}, 'k') + let calls = 0 + const load = jest.fn(async () => { + calls++ + await Promise.resolve() + return {v: calls} + }) + const Comp = ({staleMs}: {staleMs: number}) => { + const {data, loaded} = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs}) + return
{loaded ? data.v : 'x'}
+ } + const first = render() + await flush() + expect(calls).toBe(1) + + // a re-render of the same fiber proves nothing: the effect deps are stable so + // it never re-runs. The window only matters to a NEW consumer of this cache. + first.unmount() + const second = render() + await flush() + expect(calls).toBe(1) + expect(cache.getData()).toEqual({v: 1}) + + // and a consumer that considers it stale does reload it + second.unmount() + render() + await flush() + expect(calls).toBe(2) +}) + +// A reload() fired because something changed must not settle to data that was +// already on the wire before the change: joining it would serve pre-change data +// AND stamp loadedAt on it, pinning the stale value for the whole window. +test('a forced reload supersedes a request that predates it', async () => { + const cache = createCachedResourceCache({v: 0}, 'k') + let calls = 0 + const releases: Array<(v: Data) => void> = [] + const load = jest.fn(async () => { + calls++ + return await new Promise(resolve => { + releases.push(resolve) + }) + }) + let reload: (() => Promise) | undefined + const Comp = () => { + 'use no memo' + const resource = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) + reload = resource.reload + return
{resource.loaded ? `v${resource.data.v}` : 'pending'}
+ } + const view = render() + await flush() + expect(calls).toBe(1) + + // the mutation happens here, while request 1 is still outstanding + let forced: Promise | undefined + act(() => { + forced = reload?.() + }) + await flush() + expect(calls).toBe(2) + + // request 1 lands with pre-change data and must not win + act(() => { + releases[0]?.({v: 1}) + }) + await flush() + act(() => { + releases[1]?.({v: 2}) + }) + await forced + await flush() + expect(view.getAllByText('v2')).toHaveLength(1) + expect(cache.getData()).toEqual({v: 2}) +}) + +// Two consumers mounting together must share one request, not race two. This is +// the property the module-level caches depend on: without it, sharing a cache +// across screens still issues an RPC per screen. +test('concurrent consumers of one cache share a single load', async () => { + const cache = createCachedResourceCache({v: 0}, 'k') + let calls = 0 + let release: ((v: Data) => void) | undefined + const load = jest.fn(async () => { + calls++ + return await new Promise(resolve => { + release = resolve + }) + }) + const Comp = () => { + const {data, loaded} = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) + return
{loaded ? `v${data.v}` : 'pending'}
+ } + const view = render( + <> + + + + ) + await flush() + expect(calls).toBe(1) + + act(() => { + release?.({v: 7}) + }) + await flush() + expect(calls).toBe(1) + // both consumers got the single load's result + expect(view.getAllByText('v7')).toHaveLength(2) +}) diff --git a/shared/teams/use-cached-resource.tsx b/shared/teams/use-cached-resource.tsx index 4d151998b6f2..e84925b410ad 100644 --- a/shared/teams/use-cached-resource.tsx +++ b/shared/teams/use-cached-resource.tsx @@ -5,16 +5,26 @@ import {produce} from 'immer' export type CachedResourceCache = { clearInFlight: (request: Promise) => void getData: () => T + getFailedAt: () => number getGeneration: () => number getInFlight: () => Promise | undefined + getInFlightStartedAt: () => number getKey: () => K getLoadedAt: () => number invalidate: (key: K) => void reset: (data: T, key: K) => void setDataLoaded: (data: T, generation: number) => void setInFlight: (request: Promise) => void + setLoadFailed: (generation: number) => void } +// A load that never lands (it rejected, or its result was discarded because the +// cache was invalidated mid-flight) leaves loadedAt at 0, i.e. permanently +// stale. Without this window every re-run of the effect below would re-issue the +// request the instant the previous one settled - an unbounded retry loop at RPC +// speed. An explicit reload()/invalidate() still retries immediately. +const loadFailureBackoffMs = 5_000 + type CachedResourceState = { data: T loaded: boolean @@ -65,10 +75,28 @@ const storedState = ( initialData, }) +// Settling to already-current data must not produce a new state object: every +// loadIfStale() that finds the cache fresh would otherwise force a re-render, +// which re-runs the effect that called it, which re-renders... a loop paced only +// by how fast React can commit. +const settledState = + (cache: CachedResourceCache, cacheKey: K, initialData: T, data: T) => + (prev: StoredCachedResourceState): StoredCachedResourceState => + prev.cache === cache && + Object.is(prev.cacheKey, cacheKey) && + Object.is(prev.initialData, initialData) && + Object.is(prev.data, data) && + prev.loaded && + !prev.loading + ? prev + : storedState(cache, cacheKey, initialData, {data, loaded: true, loading: false}) + export const createCachedResourceCache = (initialData: T, key: K): CachedResourceCache => { let data = initialData + let failedAt = 0 let generation = 0 let inFlight: Promise | undefined + let inFlightStartedAt = 0 let loadedAt = 0 let storedKey = key @@ -79,11 +107,14 @@ export const createCachedResourceCache = (initialData: T, key: K): CachedR } }, getData: () => data, + getFailedAt: () => failedAt, getGeneration: () => generation, getInFlight: (): Promise | undefined => inFlight, + getInFlightStartedAt: () => inFlightStartedAt, getKey: () => storedKey, getLoadedAt: () => loadedAt, invalidate: nextKey => { + failedAt = 0 generation += 1 inFlight = undefined loadedAt = 0 @@ -91,6 +122,7 @@ export const createCachedResourceCache = (initialData: T, key: K): CachedR }, reset: (nextData, nextKey) => { data = nextData + failedAt = 0 generation += 1 inFlight = undefined loadedAt = 0 @@ -99,11 +131,18 @@ export const createCachedResourceCache = (initialData: T, key: K): CachedR setDataLoaded: (nextData, requestGeneration) => { if (generation === requestGeneration) { data = nextData + failedAt = 0 loadedAt = Date.now() } }, setInFlight: request => { inFlight = request + inFlightStartedAt = Date.now() + }, + setLoadFailed: requestGeneration => { + if (generation === requestGeneration) { + failedAt = Date.now() + } }, } } @@ -130,19 +169,31 @@ const runLoad = async ( onError: ((error: unknown) => void) | undefined, requestVersion: number, requestVersionRef: React.RefObject, - setState: React.Dispatch>> + setState: React.Dispatch>>, + force: boolean, + requestedAt: number ) => { let request: Promise | undefined + // A forced load exists because something changed, so joining a request that + // was already on the wire before we asked would settle to pre-change data - + // and stamp loadedAt on it, holding it stale for the whole window. Only join a + // request that started at or after this one was asked for, which still + // collapses the N-mounted-consumers-reload-together case to one RPC. + const staleInFlight = force && cache.getInFlightStartedAt() < requestedAt + if (staleInFlight) { + cache.invalidate(cacheKey) + } + // after the invalidate above, which bumps it + const generation = cache.getGeneration() try { - const inFlight = cache.getInFlight() + const inFlight = staleInFlight ? undefined : cache.getInFlight() if (inFlight) { const data = await inFlight if (requestVersion === requestVersionRef.current) { - setState(storedState(cache, cacheKey, initialData, {data, loaded: true, loading: false})) + setState(settledState(cache, cacheKey, initialData, data)) } return } - const generation = cache.getGeneration() request = load().then(data => { cache.setDataLoaded(data, generation) return data @@ -150,9 +201,12 @@ const runLoad = async ( cache.setInFlight(request) const data = await request if (requestVersion === requestVersionRef.current) { - setState(storedState(cache, cacheKey, initialData, {data, loaded: true, loading: false})) + setState(settledState(cache, cacheKey, initialData, data)) } } catch (error) { + // record the failure even for a superseded request: the backoff belongs to + // the shared cache, not to whichever instance happened to own the request + cache.setLoadFailed(generation) if (requestVersion !== requestVersionRef.current) { return } @@ -177,22 +231,6 @@ export const useCachedResource = (props: Props) => { const hasFocusedSinceMountRef = React.useRef(false) const requestVersionRef = React.useRef(0) - const resetCache = React.useCallback( - (nextKey: K) => { - cache.reset(initialData, nextKey) - }, - [cache, initialData] - ) - - const clear = React.useCallback( - (nextKey: K = cacheKey) => { - requestVersionRef.current += 1 - resetCache(nextKey) - setState(storedState(cache, nextKey, initialData, emptyState(initialData))) - }, - [cache, cacheKey, initialData, resetCache] - ) - const latestRef = React.useRef({ cache, cacheKey, @@ -200,7 +238,6 @@ export const useCachedResource = (props: Props) => { initialData, load, onError, - resetCache, staleMs, }) React.useLayoutEffect(() => { @@ -211,13 +248,37 @@ export const useCachedResource = (props: Props) => { initialData, load, onError, - resetCache, staleMs, } - }, [cache, cacheKey, enabled, initialData, load, onError, resetCache, staleMs]) + }, [cache, cacheKey, enabled, initialData, load, onError, staleMs]) + + // deliberately does not depend on initialData: resetCache is in the main + // effect's dep array, and callers routinely rebuild initialData (seeding it + // from another store), which would re-run the effect on every such change. + const resetCache = React.useCallback( + (nextKey: K) => { + cache.reset(latestRef.current.initialData, nextKey) + }, + [cache] + ) + + const clear = React.useCallback( + (nextKey: K = cacheKey) => { + requestVersionRef.current += 1 + resetCache(nextKey) + setState(storedState(cache, nextKey, initialData, emptyState(initialData))) + }, + [cache, cacheKey, initialData, resetCache] + ) const loadResource = React.useCallback(async (force: boolean) => { - const {cache, cacheKey, enabled, initialData, load, onError, resetCache, staleMs} = latestRef.current + // stamped before any await so a forced load can tell an in-flight request + // that predates it from one issued in response to the same change + const requestedAt = Date.now() + const {cache, cacheKey, enabled, initialData, load, onError, staleMs} = latestRef.current + const resetCache = (nextKey: K) => { + cache.reset(initialData, nextKey) + } if (!Object.is(cache.getKey(), cacheKey)) { requestVersionRef.current += 1 resetCache(cacheKey) @@ -229,18 +290,33 @@ export const useCachedResource = (props: Props) => { } const loadedAt = cache.getLoadedAt() if (!force && loadedAt && Date.now() - loadedAt < staleMs) { - setState( - storedState(cache, cacheKey, initialData, {data: cache.getData(), loaded: true, loading: false}) - ) + setState(settledState(cache, cacheKey, initialData, cache.getData())) + return + } + const failedAt = cache.getFailedAt() + if (!force && failedAt && Date.now() - failedAt < loadFailureBackoffMs) { return } const requestVersion = ++requestVersionRef.current setState(prev => prev.cache === cache && Object.is(prev.cacheKey, cacheKey) && Object.is(prev.initialData, initialData) - ? {...prev, loading: true} + ? prev.loading + ? prev + : {...prev, loading: true} : storedState(cache, cacheKey, initialData, {...emptyState(initialData), loading: true}) ) - await runLoad(cache, cacheKey, initialData, load, onError, requestVersion, requestVersionRef, setState) + await runLoad( + cache, + cacheKey, + initialData, + load, + onError, + requestVersion, + requestVersionRef, + setState, + force, + requestedAt + ) }, []) const reload = React.useCallback(async () => { diff --git a/shared/teams/use-teams-list.tsx b/shared/teams/use-teams-list.tsx index 71ba3a28b531..4155983e2fcf 100644 --- a/shared/teams/use-teams-list.tsx +++ b/shared/teams/use-teams-list.tsx @@ -15,6 +15,7 @@ import { createCachedResourceCache, useCachedResource, } from './use-cached-resource' +import {registerExternalResetter} from '@/util/zustand' type TeamsList = { reload: () => void @@ -44,6 +45,13 @@ const teamsRoleMapCache = createCachedResourceCache { + teamsListCache.reset(emptyTeams, undefined) + teamsRoleMapCache.reset(emptyTeamRoleMap, undefined) +}) + const teamListToArray = (list: ReadonlyArray) => { return [...Teams.teamListToMeta(list).values()] } From afcf6fd9169ccf98d15e57cae77b3fed6527364a Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Sun, 26 Jul 2026 15:00:52 -0400 Subject: [PATCH 3/7] perf(tracker): one identify per profile view, shared by every surface 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. --- shared/common-adapters/profile-card.tsx | 5 +- shared/people/container.tsx | 9 +- shared/profile/generic/proofs-list.tsx | 2 +- shared/profile/user/hooks.tsx | 4 +- shared/profile/user/index.tsx | 6 + shared/profile/user/teams/index.tsx | 3 + .../user/teams/use-team-info-popup.tsx | 21 +- shared/tracker/identify-session.tsx | 409 ++++++++++++++++++ shared/tracker/use-profile.tsx | 333 +++----------- 9 files changed, 498 insertions(+), 294 deletions(-) create mode 100644 shared/tracker/identify-session.tsx diff --git a/shared/common-adapters/profile-card.tsx b/shared/common-adapters/profile-card.tsx index d3c78a9f1c0d..709864bab9df 100644 --- a/shared/common-adapters/profile-card.tsx +++ b/shared/common-adapters/profile-card.tsx @@ -140,7 +140,10 @@ const ProfileCard = ({ onLayoutChange, username, }: Props) => { - const {details: userDetails, loadProfile} = useTrackerProfile(username) + // a hover card is incidental, not "check this identity now" - the cached proof + // results are what it should show, and forcing a check re-fetches every proof + // from its third-party host + const {details: userDetails, loadProfile} = useTrackerProfile(username, {cachedOnMount: true}) const followThem = useFollowerState(s => s.following.has(username)) const followsYou = useFollowerState(s => s.followers.has(username)) const isSelf = useCurrentUserState(s => s.username === username) diff --git a/shared/people/container.tsx b/shared/people/container.tsx index 9a19244204c3..becd11fb43eb 100644 --- a/shared/people/container.tsx +++ b/shared/people/container.tsx @@ -435,7 +435,14 @@ const PeopleReloadable = () => { const onClickUser = (username: string) => navToProfile(username) - const onReload = (isRetry?: boolean) => getData(false, isRetry === true || !followSuggestions.length) + const reload = React.useEffectEvent((isRetry?: boolean) => { + getData(false, isRetry === true || !followSuggestions.length) + }) + // frozen wrapper: effect-event identities change every render, and an unstable + // fn makes useSafeFocusEffect re-run (so re-reload) on every render while focused + const [onReload] = React.useState(() => (isRetry?: boolean) => { + reload(isRetry) + }) C.Router2.useSafeFocusEffect(onReload) useEngineActionListener('keybase.1.homeUI.homeUIRefresh', () => { diff --git a/shared/profile/generic/proofs-list.tsx b/shared/profile/generic/proofs-list.tsx index e74f21e8cdc7..45f593bce6fe 100644 --- a/shared/profile/generic/proofs-list.tsx +++ b/shared/profile/generic/proofs-list.tsx @@ -349,7 +349,7 @@ const runProofFlow = async (p: { const ProofsList = ({platform, reason = 'profile'}: Props) => { const currentUsername = useCurrentUserState(s => s.username) const {proofSuggestions} = useProofSuggestions() - const {loadProfile} = useTrackerProfile(currentUsername) + const {loadProfile} = useTrackerProfile(currentUsername, {loadOnMount: false}) const registerCryptoAddress = C.useRPC(T.RPCGen.cryptocurrencyRegisterAddressRpcPromise) const isDarkMode = useColorScheme() === 'dark' const {clearModals, navigateAppend, navigateUp} = C.Router2 diff --git a/shared/profile/user/hooks.tsx b/shared/profile/user/hooks.tsx index 602dd6eb1d54..61f4dbe6a891 100644 --- a/shared/profile/user/hooks.tsx +++ b/shared/profile/user/hooks.tsx @@ -62,9 +62,7 @@ const useUserData = (username: string) => { loadNonUserProfile, loadProfile, nonUserDetails, - } = useTrackerProfile(username, { - reloadOnFocus: true, - }) + } = useTrackerProfile(username) const requestIDRef = React.useRef(0) const [sharedTeamsState, setSharedTeamsState] = React.useState<{ teams?: ReadonlyArray diff --git a/shared/profile/user/index.tsx b/shared/profile/user/index.tsx index 726286c8e24b..69febb4545d2 100644 --- a/shared/profile/user/index.tsx +++ b/shared/profile/user/index.tsx @@ -15,6 +15,7 @@ import {SiteIcon} from '../generic/site-icon' import useUserData from './hooks' import {LoadedTeamsListProvider} from '@/teams/use-teams-list' import * as TestIDs from '@/tests/e2e/shared/test-ids' +import {registerExternalResetter} from '@/util/zustand' export type BackgroundColorType = 'red' | 'green' | 'blue' @@ -566,6 +567,11 @@ const User = (props: {username: string}) => { const usernameSelectedTab = new Map() +// module scope outlives sign-out; keyed by username +registerExternalResetter('profile-user-selected-tab', () => { + usernameSelectedTab.clear() +}) + const avatarSize = 128 const styles = Kb.Styles.styleSheetCreate(() => ({ diff --git a/shared/profile/user/teams/index.tsx b/shared/profile/user/teams/index.tsx index 544ee5de25ea..6dfa1f08ab24 100644 --- a/shared/profile/user/teams/index.tsx +++ b/shared/profile/user/teams/index.tsx @@ -63,6 +63,9 @@ type TeamShowcaseProps = T.Tracker.TeamShowcase & { const TeamShowcase = (props: TeamShowcaseProps) => { const {name, isOpen} = props const {onClick, popup, popupAnchor} = useTeamInfoPopup({ + // a profile can feature every team its owner is in; annotating them all up + // front is one expensive getAnnotatedTeam per row for a popup nobody opened + loadOnDemand: true, popupInfo: props, teamname: name, }) diff --git a/shared/profile/user/teams/use-team-info-popup.tsx b/shared/profile/user/teams/use-team-info-popup.tsx index f71cdbd12e27..dcd9f159299d 100644 --- a/shared/profile/user/teams/use-team-info-popup.tsx +++ b/shared/profile/user/teams/use-team-info-popup.tsx @@ -27,11 +27,12 @@ const useTeamInfoPopup = ({loadOnDemand = false, popupInfo, teamID: initialTeamI const teamID = initialTeamID ?? teamNameToID.get(teamname) ?? T.Teams.noTeamID const teamLoadEnabled = !loadOnDemand || hasRequestedLoad const {loaded, loading: loadingTeam, teamDetails, teamMeta} = useLoadedTeam(teamID, teamLoadEnabled) - const hasLoadedTeam = teamMeta.teamname.length > 0 + // `loaded`, not a non-empty teamname: useLoadedTeam seeds teamMeta from the cheap + // teams-list, so a team you're in looks named long before its details exist. const inTeam = teamMeta.role !== 'none' - const description = hasLoadedTeam ? teamDetails.description : (popupInfo?.description ?? '') - const isOpen = hasLoadedTeam ? teamDetails.settings.open : (popupInfo?.isOpen ?? false) - const membersCount = hasLoadedTeam ? teamDetails.members.size : (popupInfo?.membersCount ?? 0) + const description = loaded ? teamDetails.description : (popupInfo?.description ?? '') + const isOpen = loaded ? teamDetails.settings.open : (popupInfo?.isOpen ?? false) + const membersCount = loaded ? teamDetails.members.size : (popupInfo?.membersCount ?? 0) const onJoinTeam = React.useCallback( (teamname: string) => { @@ -89,17 +90,23 @@ const useTeamInfoPopup = ({loadOnDemand = false, popupInfo, teamID: initialTeamI }, [loaded, loadingTeam, pendingOpen, showPopup]) const onClick = React.useCallback(() => { - if (!loadOnDemand || teamID === T.Teams.noTeamID || hasLoadedTeam) { + if (!loadOnDemand || teamID === T.Teams.noTeamID || loaded) { + showPopup() + return + } + setHasRequestedLoad(true) + // popupInfo already carries everything the popup shows (it comes from the + // profile's showcase payload), so open now and let the load refine it + if (popupInfo) { showPopup() return } if (loadingTeam || pendingOpen) { return } - setHasRequestedLoad(true) hasSeenPendingLoadRef.current = false setPendingOpen(true) - }, [hasLoadedTeam, loadOnDemand, loadingTeam, pendingOpen, showPopup, teamID]) + }, [loaded, loadOnDemand, loadingTeam, pendingOpen, popupInfo, showPopup, teamID]) return {loadingTeam, onClick, pendingOpen, popup, popupAnchor} } diff --git a/shared/tracker/identify-session.tsx b/shared/tracker/identify-session.tsx new file mode 100644 index 000000000000..9711cac44c59 --- /dev/null +++ b/shared/tracker/identify-session.tsx @@ -0,0 +1,409 @@ +import * as C from '@/constants' +import * as T from '@/constants/types' +import logger from '@/logger' +import {RPCError} from '@/util/errors' +import {generateGUIID, ignorePromise} from '@/constants/utils' +import {navigateAppend, navigateUp} from '@/constants/router' +import {produce} from 'immer' +import {registerExternalResetter} from '@/util/zustand' +import {subscribeToEngineAction} from '@/engine/action-listener' +import {useCurrentUserState} from '@/stores/current-user' +import {useUsersState} from '@/stores/users' +import { + identifyResultToDetailsState, + makeDetails, + noNonUserDetails, + updateTrackerDetailsBlocked, + updateTrackerDetailsReset, + updateTrackerDetailsResult, + updateTrackerDetailsRow, + updateTrackerDetailsSummary, + updateTrackerDetailsUserCard, +} from './model' + +// One identify per user, shared by every surface showing that user. An +// Identify3 with ignoreCache re-fetches every proof from the third party host, +// so N surfaces each running their own identify is N times the load on those +// hosts (and the service serializes them per uid, so they also run back to +// back). Instead a single session per username owns the identify, accumulates +// every identify3Ui event for it, and fans the result out to its subscribers - +// including ones that join after the identify already started. +type Session = { + details: T.Tracker.Details + generation: number + ignoreCache: boolean + inFlight: boolean + nonUserDetails: T.Tracker.NonUserDetails + startedAt: number + username: string +} + +export type IdentifyLoadOptions = { + // Join an identify that is already running only if it started at or after + // this timestamp, mirroring the singleflight in go/libkb/proof_cache.go. + // 0: any in-flight identify is good enough (mount, focus). + // Date.now(): only one started in reaction to the same event (notifications). + // Infinity: never join, always a fresh identify (explicit user reload). + freshAfter: number + ignoreCache: boolean + // Skip entirely if an identify at least this strong finished less than this + // long ago. Only the mount path sets it: reopening a profile you just looked + // at should not re-run every proof check. An explicit reload leaves it unset. + maxAgeMs?: number +} + +const sessions = new Map() +// Survives the session being dropped when it goes idle: closing a profile and +// reopening it should still count as "we just checked this user". +const lastCompleted = new Map() +// Kept outside of Session so a subscriber stays attached to its username even +// if the session object behind it is replaced. +const subscribersByUsername = new Map void>>() + +const notify = (username: string) => { + const subscribers = subscribersByUsername.get(username) + if (!subscribers?.size) { + return + } + for (const cb of [...subscribers]) { + cb() + } +} + +const setDetails = (s: Session, next: T.Tracker.Details) => { + if (next === s.details) { + return + } + s.details = next + notify(s.username) +} + +const makeSession = (username: string): Session => ({ + details: makeDetails(username), + generation: 0, + ignoreCache: false, + inFlight: false, + nonUserDetails: noNonUserDetails, + startedAt: 0, + username, +}) + +const ensureSession = (username: string) => { + const existing = sessions.get(username) + if (existing) { + return existing + } + const created = makeSession(username) + sessions.set(username, created) + return created +} + +const dropSessionIfIdle = (s: Session) => { + if (s.inFlight || subscribersByUsername.get(s.username)?.size) { + return + } + if (sessions.get(s.username) === s) { + sessions.delete(s.username) + } +} + +const sessionForGuiID = (guiID: string) => { + for (const s of sessions.values()) { + if (s.details.guiID === guiID) { + return s + } + } + return undefined +} + +const loadNonUserDetails = async (s: Session, generation: number) => { + const assertion = s.username + try { + const res = await T.RPCGen.userSearchGetNonUserDetailsRpcPromise({assertion}) + if (s.generation !== generation || !res.isNonUser) { + return + } + const common = { + assertionKey: res.assertionKey, + assertionValue: res.assertionValue, + description: res.description, + siteIcon: res.siteIcon || [], + siteIconDarkmode: res.siteIconDarkmode || [], + siteIconFull: res.siteIconFull || [], + siteIconFullDarkmode: res.siteIconFullDarkmode || [], + siteURL: '', + } + if (res.service) { + s.nonUserDetails = {...noNonUserDetails, ...common, ...res.service} + notify(s.username) + } else { + const {formatPhoneNumberInternational} = await import('@/util/phone-numbers') + const formattedName = + res.assertionKey === 'phone' ? formatPhoneNumberInternational('+' + res.assertionValue) : undefined + const fullName = res.contact?.contactName ?? '' + if (s.generation !== generation) { + return + } + s.nonUserDetails = {...noNonUserDetails, ...common, formattedName, fullName} + notify(s.username) + } + } catch (error) { + if (error instanceof RPCError) { + logger.warn(`Error loading non user profile: ${error.message}`) + } + } finally { + dropSessionIfIdle(s) + } +} + +export const loadNonUserProfile = (username: string) => { + if (!username) { + return + } + const s = ensureSession(username) + ignorePromise(loadNonUserDetails(s, s.generation)) +} + +const runIdentify = async (s: Session, generation: number, guiID: string, ignoreCache: boolean) => { + try { + await T.RPCGen.identify3Identify3RpcListener({ + incomingCallMap: {}, + params: {assertion: s.username, guiID, ignoreCache}, + waitingKey: C.waitingKeyTrackerProfileLoad, + }) + } catch (error) { + if (!(error instanceof RPCError) || s.generation !== generation) { + return + } + if (error.code === T.RPCGen.StatusCode.scresolutionfailed) { + setDetails( + s, + produce(s.details, draft => { + draft.state = 'notAUserYet' + }) + ) + loadNonUserProfile(s.username) + } else if (error.code === T.RPCGen.StatusCode.scnotfound) { + navigateUp() + navigateAppend({ + name: 'keybaseLinkError', + params: { + error: `You followed a profile link for a user (${s.username}) that does not exist.`, + }, + }) + } + logger.error(`Error loading profile: ${error.message}`) + } finally { + if (s.generation === generation) { + s.inFlight = false + lastCompleted.set(s.username, {at: Date.now(), ignoreCache: s.ignoreCache}) + dropSessionIfIdle(s) + } + } +} + +const loadFollowers = async (s: Session, generation: number) => { + try { + const fs = await T.RPCGen.userListTrackersUnverifiedRpcPromise( + {assertion: s.username}, + C.waitingKeyTrackerProfileLoad + ) + if (s.generation !== generation) { + return + } + setDetails( + s, + produce(s.details, draft => { + draft.followers = new Set((fs.users ?? []).map(f => f.username)) + draft.followersCount = fs.users?.length ?? 0 + }) + ) + if (fs.users) { + useUsersState + .getState() + .dispatch.updates(fs.users.map(u => ({info: {fullname: u.fullName}, name: u.username}))) + } + } catch (error) { + if (error instanceof RPCError) { + logger.error(`Error loading follower info: ${error.message}`) + } + } +} + +const loadFollowing = async (s: Session, generation: number) => { + try { + const fs = await T.RPCGen.userListTrackingRpcPromise( + {assertion: s.username, filter: ''}, + C.waitingKeyTrackerProfileLoad + ) + if (s.generation !== generation) { + return + } + setDetails( + s, + produce(s.details, draft => { + draft.following = new Set((fs.users ?? []).map(f => f.username)) + draft.followingCount = fs.users?.length ?? 0 + }) + ) + if (fs.users) { + useUsersState + .getState() + .dispatch.updates(fs.users.map(u => ({info: {fullname: u.fullName}, name: u.username}))) + } + } catch (error) { + if (error instanceof RPCError) { + logger.error(`Error loading following info: ${error.message}`) + } + } +} + +export const loadProfileIdentify = (username: string, options: IdentifyLoadOptions) => { + if (!username) { + return + } + ensureEngineSubscriptions() + const {freshAfter, ignoreCache} = options + const s = ensureSession(username) + // A weaker in-flight identify (one that was allowed to use the proof cache) + // cannot stand in for a caller that asked for a forced remote check. + const strongEnough = s.ignoreCache || !ignoreCache + if (s.inFlight && strongEnough && s.startedAt >= freshAfter) { + return + } + // A recent finished identify counts too, so long as it was at least as strong + // as what is being asked for now. + const {maxAgeMs} = options + const done = lastCompleted.get(username) + if (maxAgeMs !== undefined && done && (done.ignoreCache || !ignoreCache) && Date.now() - done.at < maxAgeMs) { + return + } + + const guiID = generateGUIID() + const generation = ++s.generation + s.ignoreCache = ignoreCache + s.inFlight = true + s.startedAt = Date.now() + setDetails( + s, + produce(s.details, draft => { + draft.guiID = guiID + if (!draft.resetBrokeTrack) { + draft.reason = '' + } + draft.state = 'checking' + }) + ) + ignorePromise(runIdentify(s, generation, guiID, ignoreCache)) + ignorePromise(loadFollowers(s, generation)) + ignorePromise(loadFollowing(s, generation)) +} + +// Registered once for the lifetime of the module. The unsubscribes are dropped +// on purpose; sign out clears the engine listener registry wholesale and the +// flag below lets them be re-registered on the next load. +let engineSubscribed = false +const ensureEngineSubscriptions = () => { + if (engineSubscribed) { + return + } + engineSubscribed = true + + subscribeToEngineAction('keybase.1.identify3Ui.identify3Result', action => { + const {guiID, result} = action.payload.params + const s = sessionForGuiID(guiID) + if (!s) { + return + } + setDetails(s, updateTrackerDetailsResult(s.details, identifyResultToDetailsState(result))) + }) + + subscribeToEngineAction('keybase.1.identify3Ui.identify3UpdateRow', action => { + const {row} = action.payload.params + const s = sessionForGuiID(row.guiID) + if (!s) { + return + } + setDetails(s, updateTrackerDetailsRow(s.details, row)) + }) + + subscribeToEngineAction('keybase.1.identify3Ui.identify3UserReset', action => { + const s = sessionForGuiID(action.payload.params.guiID) + if (!s) { + return + } + setDetails(s, updateTrackerDetailsReset(s.details)) + }) + + subscribeToEngineAction('keybase.1.identify3Ui.identify3UpdateUserCard', action => { + const {guiID, card} = action.payload.params + const s = sessionForGuiID(guiID) + if (!s) { + return + } + setDetails(s, updateTrackerDetailsUserCard(s.details, card)) + useUsersState.getState().dispatch.updates([{info: {fullname: card.fullName}, name: s.username}]) + }) + + subscribeToEngineAction('keybase.1.identify3Ui.identify3Summary', action => { + const {summary} = action.payload.params + const s = sessionForGuiID(summary.guiID) + if (!s) { + return + } + setDetails(s, updateTrackerDetailsSummary(s.details, summary)) + }) + + subscribeToEngineAction('keybase.1.NotifyTracking.notifyUserBlocked', action => { + for (const s of [...sessions.values()]) { + setDetails(s, updateTrackerDetailsBlocked(s.details, action.payload.params.b)) + } + }) + + subscribeToEngineAction('keybase.1.NotifyTracking.trackingChanged', action => { + const s = sessions.get(action.payload.params.username) + if (!s?.details.guiID) { + return + } + lastCompleted.delete(s.username) + loadProfileIdentify(s.username, {freshAfter: Date.now(), ignoreCache: true}) + }) + + subscribeToEngineAction('keybase.1.NotifyUsers.userChanged', action => { + const {uid, username} = useCurrentUserState.getState() + if (!username || uid !== action.payload.params.uid || !sessions.has(username)) { + return + } + lastCompleted.delete(username) + loadProfileIdentify(username, {freshAfter: Date.now(), ignoreCache: false}) + }) +} + +export const subscribeToProfile = (username: string, cb: () => void) => { + ensureSession(username) + let subscribers = subscribersByUsername.get(username) + if (!subscribers) { + subscribers = new Set() + subscribersByUsername.set(username, subscribers) + } + subscribers.add(cb) + return () => { + subscribers.delete(cb) + if (!subscribers.size) { + subscribersByUsername.delete(username) + } + const s = sessions.get(username) + if (s) { + dropSessionIfIdle(s) + } + } +} + +export const getProfileDetails = (username: string) => sessions.get(username)?.details +export const getProfileNonUserDetails = (username: string) => sessions.get(username)?.nonUserDetails + +registerExternalResetter('tracker-identify-sessions', () => { + sessions.clear() + lastCompleted.clear() + engineSubscribed = false +}) diff --git a/shared/tracker/use-profile.tsx b/shared/tracker/use-profile.tsx index b78f06f8f539..05e673efcce0 100644 --- a/shared/tracker/use-profile.tsx +++ b/shared/tracker/use-profile.tsx @@ -1,304 +1,75 @@ -import * as C from '@/constants' import * as React from 'react' -import * as T from '@/constants/types' -import {generateGUIID, ignorePromise} from '@/constants/utils' -import {useCurrentUserState} from '@/stores/current-user' -import {useUsersState} from '@/stores/users' -import {useEngineActionListener} from '@/engine/action-listener' -import {produce} from 'immer' -import logger from '@/logger' -import {RPCError} from '@/util/errors' -import {navigateAppend, navigateUp} from '@/constants/router' +import {makeDetails, noNonUserDetails} from './model' import { - identifyResultToDetailsState, - makeDetails, - noNonUserDetails, - updateTrackerDetailsBlocked, - updateTrackerDetailsReset, - updateTrackerDetailsResult, - updateTrackerDetailsRow, - updateTrackerDetailsSummary, - updateTrackerDetailsUserCard, -} from './model' + getProfileDetails, + getProfileNonUserDetails, + loadNonUserProfile, + loadProfileIdentify, + subscribeToProfile, +} from './identify-session' + +// Reopening a profile you were just looking at re-runs every proof check, and +// each one is an outbound request to a third-party host. Opening it again within +// this window reuses the check that just ran; an explicit reload always forces. +const profileRecheckMs = 30_000 type Options = { - reloadOnFocus?: boolean -} - -const loadNonUserDetails = async ( - username: string, - version: number, - requestVersionRef: React.RefObject, - setNonUserDetails: React.Dispatch< - React.SetStateAction<{details: T.Tracker.NonUserDetails; username: string}> - > -) => { - const assertion = username - try { - const res = await T.RPCGen.userSearchGetNonUserDetailsRpcPromise({assertion}) - if (requestVersionRef.current !== version || !res.isNonUser) { - return - } - const common = { - assertionKey: res.assertionKey, - assertionValue: res.assertionValue, - description: res.description, - siteIcon: res.siteIcon || [], - siteIconDarkmode: res.siteIconDarkmode || [], - siteIconFull: res.siteIconFull || [], - siteIconFullDarkmode: res.siteIconFullDarkmode || [], - siteURL: '', - } - if (res.service) { - setNonUserDetails({details: {...noNonUserDetails, ...common, ...res.service}, username}) - } else { - const {formatPhoneNumberInternational} = await import('@/util/phone-numbers') - const formattedName = - res.assertionKey === 'phone' ? formatPhoneNumberInternational('+' + res.assertionValue) : undefined - const fullName = res.contact?.contactName ?? '' - if (requestVersionRef.current !== version) { - return - } - setNonUserDetails({ - details: {...noNonUserDetails, ...common, formattedName, fullName}, - username, - }) - } - } catch (error) { - if (error instanceof RPCError) { - logger.warn(`Error loading non user profile: ${error.message}`) - } - } + // surfaces that only want loadProfile() to call after an action, and never + // read details, can skip the identify their mount would otherwise trigger + loadOnMount?: boolean + // Opening a profile means "check this identity now", so it forces a remote + // check of every proof. Incidental surfaces - a hover card, a follow button in + // a list - do not: they still need an identify session, but the cached proof + // results answer them, and forcing one re-fetches every proof from its + // third-party host, which those hosts rate limit. + cachedOnMount?: boolean } export const useTrackerProfile = (username: string, options?: Options) => { - const currentUser = useCurrentUserState( - C.useShallow(s => ({ - uid: s.uid, - username: s.username, - })) - ) - const [details, setDetails] = React.useState(() => makeDetails(username)) - const [nonUserDetails, setNonUserDetails] = React.useState<{ - details: T.Tracker.NonUserDetails - username: string - }>(() => ({ - details: {...noNonUserDetails}, - username, - })) - const requestVersionRef = React.useRef(0) - const detailsRef = React.useRef(details) - const hasSeenFocusRef = React.useRef(false) + const emptyDetails = React.useMemo(() => makeDetails(username), [username]) - React.useEffect(() => { - detailsRef.current = details - }, [details]) + const subscribe = React.useCallback((cb: () => void) => subscribeToProfile(username, cb), [username]) + const getDetails = React.useCallback(() => getProfileDetails(username) ?? emptyDetails, [ + emptyDetails, + username, + ]) + const getNonUserDetails = React.useCallback( + () => getProfileNonUserDetails(username) ?? noNonUserDetails, + [username] + ) + const details = React.useSyncExternalStore(subscribe, getDetails) + const nonUserDetails = React.useSyncExternalStore(subscribe, getNonUserDetails) - const loadNonUserProfile = React.useCallback(() => { - if (!username) { - return - } - ignorePromise( - loadNonUserDetails(username, requestVersionRef.current, requestVersionRef, setNonUserDetails) - ) + const loadNonUser = React.useCallback(() => { + loadNonUserProfile(username) }, [username]) + // Every caller of this is a deliberate user action (opening reload, or a + // refresh after follow / profile edit), so it never joins an identify that + // was already running. const loadProfile = React.useCallback( (ignoreCache = true) => { - if (!username) { - return - } - const guiID = generateGUIID() - const version = requestVersionRef.current + 1 - requestVersionRef.current = version - const preserveExistingData = detailsRef.current.username === username - - setDetails(prev => - produce(preserveExistingData ? prev : makeDetails(username), draft => { - draft.guiID = guiID - if (!draft.resetBrokeTrack) { - draft.reason = '' - } - draft.state = 'checking' - }) - ) - - const load = async () => { - try { - await T.RPCGen.identify3Identify3RpcListener({ - incomingCallMap: {}, - params: {assertion: username, guiID, ignoreCache}, - waitingKey: C.waitingKeyTrackerProfileLoad, - }) - } catch (error) { - if (!(error instanceof RPCError) || requestVersionRef.current !== version) { - return - } - if (error.code === T.RPCGen.StatusCode.scresolutionfailed) { - setDetails( - produce(draft => { - draft.state = 'notAUserYet' - }) - ) - loadNonUserProfile() - } else if (error.code === T.RPCGen.StatusCode.scnotfound) { - navigateUp() - navigateAppend({ - name: 'keybaseLinkError', - params: { - error: `You followed a profile link for a user (${username}) that does not exist.`, - }, - }) - } - logger.error(`Error loading profile: ${error.message}`) - } - } - ignorePromise(load()) - - const loadFollowers = async () => { - try { - const fs = await T.RPCGen.userListTrackersUnverifiedRpcPromise( - {assertion: username}, - C.waitingKeyTrackerProfileLoad - ) - if (requestVersionRef.current !== version) { - return - } - setDetails( - produce(draft => { - draft.followers = new Set((fs.users ?? []).map(f => f.username)) - draft.followersCount = fs.users?.length ?? 0 - }) - ) - if (fs.users) { - useUsersState - .getState() - .dispatch.updates(fs.users.map(u => ({info: {fullname: u.fullName}, name: u.username}))) - } - } catch (error) { - if (error instanceof RPCError) { - logger.error(`Error loading follower info: ${error.message}`) - } - } - } - ignorePromise(loadFollowers()) - - const loadFollowing = async () => { - try { - const fs = await T.RPCGen.userListTrackingRpcPromise( - {assertion: username, filter: ''}, - C.waitingKeyTrackerProfileLoad - ) - if (requestVersionRef.current !== version) { - return - } - setDetails( - produce(draft => { - draft.following = new Set((fs.users ?? []).map(f => f.username)) - draft.followingCount = fs.users?.length ?? 0 - }) - ) - if (fs.users) { - useUsersState - .getState() - .dispatch.updates(fs.users.map(u => ({info: {fullname: u.fullName}, name: u.username}))) - } - } catch (error) { - if (error instanceof RPCError) { - logger.error(`Error loading following info: ${error.message}`) - } - } - } - ignorePromise(loadFollowing()) + loadProfileIdentify(username, {freshAfter: Infinity, ignoreCache}) }, - [loadNonUserProfile, username] + [username] ) + const loadOnMount = options?.loadOnMount ?? true + const cachedOnMount = options?.cachedOnMount ?? false React.useEffect(() => { - if (username) { - loadProfile() - } - }, [loadProfile, username]) - - C.Router2.useSafeFocusEffect( - React.useCallback(() => { - if (!options?.reloadOnFocus) { - return - } - // The initial mount path already loads once. Skip the first focus callback - // so entering the screen does not immediately trigger a second hard reload. - if (!hasSeenFocusRef.current) { - hasSeenFocusRef.current = true - return - } - loadProfile(false) - }, [loadProfile, options?.reloadOnFocus]) - ) - - useEngineActionListener('keybase.1.NotifyTracking.trackingChanged', action => { - if (action.payload.params.username === username && detailsRef.current.guiID) { - loadProfile() - } - }) - - useEngineActionListener('keybase.1.identify3Ui.identify3Result', action => { - const {guiID, result} = action.payload.params - if (guiID !== detailsRef.current.guiID) { - return - } - setDetails(prev => updateTrackerDetailsResult(prev, identifyResultToDetailsState(result))) - }) - - useEngineActionListener('keybase.1.NotifyUsers.userChanged', action => { - if (currentUser.username === username && currentUser.uid === action.payload.params.uid) { - loadProfile(false) - } - }) - - useEngineActionListener('keybase.1.NotifyTracking.notifyUserBlocked', action => { - setDetails(prev => updateTrackerDetailsBlocked(prev, action.payload.params.b)) - }) - - useEngineActionListener('keybase.1.identify3Ui.identify3UpdateRow', action => { - const {row} = action.payload.params - if (row.guiID !== detailsRef.current.guiID) { - return - } - setDetails(prev => updateTrackerDetailsRow(prev, row)) - }) - - useEngineActionListener('keybase.1.identify3Ui.identify3UserReset', action => { - if (action.payload.params.guiID !== detailsRef.current.guiID) { - return - } - setDetails(prev => updateTrackerDetailsReset(prev)) - }) - - useEngineActionListener('keybase.1.identify3Ui.identify3UpdateUserCard', action => { - const {guiID, card} = action.payload.params - if (guiID !== detailsRef.current.guiID) { - return - } - setDetails(prev => updateTrackerDetailsUserCard(prev, card)) - useUsersState.getState().dispatch.updates([{info: {fullname: card.fullName}, name: username}]) - }) - - useEngineActionListener('keybase.1.identify3Ui.identify3Summary', action => { - const {summary} = action.payload.params - if (summary.guiID !== detailsRef.current.guiID) { - return + if (loadOnMount) { + loadProfileIdentify(username, { + freshAfter: 0, + ignoreCache: !cachedOnMount, + maxAgeMs: profileRecheckMs, + }) } - setDetails(prev => updateTrackerDetailsSummary(prev, summary)) - }) - - const detailsForUsername = details.username === username ? details : makeDetails(username) - const nonUserDetailsForUsername = - nonUserDetails.username === username ? nonUserDetails.details : {...noNonUserDetails} + }, [cachedOnMount, loadOnMount, username]) return { - details: detailsForUsername, - loadNonUserProfile, + details, + loadNonUserProfile: loadNonUser, loadProfile, - nonUserDetails: nonUserDetailsForUsername, + nonUserDetails, } } From da0ca54c3cb36179a1ae5c2c31479c09a8a3c3b5 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Sun, 26 Jul 2026 15:00:53 -0400 Subject: [PATCH 4/7] perf(chat): cache user emoji and stop the inbox search remounting 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. --- .../chat/conversation/attachment-actions.tsx | 7 ++ .../attachment-fullscreen/hooks.tsx | 6 + shared/chat/conversation/bot/install.tsx | 30 ++--- shared/chat/conversation/fwd-msg.tsx | 6 + .../input-area/suggestors/emoji.tsx | 10 +- .../input-area/suggestors/users.tsx | 6 + .../messages/attachment/shared.tsx | 14 ++- .../messages/wrapper/send-indicator.tsx | 6 + shared/chat/inbox-search/index.tsx | 11 +- shared/chat/inbox/index.tsx | 8 +- shared/chat/inbox/use-inbox-search.tsx | 5 +- shared/chat/inbox/use-inbox-state.tsx | 12 +- shared/chat/user-emoji.tsx | 104 +++++++++++++----- shared/constants/types/chat/index.tsx | 4 +- 14 files changed, 165 insertions(+), 64 deletions(-) diff --git a/shared/chat/conversation/attachment-actions.tsx b/shared/chat/conversation/attachment-actions.tsx index febd30410e86..83457df5b0e7 100644 --- a/shared/chat/conversation/attachment-actions.tsx +++ b/shared/chat/conversation/attachment-actions.tsx @@ -15,6 +15,7 @@ import { useConversationThreadID, useConversationThreadStore, } from './thread-context' +import {registerExternalResetter} from '@/util/zustand' const {darwinCopyToChatTempUploadFile} = KB2.functions @@ -93,6 +94,12 @@ export const showAttachmentPreview = ( const pdfMessageHandoff = new Map() +// module scope outlives sign-out; both hold attachment messages keyed by conversation +registerExternalResetter('chat-attachment-message-handoff', () => { + attachmentPreviewMessageHandoff.clear() + pdfMessageHandoff.clear() +}) + export const takePDFMessage = ( conversationIDKey: T.Chat.ConversationIDKey, messageID: T.Chat.MessageID diff --git a/shared/chat/conversation/attachment-fullscreen/hooks.tsx b/shared/chat/conversation/attachment-fullscreen/hooks.tsx index 81afe8ed4c86..361b1b0b6057 100644 --- a/shared/chat/conversation/attachment-fullscreen/hooks.tsx +++ b/shared/chat/conversation/attachment-fullscreen/hooks.tsx @@ -12,6 +12,7 @@ import { } from '../attachment-actions' import {showConversationInfoPanel} from '../thread-context' import {useConversationMessage} from '../data-hooks' +import {registerExternalResetter} from '@/util/zustand' const blankMessage = Chat.makeMessageAttachment({}) export const useData = ( @@ -116,6 +117,11 @@ export const useData = ( // if we've seen it its likely cached so lets just always just show it and never fallback const seenPaths = new Set() + +// module scope outlives sign-out; holds the previous user's attachment cache paths +registerExternalResetter('chat-attachment-seen-paths', () => { + seenPaths.clear() +}) // preload full and return ''. If too much time passes show preview. Show full when loaded export const usePreviewFallback = ( path: string, diff --git a/shared/chat/conversation/bot/install.tsx b/shared/chat/conversation/bot/install.tsx index 3c8136f05c56..f3def9270c32 100644 --- a/shared/chat/conversation/bot/install.tsx +++ b/shared/chat/conversation/bot/install.tsx @@ -1,6 +1,5 @@ import * as C from '@/constants' import * as ChatCommon from '@/constants/chat/common' -import * as Meta from '@/constants/chat/meta' import * as Teams from '@/constants/teams' import * as Kb from '@/common-adapters' import * as React from 'react' @@ -9,13 +8,15 @@ import ChannelPicker from './channel-picker' import {useChatTeam} from '../team-hooks' import {openURL} from '@/util/misc' import * as T from '@/constants/types' +import * as TestIDs from '@/tests/e2e/shared/test-ids' import {useAllChannelMetas} from '@/teams/common/channel-hooks' +import {useGeneralConvIDKey} from '@/teams/common/general-conv' import {useFeaturedBot} from '@/util/featured-bots' import {useRPCLoad} from '@/util/use-rpc-load' import {RPCError} from '@/util/errors' import logger from '@/logger' import {useBotSettings} from './settings' -import {metasReceived, participantInfoReceived} from '@/chat/inbox/metadata' +import {participantInfoReceived} from '@/chat/inbox/metadata' import {useConversationMeta} from '../data-hooks' const RestrictedItem = '---RESTRICTED---' @@ -66,22 +67,7 @@ export const useRefreshBotMembershipOnSuccess = ( export const useBotConversationIDKey = (inConvIDKey?: T.Chat.ConversationIDKey, teamID?: T.Teams.TeamID) => { const cleanInConvIDKey = T.Chat.isValidConversationIDKey(inConvIDKey ?? '') ? inConvIDKey : undefined - const {data: generalConvIDKey} = useRPCLoad( - T.RPCChat.localFindGeneralConvFromTeamIDRpcPromise, - [{teamID: teamID ?? T.Teams.noTeamID}], - { - enabled: !cleanInConvIDKey && !!teamID, - key: teamID ?? T.Teams.noTeamID, - map: conv => { - const meta = Meta.inboxUIItemToConversationMeta(conv) - if (!meta) { - return undefined - } - metasReceived([meta]) - return meta.conversationIDKey - }, - } - ) + const generalConvIDKey = useGeneralConvIDKey(teamID, !cleanInConvIDKey) return cleanInConvIDKey ?? generalConvIDKey } @@ -622,7 +608,13 @@ const InstallBotPopup = (props: Props) => { ) return ( <> - + {bodyContent} {enabled && (!readOnly || showReviewButton) ? ( diff --git a/shared/chat/conversation/fwd-msg.tsx b/shared/chat/conversation/fwd-msg.tsx index 112984361c51..52c00fa2d258 100644 --- a/shared/chat/conversation/fwd-msg.tsx +++ b/shared/chat/conversation/fwd-msg.tsx @@ -7,6 +7,7 @@ import {useNavigation} from '@react-navigation/native' import {Avatars, TeamAvatar} from '@/chat/avatars' import logger from '@/logger' import {useConversationMessage} from './data-hooks' +import {registerExternalResetter} from '@/util/zustand' type Props = {conversationIDKey?: T.Chat.ConversationIDKey; messageID: T.Chat.MessageID} @@ -16,6 +17,11 @@ const forwardMessageHandoff = new Map() const forwardMessageKey = (conversationIDKey: T.Chat.ConversationIDKey, messageID: T.Chat.MessageID) => `${conversationIDKey}:${T.Chat.messageIDToNumber(messageID)}` +// module scope outlives sign-out; holds message bodies keyed by conversation +registerExternalResetter('chat-forward-message-handoff', () => { + forwardMessageHandoff.clear() +}) + const getForwardMessage = (conversationIDKey: T.Chat.ConversationIDKey, messageID: T.Chat.MessageID) => { const key = forwardMessageKey(conversationIDKey, messageID) return forwardMessageHandoff.get(key) diff --git a/shared/chat/conversation/input-area/suggestors/emoji.tsx b/shared/chat/conversation/input-area/suggestors/emoji.tsx index 20a2a4952b51..864af09001ce 100644 --- a/shared/chat/conversation/input-area/suggestors/emoji.tsx +++ b/shared/chat/conversation/input-area/suggestors/emoji.tsx @@ -53,9 +53,15 @@ const cachedEmojiData = (emoji: T.RPCChat.Emoji) => { } const useDataSource = (conversationIDKey: T.Chat.ConversationIDKey, filter: string) => { - const {emojis: userEmojis, loading: userEmojisLoading} = useUserEmoji({conversationIDKey}) + // a filter the prepass rejects discards the result below, so don't pay for the + // fetch — userEmojis is expensive to resolve service-side + const matchesPrepass = emojiPrepass.test(filter) + const {emojis: userEmojis, loading: userEmojisLoading} = useUserEmoji({ + conversationIDKey, + disabled: !matchesPrepass, + }) - if (!emojiPrepass.test(filter)) { + if (!matchesPrepass) { return { items: empty, loading: false, diff --git a/shared/chat/conversation/input-area/suggestors/users.tsx b/shared/chat/conversation/input-area/suggestors/users.tsx index 5dc07ec5a86b..3b9cec4cb52d 100644 --- a/shared/chat/conversation/input-area/suggestors/users.tsx +++ b/shared/chat/conversation/input-area/suggestors/users.tsx @@ -6,6 +6,7 @@ import {useUsersState} from '@/stores/users' import {useChatTeamMembers} from '../../team-hooks' import {useInboxLayoutState} from '@/chat/inbox/layout-state' import {useConversationMetadata} from '../../data-hooks' +import {registerExternalResetter} from '@/util/zustand' export const transformer = ( input: { @@ -259,6 +260,11 @@ const keyExtractor = (item: ListItem) => { // the memoized suggestion rows can bail. Bounded by users/channels ever // suggested; the size guard is a backstop for pathological accounts. const listItemCache = new Map() + +// module scope outlives sign-out; keyed by username / team#channel +registerExternalResetter('chat-user-suggestor-item-cache', () => { + listItemCache.clear() +}) const canonicalizeItems = (items: Array) => { if (listItemCache.size > 8192) { listItemCache.clear() diff --git a/shared/chat/conversation/messages/attachment/shared.tsx b/shared/chat/conversation/messages/attachment/shared.tsx index 59b62e2c7a51..4a7f692365c8 100644 --- a/shared/chat/conversation/messages/attachment/shared.tsx +++ b/shared/chat/conversation/messages/attachment/shared.tsx @@ -53,12 +53,14 @@ export const ShowToastAfterSaving = ({transferState, toastTargetRef}: Props) => const [allowToast, setAllowToast] = React.useState(true) // since this uses portals we need to hide if we're hidden else we can get stuck showing if our render is frozen - C.Router2.useSafeFocusEffect(() => { - setAllowToast(true) - return () => { - setAllowToast(false) - } - }) + C.Router2.useSafeFocusEffect( + React.useCallback(() => { + setAllowToast(true) + return () => { + setAllowToast(false) + } + }, []) + ) return allowToast && showingToast ? ( diff --git a/shared/chat/conversation/messages/wrapper/send-indicator.tsx b/shared/chat/conversation/messages/wrapper/send-indicator.tsx index ddd714701c94..3a94bd44fc18 100644 --- a/shared/chat/conversation/messages/wrapper/send-indicator.tsx +++ b/shared/chat/conversation/messages/wrapper/send-indicator.tsx @@ -2,6 +2,7 @@ import * as React from 'react' import * as Kb from '@/common-adapters' import {produce} from 'immer' import {useColorScheme} from 'react-native' +import {registerExternalResetter} from '@/util/zustand' type AnimationStatus = | 'encrypting' @@ -46,6 +47,11 @@ const statusToIconDarkExploding = { const shownEncryptingSet = new Set() +// module scope outlives sign-out; holds the previous user's outbox ids +registerExternalResetter('chat-send-indicator-shown', () => { + shownEncryptingSet.clear() +}) + type OwnProps = { failed: boolean id: number diff --git a/shared/chat/inbox-search/index.tsx b/shared/chat/inbox-search/index.tsx index 02ccfccebd9b..c8d7e047b0f6 100644 --- a/shared/chat/inbox-search/index.tsx +++ b/shared/chat/inbox-search/index.tsx @@ -16,6 +16,7 @@ import { type InboxSearchVisibleResultCounts, } from '../inbox/use-inbox-search' import {showTeamByName} from '@/teams/team-page-actions' +import {registerExternalResetter} from '@/util/zustand' type OwnProps = { header?: React.ReactElement | null @@ -57,6 +58,12 @@ const textResultCache = new Map() const openTeamItemCache = new WeakMap() const botItemCache = new WeakMap() +// module scope outlives sign-out; these hold conversation and username hits +registerExternalResetter('chat-inbox-search-item-caches', () => { + nameResultCache.clear() + textResultCache.clear() +}) + const canonNameResult = (next: NameResult) => { if (nameResultCache.size > 4096) { nameResultCache.clear() @@ -258,7 +265,7 @@ export default function InboxSearchContainer(ownProps: OwnProps) { renderHeaderWithMore(section, _botsResults.length, botsCollapsed, botsAll, toggleBotsAll) const renderTextHeader = (section: Section) => { - const ratio = indexPercent / 100.0 + const ratio = (indexPercent ?? 0) / 100.0 return ( - ) : indexPercent > 0 && indexPercent < 100 ? ( + ) : indexPercent !== undefined && indexPercent < 100 ? ( Indexing... {isMobile ? ( diff --git a/shared/chat/inbox/index.tsx b/shared/chat/inbox/index.tsx index 8f0d557b957a..234e61c0338b 100644 --- a/shared/chat/inbox/index.tsx +++ b/shared/chat/inbox/index.tsx @@ -510,9 +510,11 @@ function NativeInboxBody(p: ControlledInboxProps) { const setOpenRow = useOpenedRowState(s => s.dispatch.setOpenRow) - C.Router2.useSafeFocusEffect(() => { - setOpenRow(Chat.noConversationIDKey) - }) + C.Router2.useSafeFocusEffect( + React.useCallback(() => { + setOpenRow(Chat.noConversationIDKey) + }, [setOpenRow]) + ) const promptSmallTeamsNum = React.useCallback(() => { if (isIOS) { diff --git a/shared/chat/inbox/use-inbox-search.tsx b/shared/chat/inbox/use-inbox-search.tsx index babdfa90579e..682bac546f89 100644 --- a/shared/chat/inbox/use-inbox-search.tsx +++ b/shared/chat/inbox/use-inbox-search.tsx @@ -25,7 +25,7 @@ export const makeInboxSearchInfo = (): T.Chat.InboxSearchInfo => ({ botsResults: [], botsResultsSuggested: false, botsStatus: 'initial', - indexPercent: 0, + indexPercent: undefined, nameResults: [], nameResultsUnread: false, nameStatus: 'initial', @@ -311,6 +311,9 @@ export function useInboxSearch(): InboxSearchController { updateIfActive(prev => ({ ...prev, botsStatus: 'inprogress', + // unknown again until this search reports one, rather than carrying + // the previous search's number + indexPercent: undefined, nameStatus: 'inprogress', openTeamsStatus: 'inprogress', selectedIndex: 0, diff --git a/shared/chat/inbox/use-inbox-state.tsx b/shared/chat/inbox/use-inbox-state.tsx index d5a571b60dba..83d55f37e629 100644 --- a/shared/chat/inbox/use-inbox-state.tsx +++ b/shared/chat/inbox/use-inbox-state.tsx @@ -139,11 +139,13 @@ export function useInboxState( C.Router2.setChatRootParams({refreshInbox: undefined}) }, [inboxRefresh, isFocused, loggedIn, refreshInbox, username]) - C.Router2.useSafeFocusEffect(() => { - if (!inboxHasLoaded) { - C.ignorePromise(inboxRefresh('componentNeverLoaded')) - } - }) + C.Router2.useSafeFocusEffect( + React.useCallback(() => { + if (!inboxHasLoaded) { + C.ignorePromise(inboxRefresh('componentNeverLoaded')) + } + }, [inboxHasLoaded, inboxRefresh]) + ) React.useEffect(() => { const ready = loggedIn && !!username diff --git a/shared/chat/user-emoji.tsx b/shared/chat/user-emoji.tsx index d3410c7f47e3..a2a4f3e3c0d4 100644 --- a/shared/chat/user-emoji.tsx +++ b/shared/chat/user-emoji.tsx @@ -1,9 +1,37 @@ +import * as React from 'react' import * as T from '@/constants/types' -import {useRPCLoad} from '@/util/use-rpc-load' +import {useEmojiState} from '@/teams/emojis/use-emoji' +import { + type CachedResourceCache, + createCachedResourceCache, + getCachedResourceCache, + useCachedResource, +} from '@/teams/use-cached-resource' +import {registerExternalResetter} from '@/util/zustand' const emptyEmojiGroups: ReadonlyArray = [] const emptyEmojis: ReadonlyArray = [] +type UserEmojiData = { + emojiGroups: ReadonlyArray + emojis: ReadonlyArray +} + +const emptyUserEmojiData: UserEmojiData = {emojiGroups: emptyEmojiGroups, emojis: emptyEmojis} + +// One cache per request key, shared by every consumer. userEmojis is expensive on +// the service side - it resolves two attachment URLs per custom emoji - so the +// suggestor remounting on each ':' trigger used to refetch the entire set. The +// shared cache also collapses concurrent mounts onto a single in-flight request. +const userEmojiCaches = new Map>() +const userEmojiStaleMs = 60_000 + +// module scope outlives sign-out, so the next user would be served the previous +// user's custom emoji until the entries went stale +registerExternalResetter('chat-user-emoji-caches', () => { + userEmojiCaches.clear() +}) + const flattenUserEmojis = (groups: ReadonlyArray) => { const emojis = new Array() groups.forEach(group => { @@ -25,33 +53,59 @@ export const useUserEmoji = ({ const requestKey = `${conversationIDKey ?? T.Chat.noConversationIDKey}:${ requestOnlyInTeam ? 'team' : 'all' }` - const {data, loading} = useRPCLoad( - T.RPCChat.localUserEmojisRpcPromise, - [ - { - convID: - conversationIDKey && conversationIDKey !== T.Chat.noConversationIDKey - ? T.Chat.keyToConversationID(conversationIDKey) - : null, - opts: { - getAliases: true, - getCreationInfo: false, - onlyInTeam: requestOnlyInTeam, - }, - }, - ], - { - enabled: !disabled, - key: requestKey, - map: results => { - const nextGroups = results.emojis.emojis ?? emptyEmojiGroups - return {emojiGroups: nextGroups, emojis: flattenUserEmojis(nextGroups)} + // adding, aliasing or removing an emoji bumps this; it busts the cache so the + // picker and the suggestor pick the change up instead of serving a stale set + const emojiUpdatedTrigger = useEmojiState(s => s.emojiUpdatedTrigger) + // a disabled instance resets the cache it holds, so keep those off the shared one + const [localCache] = React.useState>(() => + createCachedResourceCache(emptyUserEmojiData, requestKey) + ) + const sharedCache = React.useMemo( + () => getCachedResourceCache(userEmojiCaches, emptyUserEmojiData, requestKey), + [requestKey] + ) + const load = React.useCallback(async () => { + const results = await T.RPCChat.localUserEmojisRpcPromise({ + convID: + conversationIDKey && conversationIDKey !== T.Chat.noConversationIDKey + ? T.Chat.keyToConversationID(conversationIDKey) + : null, + opts: { + getAliases: true, + getCreationInfo: false, + onlyInTeam: requestOnlyInTeam, }, + }) + const emojiGroups = results.emojis.emojis ?? emptyEmojiGroups + return {emojiGroups, emojis: flattenUserEmojis(emojiGroups)} + }, [conversationIDKey, requestOnlyInTeam]) + + const {data, loading, reload} = useCachedResource({ + cache: disabled ? localCache : sharedCache, + cacheKey: requestKey, + enabled: !disabled, + initialData: emptyUserEmojiData, + load, + refreshKey: emojiUpdatedTrigger, + staleMs: userEmojiStaleMs, + }) + + // refreshKey alone only re-checks staleness, and an edit we just made is never + // stale — force past the cache when the emoji set actually changed + const lastEmojiUpdatedTriggerRef = React.useRef(emojiUpdatedTrigger) + React.useEffect(() => { + if (lastEmojiUpdatedTriggerRef.current === emojiUpdatedTrigger) { + return } - ) + lastEmojiUpdatedTriggerRef.current = emojiUpdatedTrigger + if (!disabled) { + void reload() + } + }, [disabled, emojiUpdatedTrigger, reload]) + return { - emojiGroups: disabled ? undefined : (data?.emojiGroups ?? emptyEmojiGroups), - emojis: data?.emojis ?? emptyEmojis, + emojiGroups: disabled ? undefined : data.emojiGroups, + emojis: data.emojis, loading, } } diff --git a/shared/constants/types/chat/index.tsx b/shared/constants/types/chat/index.tsx index 2da28cb4cb6a..60e5d47cc913 100644 --- a/shared/constants/types/chat/index.tsx +++ b/shared/constants/types/chat/index.tsx @@ -50,7 +50,9 @@ export type InboxSearchOpenTeamHit = { } export type InboxSearchInfo = { - indexPercent: number + // undefined until the service reports one; 0 is a real value meaning nothing + // is indexed yet, which is what an index rebuild looks like + indexPercent?: number botsResults: ReadonlyArray botsResultsSuggested: boolean botsStatus: InboxSearchStatus From 48e4e705303f741e887673e9e58dc7c2dd364c2b Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Sun, 26 Jul 2026 15:01:03 -0400 Subject: [PATCH 5/7] fix(e2e): repair four desktop flow tests 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 " 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. --- .../tests/e2e/electron/flows/chat-modals.test.ts | 13 ++++++++----- .../electron/flows/device-wallet-modals.test.ts | 3 ++- .../tests/e2e/electron/flows/teams-inner.test.ts | 15 +++++++++------ .../tests/e2e/electron/flows/teams-modals.test.ts | 15 +++++++++++++-- shared/tests/e2e/electron/playwright.config.ts | 4 +++- shared/tests/e2e/shared/test-ids.ts | 6 ++++++ 6 files changed, 41 insertions(+), 15 deletions(-) diff --git a/shared/tests/e2e/electron/flows/chat-modals.test.ts b/shared/tests/e2e/electron/flows/chat-modals.test.ts index 1b7915a975de..ffc6036df91d 100644 --- a/shared/tests/e2e/electron/flows/chat-modals.test.ts +++ b/shared/tests/e2e/electron/flows/chat-modals.test.ts @@ -79,13 +79,16 @@ test('bot install preview opens', async ({page}, testInfo) => { await page.getByTestId(T.CHAT_INFO_PANEL).getByText('Bots', {exact: true}).click() const botRows = page.getByTestId(T.CHAT_BOT_ROW) await expect(botRows.first()).toBeVisible({timeout: 10_000}) - await botRows.first().click() - // installed bot: Edit settings; new bot: Install; restricted bot: Review - const installButton = page.getByText(/^(Install|Edit settings|Review)$/).first() - await expect(installButton).toBeVisible({timeout: 10_000}) + // the row center sits on the "by " username link, which navigates to + // that user's profile instead — click the avatar column on the left + await botRows.first().click({position: {x: 20, y: 28}}) + // the footer button depends on the bot's state (Install / Review / Edit + // settings / Uninstall), so assert on the modal instead of a label + const installModal = page.getByTestId(T.CHAT_BOT_INSTALL).first() + await expect(installModal).toBeVisible({timeout: 10_000}) await snap(page, testInfo) await page.locator('.icon-gen-iconfont-close:visible').first().click() - await expect(installButton).not.toBeVisible({timeout: 5_000}) + await expect(installModal).not.toBeVisible({timeout: 5_000}) // close the info panel again await page.locator('.icon-gen-iconfont-info:visible').first().click() await expect(page.getByTestId(T.CHAT_INFO_PANEL)).not.toBeVisible({timeout: 5_000}) diff --git a/shared/tests/e2e/electron/flows/device-wallet-modals.test.ts b/shared/tests/e2e/electron/flows/device-wallet-modals.test.ts index dc70fa11a5e7..7874851024b6 100644 --- a/shared/tests/e2e/electron/flows/device-wallet-modals.test.ts +++ b/shared/tests/e2e/electron/flows/device-wallet-modals.test.ts @@ -20,7 +20,8 @@ test('add device chooser opens', async ({page}, testInfo) => { test('crypto recipients team builder opens', async ({page}, testInfo) => { await navigateToCrypto(page) await page.getByTestId(T.CRYPTO_NAV_ENCRYPT).click() - await page.getByPlaceholder('Search people').locator('visible=true').first().click() + // the "Search people" input itself is pointerEvents:none — click its wrapper + await page.getByTestId(T.CRYPTO_RECIPIENTS).locator('visible=true').first().click() const search = page.getByPlaceholder('Search Keybase').locator('visible=true') await expect(search.first()).toBeVisible({timeout: 5_000}) await snap(page, testInfo) diff --git a/shared/tests/e2e/electron/flows/teams-inner.test.ts b/shared/tests/e2e/electron/flows/teams-inner.test.ts index 31f3577fb2ce..ba0b56f5806d 100644 --- a/shared/tests/e2e/electron/flows/teams-inner.test.ts +++ b/shared/tests/e2e/electron/flows/teams-inner.test.ts @@ -23,6 +23,9 @@ test('members tab renders', async ({page}) => { test.skip() return } + // the team page remembers the last tab per team for the life of the app, so an + // earlier test can leave it on Channels/Settings — select Members explicitly + await page.getByTestId(T.TEAMS_TAB_MEMBERS_BUTTON).locator('visible=true').first().click() await expect(page.getByTestId(T.TEAMS_MEMBER_LIST).first()).toBeVisible({timeout: 5_000}) }) @@ -32,10 +35,10 @@ test('settings tab renders', async ({page}) => { test.skip() return } - // 'Settings' appears in both nav sidebar and team tabs — use .nth(1) for team tab - await page.getByText('Settings', {exact: true}).nth(1).click() - // Verify we're still in the team view (Members tab still visible) - await expect(page.getByText('Members', {exact: true}).first()).toBeVisible({timeout: 5_000}) + await page.getByTestId(T.TEAMS_TAB_SETTINGS_BUTTON).locator('visible=true').first().click() + // assert the settings BODY, not the 'Members' tab label: that label is part of + // the tab bar and is visible on every tab, so it passes without switching + await expect(page.getByTestId(T.TEAMS_SETTINGS_TAB).first()).toBeVisible({timeout: 5_000}) }) test('bots tab renders', async ({page}) => { @@ -45,7 +48,7 @@ test('bots tab renders', async ({page}) => { return } await page.getByText('Bots', {exact: true}).first().click() - await expect(page.getByText('Members', {exact: true}).first()).toBeVisible({timeout: 5_000}) + await expect(page.getByTestId(T.TEAMS_BOTS_TAB).first()).toBeVisible({timeout: 5_000}) }) test('channels tab renders (if big team or admin)', async ({page}) => { @@ -60,5 +63,5 @@ test('channels tab renders (if big team or admin)', async ({page}) => { return } await channelsTab.click() - await expect(page.getByText('Members', {exact: true}).first()).toBeVisible({timeout: 5_000}) + await expect(page.getByTestId(T.TEAMS_CHANNEL_LIST).first()).toBeVisible({timeout: 5_000}) }) diff --git a/shared/tests/e2e/electron/flows/teams-modals.test.ts b/shared/tests/e2e/electron/flows/teams-modals.test.ts index 313eba84a61e..fd69f12bd6ae 100644 --- a/shared/tests/e2e/electron/flows/teams-modals.test.ts +++ b/shared/tests/e2e/electron/flows/teams-modals.test.ts @@ -130,8 +130,19 @@ test('retention warning opens', async ({page}, testInfo) => { test.skip() return } - await dropdown.click() - await page.getByText('7 days', {exact: true}).locator('visible=true').last().click() + // the settings tab re-renders as the team's retention policy lands, which can + // swallow the popup the first click opens — reopen until the menu sticks + const sevenDays = page.getByText('7 days', {exact: true}).locator('visible=true').last() + let menuOpen = false + for (let i = 0; i < 3 && !menuOpen; i++) { + await dropdown.click() + menuOpen = await becomesVisible(sevenDays, 2_000) + } + if (!menuOpen) { + test.skip() + return + } + await sevenDays.click() const confirm = page.getByText('Yes, set to 7 days') if (!(await becomesVisible(confirm, 3_000))) { // no warning fired (already at/below 7 days) — nothing to capture diff --git a/shared/tests/e2e/electron/playwright.config.ts b/shared/tests/e2e/electron/playwright.config.ts index d360d19b20cf..4413da4ccd21 100644 --- a/shared/tests/e2e/electron/playwright.config.ts +++ b/shared/tests/e2e/electron/playwright.config.ts @@ -2,7 +2,9 @@ import {defineConfig} from '@playwright/test' export default defineConfig({ testDir: './', - timeout: 15_000, + // several flows chain 3-4 five-second waits against a live service, so the + // per-test budget has to clear the sum of their step timeouts + timeout: 30_000, retries: 1, workers: 1, outputDir: '../../results/test-results', diff --git a/shared/tests/e2e/shared/test-ids.ts b/shared/tests/e2e/shared/test-ids.ts index 9b4f455421a5..c758337602cd 100644 --- a/shared/tests/e2e/shared/test-ids.ts +++ b/shared/tests/e2e/shared/test-ids.ts @@ -22,6 +22,9 @@ export const CHAT_EMOJI_PICKER = 'chat-emoji-picker' export const CHAT_ATTACHMENT_IMAGE = 'chat-attachment-image' export const CHAT_ATTACHMENT_FULLSCREEN = 'chat-attachment-fullscreen' export const CHAT_BOT_ROW = 'chat-bot-row' +// the install modal's footer button varies with the bot's state (Install / +// Review / Edit settings / Uninstall), so tests key off the modal itself +export const CHAT_BOT_INSTALL = 'chat-bot-install' export const CHAT_SUGGESTION_LIST = 'chat-suggestion-list' export const CHAT_EMOJI_BUTTON = 'chat-emoji-button' export const CHAT_INFO_PANEL_SETTINGS_TAB = 'chat-info-panel-settings-tab' @@ -95,6 +98,9 @@ export const CRYPTO_DECRYPT_INPUT = 'crypto-decrypt-input' export const CRYPTO_SIGN_INPUT = 'crypto-sign-input' export const CRYPTO_VERIFY_INPUT = 'crypto-verify-input' export const CRYPTO_RUN_BUTTON = 'crypto-run-button' +// The recipients field is a display-only input inside a pointerEvents="none" +// wrapper, so only this outer clickable can receive a click. +export const CRYPTO_RECIPIENTS = 'crypto-recipients' // Common — keep value matching existing testID="backButton" in .maestro subflows export const COMMON_BACK_BUTTON = 'backButton' From 196c79109781b8606a338ed5647bc76eb0aaaf43 Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Sun, 26 Jul 2026 15:01:04 -0400 Subject: [PATCH 6/7] fix(stores): clear module-level caches on sign-out, and stop two loops 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. --- shared/common-adapters/hot-key.tsx | 33 ++++++++++++------- shared/common-adapters/markdown/spoiler.tsx | 7 ++++ shared/common-adapters/with-tooltip.tsx | 15 ++++++--- shared/crypto/recipients.tsx | 2 ++ shared/engine/action-listener.test.ts | 20 +++++++++++ shared/engine/action-listener.tsx | 6 +++- shared/fs/browser/root.tsx | 6 ++++ shared/git/index.tsx | 5 +-- shared/login/join-or-login.tsx | 10 +++--- .../router-v2/screen-layout-modal.desktop.tsx | 14 ++++---- shared/util/fs-platform.tsx | 6 ++++ shared/util/use-local-badging.tsx | 16 +++++---- shared/wallets/index.tsx | 4 +-- 13 files changed, 107 insertions(+), 37 deletions(-) diff --git a/shared/common-adapters/hot-key.tsx b/shared/common-adapters/hot-key.tsx index 0eb12d37105a..67d3c95e1ecc 100644 --- a/shared/common-adapters/hot-key.tsx +++ b/shared/common-adapters/hot-key.tsx @@ -202,20 +202,31 @@ const unregisterKeys = (keysArr: Array, cb: (key: string) => void) => { } export function useHotKey(keys: Array | string, cb: (key: string) => void) { - const keysArr = (() => { - const arr = typeof keys === 'string' ? [keys] : keys - return arr.filter(k => k.length > 0) - })() + // callers pass inline arrays/closures, so derive stable identities for both: + // otherwise every render unregisters and re-registers, which also re-orders the + // LIFO stack and lets a re-rendering background screen steal the key + const keysKey = typeof keys === 'string' ? keys : keys.join(',') + const keysArr = React.useMemo(() => keysKey.split(',').filter(k => k.length > 0), [keysKey]) - C.Router2.useSafeFocusEffect(() => { - if (isMobile || keysArr.length === 0) return - registerKeys(keysArr, cb) - return () => unregisterKeys(keysArr, cb) + const cbRef = React.useRef(cb) + React.useEffect(() => { + cbRef.current = cb }) + const [stableCB] = React.useState(() => (key: string) => cbRef.current(key)) + + // registering again on focus re-pushes us to the top of the stack so the + // focused screen wins the key over screens that merely stayed mounted + C.Router2.useSafeFocusEffect( + React.useCallback(() => { + if (isMobile || keysArr.length === 0) return + registerKeys(keysArr, stableCB) + return () => unregisterKeys(keysArr, stableCB) + }, [keysArr, stableCB]) + ) React.useEffect(() => { if (isMobile || keysArr.length === 0) return - registerKeys(keysArr, cb) - return () => unregisterKeys(keysArr, cb) - }, [keysArr, cb]) + registerKeys(keysArr, stableCB) + return () => unregisterKeys(keysArr, stableCB) + }, [keysArr, stableCB]) } diff --git a/shared/common-adapters/markdown/spoiler.tsx b/shared/common-adapters/markdown/spoiler.tsx index 3446391ef879..c643bdb6db2d 100644 --- a/shared/common-adapters/markdown/spoiler.tsx +++ b/shared/common-adapters/markdown/spoiler.tsx @@ -1,6 +1,7 @@ import * as React from 'react' import * as Styles from '@/styles' import Text from '@/common-adapters/text' +import {registerExternalResetter} from '@/util/zustand' type Props = { children: React.ReactNode @@ -10,6 +11,12 @@ type Props = { const spoilerState = new Map() +// module scope outlives sign-out; keyed by message content, and a spoiler the +// previous user revealed must not come up already revealed for the next one +registerExternalResetter('markdown-spoiler-state', () => { + spoilerState.clear() +}) + const Spoiler = (p: Props) => { const {children, content, context} = p const key = `${context ?? ''}:${content}` diff --git a/shared/common-adapters/with-tooltip.tsx b/shared/common-adapters/with-tooltip.tsx index d4722a67af9b..704ec8010d47 100644 --- a/shared/common-adapters/with-tooltip.tsx +++ b/shared/common-adapters/with-tooltip.tsx @@ -62,11 +62,16 @@ function WithTooltip(p: Props) { }, 3000) const {width: screenWidth, height: screenHeight} = useSafeAreaFrame() - C.Router2.useSafeFocusEffect(() => { - return () => { - setNativeVisible(false) - } - }) + // must be stable: an unstable callback re-runs the focus effect (and so its + // cleanup) on every render, which would hide the tooltip as soon as showing it + // re-renders us + C.Router2.useSafeFocusEffect( + React.useCallback(() => { + return () => { + setNativeVisible(false) + } + }, []) + ) if (!isMobile) { const onMouseEnter = () => setDesktopVisible(true) diff --git a/shared/crypto/recipients.tsx b/shared/crypto/recipients.tsx index 1e4aefa93633..7255a968dcf4 100644 --- a/shared/crypto/recipients.tsx +++ b/shared/crypto/recipients.tsx @@ -1,4 +1,5 @@ import * as Kb from '@/common-adapters' +import * as TestIDs from '@/tests/e2e/shared/test-ids' type Props = { inProgress: boolean @@ -22,6 +23,7 @@ const Recipients = ({inProgress, onAddRecipients, onClearRecipients, recipients} {/* Display-only input; block it from taking focus so opening the diff --git a/shared/engine/action-listener.test.ts b/shared/engine/action-listener.test.ts index 3bb0eeb3df42..1242ef301b1f 100644 --- a/shared/engine/action-listener.test.ts +++ b/shared/engine/action-listener.test.ts @@ -64,3 +64,23 @@ test('clearAll removes all registered listeners', () => { expect(homeListener).not.toHaveBeenCalled() }) + +test('a stale unsubscribe from before a reset leaves later subscribers alone', () => { + const before = jest.fn() + const staleUnsubscribe = subscribeToEngineAction('keybase.1.homeUI.homeUIRefresh', before) + + clearAllEngineActionListeners() + + const after = jest.fn() + subscribeToEngineAction('keybase.1.homeUI.homeUIRefresh', after) + + // the component that subscribed before the reset unmounts late + staleUnsubscribe() + + notifyEngineActionListeners({ + payload: {params: {}}, + type: 'keybase.1.homeUI.homeUIRefresh', + } as never) + expect(after).toHaveBeenCalledTimes(1) + expect(before).not.toHaveBeenCalled() +}) diff --git a/shared/engine/action-listener.tsx b/shared/engine/action-listener.tsx index 59fe268bf08e..7c552f915e7c 100644 --- a/shared/engine/action-listener.tsx +++ b/shared/engine/action-listener.tsx @@ -31,7 +31,11 @@ export const subscribeToEngineAction = ( listeners.add(untypedListener) return () => { listeners.delete(untypedListener) - if (!listeners.size) { + // Only drop the entry if the map still holds THIS set. A reset replaces the + // set for a type, so an unsubscribe left over from before the reset would + // otherwise see its own detached, now-empty set and delete the live one, + // silently unsubscribing everybody who registered after the reset. + if (!listeners.size && listenersByType.get(type) === listeners) { listenersByType.delete(type) } } diff --git a/shared/fs/browser/root.tsx b/shared/fs/browser/root.tsx index 4ac9ff4c31ab..3832fdd3786b 100644 --- a/shared/fs/browser/root.tsx +++ b/shared/fs/browser/root.tsx @@ -7,6 +7,7 @@ import SfmiBanner from '../banner/system-file-manager-integration-banner/contain import {WrapRow} from './rows/rows' import * as FS from '@/constants/fs' import {useCurrentUserState} from '@/stores/current-user' +import {registerExternalResetter} from '@/util/zustand' type Props = { destinationPickerSource?: T.FS.MoveOrCopySource | T.FS.IncomingShareSource @@ -57,6 +58,11 @@ const renderSectionHeader = ({section}: {section: {key: string; title: string}}) // recents are re-derived on every FsDataContext write; reuse item identities // so the memoized rows can bail const recentItemCache = new Map() + +// module scope outlives sign-out; keyed by TLF name +registerExternalResetter('fs-browser-recent-item-cache', () => { + recentItemCache.clear() +}) const canonRecentItem = (name: string, tlfType: T.FS.TlfType): SectionListItem => { const key = `${tlfType}-${name}` let item = recentItemCache.get(key) diff --git a/shared/git/index.tsx b/shared/git/index.tsx index a5441c6f5c4e..3e9f62a70ceb 100644 --- a/shared/git/index.tsx +++ b/shared/git/index.tsx @@ -101,13 +101,14 @@ const GitRoot = (ownProps: OwnProps) => { // errors from row actions (toggling chat), load errors come from the hook const [rowError, setRowError] = React.useState() const isNew = useConfigState(s => s.badgeState?.newGitRepoGlobalUniqueIDs) - const {badged} = useLocalBadging(new Set(isNew ?? []), () => { + const clearBadges = React.useCallback(() => { clearGitBadges( [{category: 'new_git_repo'}], () => {}, () => {} ) - }) + }, [clearGitBadges]) + const {badged} = useLocalBadging(new Set(isNew ?? []), clearBadges) const { data, error: loadError, diff --git a/shared/login/join-or-login.tsx b/shared/login/join-or-login.tsx index 1b77007db2ce..9aec72bd3767 100644 --- a/shared/login/join-or-login.tsx +++ b/shared/login/join-or-login.tsx @@ -26,10 +26,12 @@ const Intro = () => { const [showing, setShowing] = React.useState(true) Kb.useInterval(loadIsOnline, showing ? 5000 : undefined) - C.Router2.useSafeFocusEffect(() => { - setShowing(true) - return () => setShowing(false) - }) + C.Router2.useSafeFocusEffect( + React.useCallback(() => { + setShowing(true) + return () => setShowing(false) + }, []) + ) return ( { const [topMostModal, setTopMostModal] = React.useState(true) - C.Router2.useSafeFocusEffect(() => { - setTopMostModal(true) - return () => { - setTopMostModal(false) - } - }) + C.Router2.useSafeFocusEffect( + React.useCallback(() => { + setTopMostModal(true) + return () => { + setTopMostModal(false) + } + }, []) + ) React.useEffect(() => { if (!topMostModal || overlayNoClose) return diff --git a/shared/util/fs-platform.tsx b/shared/util/fs-platform.tsx index 3d7d90baba58..ec5479e3462f 100644 --- a/shared/util/fs-platform.tsx +++ b/shared/util/fs-platform.tsx @@ -11,6 +11,7 @@ import {launchImageLibraryAsync} from '@/util/expo-image-picker' import {pickDocumentsAsync} from '@/util/expo-document-picker.native' import {saveAttachmentToCameraRoll, showShareActionSheet} from '@/util/platform-specific' import {fsCacheDir, fsDownloadDir, androidAddCompleteDownload} from 'react-native-kb' +import {registerExternalResetter} from '@/util/zustand' // Desktop-only exports (stubs on mobile) export const fuseStatusToDriverStatus = (status?: T.RPCGen.FuseStatus): T.FS.DriverStatus => { @@ -301,6 +302,11 @@ export const afterKbfsDaemonRpcStatusChangedMobile = async () => { const finishedRegularDownloadIDs = new Set() +// module scope outlives sign-out; holds the previous user's download ids +registerExternalResetter('fs-finished-regular-downloads', () => { + finishedRegularDownloadIDs.clear() +}) + export const finishedRegularDownloadMobile = async ( downloadID: string, downloadState: T.FS.DownloadState, diff --git a/shared/util/use-local-badging.tsx b/shared/util/use-local-badging.tsx index 31947c08304a..35febbf7e35f 100644 --- a/shared/util/use-local-badging.tsx +++ b/shared/util/use-local-badging.tsx @@ -26,12 +26,16 @@ export const useLocalBadging = (storeSet: ReadonlySet | undefined, clear }) } - C.Router2.useSafeFocusEffect(() => { - clearStoreBadges() - return () => { - setBadged(noBadges) - } - }) + // clearStoreBadges must be stable in the caller: an unstable identity re-runs + // this focus effect every render, clearing the badges on the server repeatedly + C.Router2.useSafeFocusEffect( + React.useCallback(() => { + clearStoreBadges() + return () => { + setBadged(noBadges) + } + }, [clearStoreBadges]) + ) return {badged} } diff --git a/shared/wallets/index.tsx b/shared/wallets/index.tsx index 01e9dc11d766..7f84beabc539 100644 --- a/shared/wallets/index.tsx +++ b/shared/wallets/index.tsx @@ -118,7 +118,7 @@ const WalletsScreen = () => { const loadAccounts = C.useRPC(T.RPCStellar.localGetWalletAccountsLocalRpcPromise) C.Router2.useSafeFocusEffect( - () => { + React.useCallback(() => { loadAccounts( [undefined, loadAccountsWaitingKey], res => { @@ -138,7 +138,7 @@ const WalletsScreen = () => { } ) return () => {} - } + }, [loadAccounts, checkDisclaimer]) ) const loading = C.Waiting.useAnyWaiting(loadAccountsWaitingKey) From a913d598f89c68c69f83508e44179858db9c43ed Mon Sep 17 00:00:00 2001 From: chrisnojima Date: Sun, 26 Jul 2026 19:32:15 -0400 Subject: [PATCH 7/7] fix(teams): order in-flight loads by counter, not Date.now 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. --- shared/teams/use-cached-resource.test.tsx | 43 ++++++++++++++++++++++- shared/teams/use-cached-resource.tsx | 28 +++++++++------ 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/shared/teams/use-cached-resource.test.tsx b/shared/teams/use-cached-resource.test.tsx index c546c4225108..48ded3bda3e0 100644 --- a/shared/teams/use-cached-resource.test.tsx +++ b/shared/teams/use-cached-resource.test.tsx @@ -4,10 +4,14 @@ import {expect, jest, test} from '@jest/globals' import {act, render} from '@testing-library/react' import {createCachedResourceCache, useCachedResource} from './use-cached-resource' +// A setState that lands outside act() - every load settling on its own - is +// scheduled on React's MessageChannel, i.e. a macrotask. A flush built only from +// awaited microtasks never lets the event loop reach it, so the commit lands (or +// not) depending on how the runtime happens to interleave: use a real timer. const flush = async (turns = 40) => { for (let i = 0; i < turns; i++) { await act(async () => { - await Promise.resolve() + await new Promise(resolve => setTimeout(resolve, 0)) }) } } @@ -200,6 +204,43 @@ test('a forced reload supersedes a request that predates it', async () => { expect(cache.getData()).toEqual({v: 2}) }) +// The mutation and the reload it triggers routinely fall inside one millisecond, +// so ordering the two by Date.now() compares them equal and the forced load +// joins the very request it exists to supersede. +test('a forced reload supersedes a same-millisecond request', async () => { + const cache = createCachedResourceCache({v: 0}, 'k') + let calls = 0 + const releases: Array<(v: Data) => void> = [] + const load = jest.fn(async () => { + calls++ + return await new Promise(resolve => { + releases.push(resolve) + }) + }) + let reload: (() => Promise) | undefined + const Comp = () => { + 'use no memo' + const resource = useCachedResource({cache, cacheKey: 'k', initialData: {v: 0}, load, staleMs: 5000}) + reload = resource.reload + return
{resource.loaded ? `v${resource.data.v}` : 'pending'}
+ } + const realNow = Date.now + const frozen = realNow() + Date.now = () => frozen + try { + render() + await flush() + expect(calls).toBe(1) + act(() => { + void reload?.() + }) + await flush() + expect(calls).toBe(2) + } finally { + Date.now = realNow + } +}) + // Two consumers mounting together must share one request, not race two. This is // the property the module-level caches depend on: without it, sharing a cache // across screens still issues an RPC per screen. diff --git a/shared/teams/use-cached-resource.tsx b/shared/teams/use-cached-resource.tsx index e84925b410ad..9cf630d902ff 100644 --- a/shared/teams/use-cached-resource.tsx +++ b/shared/teams/use-cached-resource.tsx @@ -8,7 +8,7 @@ export type CachedResourceCache = { getFailedAt: () => number getGeneration: () => number getInFlight: () => Promise | undefined - getInFlightStartedAt: () => number + getInFlightSeq: () => number getKey: () => K getLoadedAt: () => number invalidate: (key: K) => void @@ -25,6 +25,14 @@ export type CachedResourceCache = { // speed. An explicit reload()/invalidate() still retries immediately. const loadFailureBackoffMs = 5_000 +// Ordering of "was this request already on the wire when I asked?" cannot use +// Date.now(): a mutation and the reload it triggers routinely land in the same +// millisecond, and the two timestamps then compare equal, so the forced load +// joins the very request it must supersede. A counter is exact. +let inFlightSequence = 0 +const nextInFlightSeq = () => ++inFlightSequence +const currentInFlightSeq = () => inFlightSequence + type CachedResourceState = { data: T loaded: boolean @@ -96,7 +104,7 @@ export const createCachedResourceCache = (initialData: T, key: K): CachedR let failedAt = 0 let generation = 0 let inFlight: Promise | undefined - let inFlightStartedAt = 0 + let inFlightSeq = 0 let loadedAt = 0 let storedKey = key @@ -110,7 +118,7 @@ export const createCachedResourceCache = (initialData: T, key: K): CachedR getFailedAt: () => failedAt, getGeneration: () => generation, getInFlight: (): Promise | undefined => inFlight, - getInFlightStartedAt: () => inFlightStartedAt, + getInFlightSeq: () => inFlightSeq, getKey: () => storedKey, getLoadedAt: () => loadedAt, invalidate: nextKey => { @@ -137,7 +145,7 @@ export const createCachedResourceCache = (initialData: T, key: K): CachedR }, setInFlight: request => { inFlight = request - inFlightStartedAt = Date.now() + inFlightSeq = nextInFlightSeq() }, setLoadFailed: requestGeneration => { if (generation === requestGeneration) { @@ -171,15 +179,15 @@ const runLoad = async ( requestVersionRef: React.RefObject, setState: React.Dispatch>>, force: boolean, - requestedAt: number + requestedSeq: number ) => { let request: Promise | undefined // A forced load exists because something changed, so joining a request that // was already on the wire before we asked would settle to pre-change data - // and stamp loadedAt on it, holding it stale for the whole window. Only join a - // request that started at or after this one was asked for, which still - // collapses the N-mounted-consumers-reload-together case to one RPC. - const staleInFlight = force && cache.getInFlightStartedAt() < requestedAt + // request that started after this one was asked for, which still collapses the + // N-mounted-consumers-reload-together case to one RPC. + const staleInFlight = force && !!cache.getInFlight() && cache.getInFlightSeq() <= requestedSeq if (staleInFlight) { cache.invalidate(cacheKey) } @@ -274,7 +282,7 @@ export const useCachedResource = (props: Props) => { const loadResource = React.useCallback(async (force: boolean) => { // stamped before any await so a forced load can tell an in-flight request // that predates it from one issued in response to the same change - const requestedAt = Date.now() + const requestedSeq = currentInFlightSeq() const {cache, cacheKey, enabled, initialData, load, onError, staleMs} = latestRef.current const resetCache = (nextKey: K) => { cache.reset(initialData, nextKey) @@ -315,7 +323,7 @@ export const useCachedResource = (props: Props) => { requestVersionRef, setState, force, - requestedAt + requestedSeq ) }, [])