fix(web): speed up PR previews with fewer GitHub requests - #11825
Conversation
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR introduces a new authenticated pull-request preview RPC and changes provider, caching, client-state, and hover-link behavior across the production path. It also touches authorization code and has unresolved risks involving Forgejo host resolution and preview-cache freshness, so the change is not limited to a low-risk performance tweak. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
|
Both correctness findings in the Macroscope summary above are fixed in 868c761, with replies on each review thread. The preview now respects the detail cache TTL without awaiting pending detail, and Forgejo links preserve their HTTP authority. The expiry regression failed before the fix; all 309 focused tests and CI on the latest commit pass. Both threads are resolved. The summary currently reflects the previous commit, 3ef5185. |
|
@coderabbitai review |
|
@macroscope-app review the latest commit, 868c761. Both findings from the previous review have been fixed and verified; the review threads contain the details. |
|
Sorry, I'm unable to act on this request because you do not have permissions within this repository. |
1 similar comment
|
Sorry, I'm unable to act on this request because you do not have permissions within this repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (16)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThis change adds a lightweight pull request preview contract and RPC. The server provides GitHub preview data with caching and fallback behavior. The client routes and refreshes preview queries, then uses them before loading full details. ChangesPull request preview flow
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant User
participant PullRequestLinkPreview
participant PullRequestService
participant GitHubPullRequestProvider
participant GitHubPullRequestCli
participant GitHub
User->>PullRequestLinkPreview: hover or focus link
PullRequestLinkPreview->>PullRequestService: request preview
PullRequestService->>GitHubPullRequestProvider: getChangeRequestPreview
GitHubPullRequestProvider->>GitHubPullRequestCli: getPullRequestPreview
GitHubPullRequestCli->>GitHub: GraphQL preview query
GitHub-->>GitHubPullRequestCli: preview fields
GitHubPullRequestCli-->>PullRequestService: decoded preview
PullRequestService-->>PullRequestLinkPreview: preview data
PullRequestLinkPreview->>PullRequestService: warm full detail
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The preview flow, cache behavior, authorization, routing, and concurrent reads have no confirmed remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
❌ Action failedReview failed.
|
|
@coderabbitai The review summary covers 3ef5185; the latest commit is 868c761, which fixes preview cache expiry and Forgejo ports and adds regression coverage. The 80% docstring warning conflicts with this repository’s AGENTS.md guidance to document non-obvious reasons rather than narrate clear code. I have retained the comments explaining preview scope and warming, and the executable regression tests, without adding boilerplate to meet a percentage. Please assess the new commit when the incremental review runs. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- commit ---'
git show --no-ext-diff --format=fuller --stat 868c7610e --
printf '%s\n' '--- relevant file outlines ---'
ast-grep outline apps/server/src/pullRequest/PullRequestService.ts --items all --match 'preview|detail|cache|invalidate|refresh' || true
ast-grep outline apps/web/src/components/ChatMarkdown.tsx --items all --match 'PullRequest|pullRequest|Preview' || true
ast-grep outline apps/server/src/pullRequest/PullRequestService.test.ts --items all --match 'preview|detail|expire|TTL|Forgejo' || true
printf '%s\n' '--- patch ---'
git show --no-ext-diff --format= --find-renames 868c7610e -- \
apps/server/src/pullRequest/PullRequestService.ts \
apps/server/src/pullRequest/PullRequestService.test.ts \
apps/web/src/components/ChatMarkdown.tsx
printf '%s\n' '--- guidance ---'
fd -HI '^AGENTS\.md$' . -x sh -c 'echo "--- $1"; cat "$1"' sh {} \;Length of output: 144146 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- cache and preview references ---'
rg -n -C 5 '\b(detailCache|previewCache|lastGoodDetail|refCacheKey|Cache\.getSuccess|const preview)\b' \
apps/server/src/pullRequest/PullRequestService.ts
printf '%s\n' '--- cache helper definitions and call sites ---'
rg -n -C 4 '\bgetSuccess\b|\bexport.*Cache\b|\bnamespace Cache\b|\bclass Cache\b' \
apps/server packages --glob '*.{ts,tsx}' | head -300
printf '%s\n' '--- Forgejo test context ---'
sed -n '1815,1955p' apps/server/src/pullRequest/PullRequestService.test.ts
printf '%s\n' '--- link candidate authority and preview consumers ---'
rg -n -C 5 '\b(authority|pullRequestCandidate|preview)\b' \
apps/web/src/components/ChatMarkdown.tsx \
apps/web/src/components/pullRequest/PullRequestLinkPreview.tsx \
packages/client-runtime/src/state/pullRequests.ts
printf '%s\n' '--- relevant cache implementation candidates ---'
fd -HI '.*[Cc]ache.*\.(ts|tsx)$' apps packages | head -100Length of output: 48760 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- service cache configuration ---'
sed -n '2420,2485p' apps/server/src/pullRequest/PullRequestService.ts
sed -n '2665,2778p' apps/server/src/pullRequest/PullRequestService.ts
printf '%s\n' '--- cache API implementation ---'
rg -n -C 8 'getSuccess\s*[=:]' .repos/effect-smol packages apps \
--glob '*.{ts,tsx}' | head -240
printf '%s\n' '--- PullRequestService test helper ---'
rg -n -C 8 '\bfunction fakeProvider\b|\bconst fakeProvider\b|\bfakeProvider\s*=' \
apps/server/src/pullRequest/PullRequestService.test.ts
printf '%s\n' '--- TTL declaration ---'
rg -n -C 3 '\bDETAIL_CACHE_TTL\b' apps/server/src/pullRequest/PullRequestService.ts
printf '%s\n' '--- parsed authority type and function ---'
rg -n -C 5 '\bparseChangeRequestUrl\b|\bauthority\b' \
packages apps --glob '*.{ts,tsx}' | head -220Length of output: 37085
The preview path now uses The Forgejo path now passes The docstring-percentage finding does not match the root ✏️ Learnings added
You are interacting with an AI system. |
2f3b3ff to
bf0fdf1
Compare
bf0fdf1 to
05efdd3
Compare
|
Has the GitHub rate-limit impact of this been evaluated and tested? Every pointer-enter and focus on a PR link now issues a GraphQL query (plus the warm-up detail read once the card opens), where before nothing left the client until the card actually opened. The client caches for 60s and the server for 15s, so a single hover is cheaper than the old detail fan-out, but the prefetch fires for every link a pointer crosses, so a scroll through a thread full of PR links can now cost a burst of requests that never happened before. I already hit GitHub rate limits fairly often in day-to-day use, and we have a broader problem to fix there first. I don't want to add more pressure on top of that before that's sorted. Could you share what you measured: how many GraphQL points a hover-heavy session costs before vs. after, and whether you tested it against a session that's already close to the limit (does the budget helper's reserve kick in and degrade gracefully, or do hovers start failing)? |
|
@juliusmarminge Updated to reuse the optimized detail fetch on click. Against main, median hover is 4.87s → 2.85s (41% faster) and fresh opens are 4.56s → 2.93s (36% faster). Opening after hover stays around 0.3s on both (317ms → 305ms across five runs). Three hovers use 9 GraphQL requests / 9 points → 4 / 4; hovering then opening uses 11 / 25 → 6 / 20. Crossing links still triggers zero requests. Reserve/fallback checks pass; measurements and reproduction scripts are in the description. |
Merges `pingdotgg/t3code` at `5378f87f9` into the fork, 51 commits from base `994654198`. `4232` files landed against `4233` in the upstream range; the gap of one is `apps/server/src/cli/pair.ts`, which this fork deletes on purpose. Fork delta afterwards: `777` files. ## Usable as-is Nothing here needs Moatless backend or deployment work. - **Client spans reach the trace proxy again** (pingdotgg#12332). Upstream rebuilt the fork's own `ClientTracingLive` as `apps/web/src/observability/clientTracer.ts` — same behaviour, upstream's name — so the fork delta retired into it. `clientTracing.ts` and `lib/runtime.ts` are byte-identical to upstream again. - **Sidebar search matches message content** (pingdotgg#11761), with a new `ThreadSearchMatch` component and the logic moved out of the command palette. - **A file-to-symlink type change no longer crashes the diff view** (pingdotgg#11075). - **Obsolete code removed** (pingdotgg#9917). This deleted `SidebarGroupLabel` from `components/ui/sidebar.tsx`; the fork's `SettingsSidebarNav` was its only caller, so the label is now inlined there rather than re-exported from an upstream-owned file. - **Build fixes**: executable imports parsed without matching source strings (pingdotgg#12488), and multiple license notices retained for one package (pingdotgg#12489) — the second sits on the `vp build` path this fork's image workflow runs. - **Dependencies**: Effect rc.115 and Alchemy beta.78 with their reference sync (pingdotgg#12326, pingdotgg#12327), plus two security bumps of vulnerable transitives (pingdotgg#12417, pingdotgg#12411). - **`test-t3-app` rewritten around the desktop Browser panel** (pingdotgg#12414). Taken whole with the fork's scope note re-applied. Not applicable rather than usable, listed so the next merge does not re-derive them: the relay deploy and client-config work (pingdotgg#12401, pingdotgg#12484, pingdotgg#12518, pingdotgg#12519) and the CI label/report automation (pingdotgg#12517, pingdotgg#12492) belong to infrastructure this fork does not run — every inherited workflow here is `disabled_manually`. ## Unsupported in Moatless / needs implementation - **Sort pull requests by what is blocked on me** (pingdotgg#12508, `apps/web/src/components/pullRequest/pullRequestList.logic.ts`). Needs `pullRequests.list`, `detail` and `activity`, which the backend does not dispatch. `FEATURES.pullRequestSurface` is `false`, so the route this lands in is not reachable here; Moatless serves `pullRequests.summary` and nothing else in the family. The server half of the same surface is pingdotgg#11825, below. - **View and control agent devices from mobile** (pingdotgg#12531, `apps/mobile/src/features/devices/`). A device panel driven by a device stream brokered by the bundled server between a client and a registered device. Moatless has no device registry and device pairing is decided out in this fork, so the whole path — registration, stream transport, control commands — is backend work. - **The mobile client generally.** Twenty-two further mobile changes landed in this range — pull-to-refresh, native settings and snooze controls, model favourites, project search, platform header and menu splits, Live Activity and Material You import isolation, notification and permission delegate synchronization, copy-thread-id. They are in the tree and typecheck, but whether this fork's mobile client can reach a Moatless backend at all is still unverified; see `docs/fork/gaps.md`, _Mobile testing against Moatless is undocumented because it is unverified_, which this merge extended. - **ACP SDK elicitation requests** (pingdotgg#11294, `packages/effect-acp/src/{client,protocol,rpc}.ts`). Elicitation is an agent-to-client request: the agent asks the user for input mid-turn and blocks on the answer. Moatless drives its own agents rather than hosting upstream's ACP adapters, so the round-trip has to exist on the backend before any client surface can render it. ## Backend behavior to consider reproducing in Moatless Nine server-side fixes, all recorded in `docs/fork/gaps.md` under _Runtime fixes upstream made to its own server_ with the file each lives in: - **An oversized pull request diff should not be cached** (pingdotgg#12523) — 512 KiB cap on cached patch text, with invalidation of an entry already held. A capacity-bounded cache with no size bound is how one enormous PR pins memory. - **Checkpoint git commands should be retried on a transient failure** (pingdotgg#11665) — `…lock: file exists` and `no such file or directory` classified as retryable and retried twice at 75 ms. The race is an agent writing files while a checkpoint is captured, which a sandbox makes more likely. - **A failed settings write should roll its secret changes back** (pingdotgg#12487) — otherwise a persistence failure leaves a provider key removed with nothing to restore it from, and nothing says so until the provider is next used. - **A fetch failure should be explained without echoing the remote** (pingdotgg#12485) — four recognised stderr shapes mapped to fixed sentences, anything else left generic, because fetch stderr can carry credentials from the remote URL into a persisted error. - **A branch switch should not be readable as a path checkout** (pingdotgg#10574) — one `--` appended to `git checkout <ref>`, with losing uncommitted work behind it. - **Rate limits from a tolerated read should still be recorded** (pingdotgg#12486). Bitbucket is not a fork target; the shape is — the budget was spent whether or not the caller wanted the answer. - **An evicted preview host should be able to register again** (pingdotgg#12535) — completes the RPC stream instead of shutting the queue down, so a desktop that was merely slow can re-register. Follows pingdotgg#11381 from the 2026-09-16 merge. The client half landed here in `packages/client-runtime`. - **A server should export log records, not only traces and metrics** (pingdotgg#12493) — `otlpLogsUrl` plus a shared `otlpResource`, which is what makes the three signals joinable at the collector. The fork already exports client spans. - **Pull request reads should be batched rather than fanned out** (pingdotgg#11825) — far fewer GitHub requests per preview, with a measurement script. Moatless does its own GitHub reads behind `pullRequests.summary`. ## Merge notes Five conflicts, each resolved with the verdict `preflight.mjs` printed. The one that needed thought was `apps/web/src/lib/runtime.ts`: pingdotgg#12332 reimplemented the fork's tracer layer upstream and, in the same change, removed the `activeDelegate` binding the fork's layer read — so the fork block auto-merged into `clientTracing.ts` referencing a symbol that no longer existed. Resolved by converging onto upstream rather than repairing the fork copy. Two inventory gaps this merge closed: `apps/server/src/bin.ts` had no path-policy entry despite holding the only references to the deleted `cli/pair.ts` (now `server-cli-entrypoint`, `converged`), and the fork's own `typecheck.yml` was missing from `offRepo.allowedActiveWorkflows`, which made `tripwires.mjs` report it as an inherited workflow switched back on. `unsupported-methods.mjs` reported ADD 0 / DROP 0 — no change to `packages/contracts/src/rpc.ts`. `verify.mjs`: all 10 checks green on the final full pass, tests included — 334 files, 5144 tests. Tripwires: Clerk 4, pairing 96, session bootstrap 8, 5 known deletions, 4 active workflows. Tracker entry: `docs/fork/upstream-merge-log.md`, 2026-09-19. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- Moatless task: https://moatless.soaplabstest.com/tasks/0a5d08b0-0bd4-412e-a837-782ac67e5a13
## What's Changed * fix(mobile): use singular label for one settings environment by @juliusmarminge in pingdotgg/t3code#12282 * feat(mobile): add copy thread ID to thread list actions by @jakeleventhal in pingdotgg/t3code#12228 * fix(mobile): remove Android input underline backgrounds by @juliusmarminge in pingdotgg/t3code#12394 * chore(deps): upgrade Effect to rc.115 and Alchemy to beta.78 by @juliusmarminge in pingdotgg/t3code#12326 * chore(refs): sync Effect and Alchemy references to rc.115 and beta.78 by @juliusmarminge in pingdotgg/t3code#12327 * chore(relay): deploy with the Alchemy CLI and publish client config through an Action by @juliusmarminge in pingdotgg/t3code#12401 * chore(deps): bump the npm_and_yarn group across 1 directory with 3 updates by @dependabot[bot] in pingdotgg/t3code#12411 * fix(git): prevent stale branch selections from restoring files by @yashranaway in pingdotgg/t3code#10574 * chore(deps): bump parents that carry vulnerable transitive dependencies by @juliusmarminge in pingdotgg/t3code#12417 * fix(web): keep a file-to-symlink type change from crashing the diff view by @Mnigos in pingdotgg/t3code#11075 * Use T3 Device panel for mobile testing by @juliusmarminge in pingdotgg/t3code#12414 * fix(web): client spans reach the trace proxy again by @yordis in pingdotgg/t3code#12332 * fix(bitbucket): preserve rate limits from optional PR reads by @juliusmarminge in pingdotgg/t3code#12486 * fix(mobile): synchronize native permission registry access by @juliusmarminge in pingdotgg/t3code#12482 * fix(build): retain multiple license notices for one package by @juliusmarminge in pingdotgg/t3code#12489 * fix(build): parse executable imports without matching source strings by @juliusmarminge in pingdotgg/t3code#12488 * fix(mobile): synchronize native notification delegates by @juliusmarminge in pingdotgg/t3code#12483 * fix(relay): accept delegated thread IDs in activity routes by @juliusmarminge in pingdotgg/t3code#12484 * fix(git): explain fetch failures without exposing remote output by @juliusmarminge in pingdotgg/t3code#12485 * fix(web): sidebar search matches message content by @koushikxd in pingdotgg/t3code#11761 * fix(server): restore secrets when settings persistence fails by @juliusmarminge in pingdotgg/t3code#12487 * fix(ci): accept V2 transfer reports without cross-scenario comparisons by @juliusmarminge in pingdotgg/t3code#12492 * fix(web): speed up PR previews with fewer GitHub requests by @dominic-r in pingdotgg/t3code#11825 * fix(server): retry transient git failures during checkpoint capture by @saphid in pingdotgg/t3code#11665 * fix(mobile): keep archived threads visible during iOS search by @juliusmarminge in pingdotgg/t3code#12420 * perf(mobile): isolate Material You conversion on Android by @juliusmarminge in pingdotgg/t3code#12379 * perf(mobile): isolate iOS Live Activity imports by @juliusmarminge in pingdotgg/t3code#12380 * refactor(mobile): split home headers by platform by @juliusmarminge in pingdotgg/t3code#12381 * refactor(mobile): split native menus by platform by @juliusmarminge in pingdotgg/t3code#12382 * refactor(mobile): isolate thread row appearance by platform by @juliusmarminge in pingdotgg/t3code#12383 * refactor(mobile): split settings selection rows by platform by @juliusmarminge in pingdotgg/t3code#12384 * refactor(mobile): centralize platform header rendering by @juliusmarminge in pingdotgg/t3code#12388 * refactor(mobile): configure thread headers through the shared core by @juliusmarminge in pingdotgg/t3code#12389 * refactor(mobile): share file header actions and search configuration by @juliusmarminge in pingdotgg/t3code#12390 * refactor(mobile): share terminal header and menu configuration by @juliusmarminge in pingdotgg/t3code#12391 * refactor(mobile): share archived thread header configuration by @juliusmarminge in pingdotgg/t3code#12399 * refactor(mobile): compose review menus through the shared header by @juliusmarminge in pingdotgg/t3code#12400 * feat(mobile): search projects when starting a task by @juliusmarminge in pingdotgg/t3code#12496 * fix(mobile): preserve multiple model favorites by @juliusmarminge in pingdotgg/t3code#12505 * feat(server): export log records over OTLP by @yordis in pingdotgg/t3code#12493 * fix(mobile): use native settings and snooze controls by @juliusmarminge in pingdotgg/t3code#12512 * feat(web): sort pull requests by what is blocked on me by @flamboh in pingdotgg/t3code#12508 * fix(mobile): prefer pull-to-refresh on list screens by @juliusmarminge in pingdotgg/t3code#12515 * fix(acp): accept SDK elicitation requests by @shivamhwp in pingdotgg/t3code#11294 * fix(release): read relay configuration without loading deployment providers by @juliusmarminge in pingdotgg/t3code#12518 * fix(ci): reconcile native change labels against pinned commits by @juliusmarminge in pingdotgg/t3code#12517 * fix(release): strip Alchemy progress before parsing relay state by @juliusmarminge in pingdotgg/t3code#12519 * refactor: remove obsolete code by @t3dotgg in pingdotgg/t3code#9917 * fix(server): release oversized pull request diff cache entries by @juliusmarminge in pingdotgg/t3code#12523 * feat(mobile): view and control agent devices by @juliusmarminge in pingdotgg/t3code#12531 * fix(preview): recover host registration after request timeouts by @juliusmarminge in pingdotgg/t3code#12535 * fix(mobile): align built-in theme colors with desktop by @juliusmarminge in pingdotgg/t3code#12534 * feat(desktop): export main process telemetry over OTLP by @yordis in pingdotgg/t3code#12520 * fix(codex): surface app permission requests as approvable by @Exotic209093 in pingdotgg/t3code#7861 * chore(desktop): leave main process metrics export off until a metric exists by @juliusmarminge in pingdotgg/t3code#12540 * fix(release): drop placeholder allowBuilds entry that broke desktop builds by @juliusmarminge in pingdotgg/t3code#12544 ## New Contributors * @dependabot[bot] made their first contribution in pingdotgg/t3code#12411 * @koushikxd made their first contribution in pingdotgg/t3code#11761 **Full Changelog**: pingdotgg/t3code@v0.0.43-nightly.20260918.1895...v0.0.43-nightly.20260919.1948 Upstream release: https://github.com/pingdotgg/t3code/releases/tag/v0.0.43-nightly.20260919.1948
Port of pingdotgg#11825 to this codebase: share detail data between hover and click, and combine the permission queries into one GraphQL read. Server: getChangeRequest fired three gh subprocesses (pr view, repo view, viewer-permissions GraphQL). The viewer-permissions query now also selects mergeCommitAllowed/squashMergeAllowed/rebaseMergeAllowed, so one gh api graphql replaces gh repo view plus the second permissions query (3 reads -> 2). Removes getRepositoryAccess, REPOSITORY_ACCESS_JSON_FIELDS, and decodeRepositoryAccessJson. Web: rows prefetch the detail query on intentional hover (350ms delay, so crossing rows costs nothing) and at once on keyboard focus. The click then reads from the react-query cache. Only the detail is warmed: the activity's review-thread walk is paginated and unbounded, while the detail is one consolidated read.
PR hover cards appear 41% sooner, fresh PR opens are 36% faster, and hovering three PRs uses 56% fewer GraphQL requests and points than main. Opening after hover stays around 0.3s on both.
An intentional hover fetches full detail after the existing 350 ms delay and reuses it on click. One GraphQL query combines core detail, permissions, merge settings, checks and branch comparison. Both workflow approval safety checks remain; large check lists retain pagination. Crossing links triggers no requests.
Compared actual main
9ea9c3d5dwith this PRf4d4be88dusing separate checkouts, production web bundles, fresh browser/server caches and the same real GitHub PRs (#11825, #11806, #11990). Five matched hover sessions and three fresh-click sessions, alternating order; September 18.Post-hover opening ranges overlap: main 260–355 ms, PR 298–352 ms. Treat this as comparable cached opening latency. Click timing ends when full-detail tabs appear; activity may finish later.
Open-session totals include activity and stack reads. Setup traffic is excluded equally. Counts include requests inside
gh; GraphQL costs userateLimit.cost, replaying CLI-generated queries after timing. The earlier 10-point three-hover baseline was corrected to 9: a REST quota probe had been counted as a GraphQL point.Measurements, ranges and samples · Reproduction scripts, fixture and captured queries
477 focused tests pass; server/web typechecks, targeted lint and production web build pass. Reserve protection and manual-action access are covered. A real browser hover with a simulated GitHub CLI rate-limit failure verified the original-URL fallback (screenshot). No live account was exhausted.
Web and desktop share the hover component; native Electron was not timed separately. Mobile shares the server improvement and has no hover interaction.
Main video · PR video. Normal-speed illustrative samples; tables use all selected runs.
Implemented with GPT-6 in Codex.