Skip to content

feat: read a linked pull request in Tcode (A4) - #664

Merged
Tryanks merged 12 commits into
mainfrom
feat/pr-client-read
Oct 9, 2026
Merged

Tryanks merged 12 commits into
mainfrom
feat/pr-client-read

Conversation

@Tryanks

@Tryanks Tryanks commented Oct 8, 2026 •

Copy link
Copy Markdown
Owner

Summary

client (desktop tab level 2 / phone page)            host
PullRequestView ── Query::PullRequest{session,key,read} ──▶ read_pull_request: a PR the thread shows
  Files ─────────── Files{page:None}  ─────────────────▶   (a visible link, or a layer of a visible
    shared DiffList (diff/list.rs, also DiffPanel)          link's native stack; core::pull_request::shown)
    gap ▸ FileText{head sha, path} ─────────────────▶ PullRequestReads (services, one cache owner)
  Conversation ──── Conversation / ThreadReplies ───▶   revisions  pulls/N (base, head, changed_files, node_id)
  Viewed box ────── ViewedFiles / SetPullRequestFilesViewed ▶ whole diff (vnd.github.diff, 8 MiB, 60 s deadline)
  every image URL ─ Media{url, validator} ──────────▶     └─ refused or cut ▸ pulls/N/files pages (100/page)
     (body images + avatars; the host classifies)       contents@sha (raw, 1 MiB) · conversation 60 s
  Refresh ───────── RefreshPullRequest (drop the PR's reads + sync)   viewed 15 s (5×100) · failures 0 · single-flight
every reply carries expires_at; the client re-asks when it passes, never on its own TTL

Closes #640 (accepted gaps: layer navigation not seen on a real stacked pull request, and the private-image, expired-signed-URL and account-switch cases fixture-only; see Gaps)

  • Host (services): PullRequestReads behind ReadCache (TTL, single-flight, failures kept for no one, byte budget, another account's answers dropped on read, invalidated by the sync when it observes a change and by RefreshPullRequest). The viewed mutation's node id comes from the REST pulls/N read revisions() already makes (no node-id map, no separate query). Transport deadline cap raised to 60 s for diff/file reads only; REST Accept override for the raw diff/file representations.
  • No on-disk cache. Reads are held in memory only: every answer has a TTL of 15 s to 10 min, so a disk copy would be stale on the next start anyway, and nothing measured a restart storm that a disk cache would absorb. Text at a commit is the only immutable read, and it is read only when a gap is expanded.
  • Revisions window: cached revisions (base, head, changed_files) are dropped whenever a files read lands a listing that disagrees with changed_files, so a push that adds or removes files is picked up on the next read. A push that changes files without changing their count can still pair a files answer with the previous head for up to 60 s (the revisions TTL).
  • Files input: base/head revisions, repository-relative paths with previous path, Hunks | Binary | Oversized | Withheld, explicit complete + changed_files, review anchors on threads. A re-read of page 1 (expiry) keeps the pages read after it, the scroll and complete while head, base and page 1 are unchanged; a moved head resets; a manual Refresh restarts from page 1. Never the local checkout.
  • Deviation from upstream (base side): the base side of a file is reconstructed by reverse-applying the patch to the text at the head revision, which gives the merge-base side the hunks were cut against. Upstream reads the file at base.sha instead, which is the base branch's tip and can differ from what the diff shows. Deleted files are reconstructed from their patch alone; there is no base-revision read.
  • Conversation: description, issue comments, review bodies (a bodiless COMMENTED review is the threads' wrapper and is dropped), review threads with resolve/outdated state, hunk excerpt and anchors, replies paged 100 at a time after the first 10 (thread verified to belong to the PR), reactions, avatars; complete false past 10 pages. The Conversation count is comments plus reviews with a body.
  • Media, one owner (host): the client sends every image URL in a body and every avatar to the host; the host classifies it — user-attachments, legacy owner/repo/assets/N/id, blob/raw → raw host, raw/media hosts (credentialed); legacy user-images, private-user-images, camo (no token); avatars only when an author in the conversation or a read reply has that avatar_url — and answers Unsupported for anything else, which the client then draws by its URL. Token only on github.com / raw / media hosts and only for credentialed sources, never on a signed hop; ≤ 3 HTTPS redirects; size checked from Content-Length and while reading; decoded dimensions ≤ 8192 px / 24 MP; video/audio answer External. Revalidation: an ETag goes back as If-None-Match, a Last-Modified date as If-Modified-Since; 304 → NotModified.
  • Media, client: a typed MediaOutcome (no strings in errors) behind its own GPUI asset, keyed by host connection + opaque account partition, 64 MiB budget. When an image's expires_at passes the client drops the asset and asks again with its validator, drawing the old copy until the answer lands (NotModified keeps it). Failed loads are dropped on Refresh and whenever the conversation is read again on expiry. An account change (a conversation answered for another account) also drops the held files, viewed marks and replies and reads them again.
  • UI (spec tmp/design/track-a4-ui.md): drill-in inside the Pull requests tab, header, layer selector, Files/Conversation switch, file column/jump with filter (virtualized, "100+ files" in both while pages remain), viewed boxes (optimistic, 400 ms batch; counter "N of <changed_files> viewed"), placeholders, inline thread cards + "comments not on this diff", paging footer, conversation list, refresh, notices, phone Destination::PullRequest (not restored at launch). A failed file-text read shows the design's toast. Unlinking the pull request whose view is open (or the link whose stack shows it) closes the view back to the list, and the phone page pops back to the thread. DiffPanel's row rendering moved to diff/list.rs behind DiffListHost; both views draw through it.

Evidence

  • Before: the Pull requests tab listed links only; a row opened GitHub.
    After: desktop light, PR tab Files with the file column on mbedTLS: Update to 4.1.0, PSA Crypto godotengine/godot#120725, 1200×800 pt, and phone light (phone --local, 393×852), the PR page's Files view on the same pull request (synthetic profiles, English, this head):
    desktop light
    phone light
  • Checks on this head: cargo fmt --all --check clean · cargo clippy --workspace --all-targets --locked -- -D warnings clean · cargo nextest run --workspace --locked → 1054 tests run: 1054 passed, 13 skipped · cargo machete → no unused dependencies.
  • Windows CI failed on the previous head: crates/services/tests/github.rs imported Credentials at the top while only the #[cfg(unix)] gh test uses it after the Store helper moved to tests/support/github.rs → -D unused-imports on Windows. Fixed by importing it inside that test.
  • Tests at the owning boundaries, real client against the loopback fixture (crates/services/tests/pull_request_reads.rs): whole diff with rename/deletion/binary + text at a revision; refused whole diff → pages, withheld/oversized patches, the original refusal when pages fail, and revisions read again when the files disagree with changed_files (fails without the invalidation); outdated multiline thread + paged replies scoped to the PR; thread list capped at 10 pages; viewed state stops at 5 pages and marking re-reads only viewed state (paths and node id as variables, node id from the REST read); single-flight (the first answer is held until the other readers have asked), TTL via each answer's expires_at, failure not cached, account switch; media classification table, token only on credentialed GitHub hosts, public classes and avatars without token, an avatar admitted for a reply's author once the reply is read but not for a body quoting its URL, If-None-Match and If-Modified-Since revalidation (304 → NotModified), HTTP redirect refused, 3-redirect cap, declared over-cap, a chunked body without Content-Length cut at the cap while reading, oversized dimensions, non-media type, unmentioned URL, Unsupported for other hosts with no request sent.
  • Runtime: a read for a pull request the thread does not show is refused; a layer of the linked PR's native stack is read; a key in no shown stack is still refused; a sync-observed change reads fresh.
  • UI: pull_requests::detail::tests::an_expiry_reread_keeps_the_pages_read_after_page_one_and_the_scroll — the view against a scripted host: page 1 lands expired, page 2 is read as the reader nears the end, the reader scrolls to file 120, then the expiry re-read of page 1 lands; 150 files, complete and the scroll stay (fails without the fix: 100 files, scroll reset). Markdown paragraph flowing around an image across CRLF breaks; markdown::selection_adapter::tests::text_wrapping_after_an_inline_image_paints_on_its_own_rows — excalidraw#8530's "Technical parts" paragraph (bold, CRLF, image, CRLF, long link) at 393 pt: no two painted text line boxes overlap (fails without the fix: the text after the image kept its CRLF, so its painted fragment dropped the link a row onto the link's wrapped tail). The flow now turns breaks into spaces once where it measures the text, for the wrapper, the shaper and the painted fragment alike. The refresh label reads "Refresh" until a files or conversation read has landed (was "updated 20735d ago").
  • Tests deleted or rewritten (CONTRIBUTING "When a test fails, or is met on the way"):
    • reads_are_shared_in_flight_…: the 15 s wall-clock sleep and its post-sleep assertions are deleted (step 1/2: a wall-clock wait proves nothing the expires_at assertions do not; expiry is the cache's until, proven by each answer's expiry); the 100 ms overlap sleep is replaced by holding the first answer until the other readers have asked (step 1: drive concurrency deterministically).
    • viewed_files_stop_at_five_pages_…: the exact alias-text assertion is deleted (step 2: the document text is an implementation detail; the variables are the contract and stay asserted); the node-id query count assertion is gone with the query.
    • media_is_read_only_from_github_assets_… renamed …_from_github_hosts_… and rewritten for the host-owned classification (step 3: the contract moved to classify; the old "private-user-images is not read" expectation was the old policy).
    • a_collapsed_file_keeps_its_header_and_an_unavailable_one_its_placeholder (diff/list.rs) deleted (step 2: it asserted list item construction, not what the view draws; the collapse/placeholder layout is not otherwise covered by a test — it is checked visually).
    • resolves_file_headers_independently_for_unified_and_split_lists moved from diff/view.rs to diff/list.rs with the functions it covers (step 3, assertions unchanged).
  • Live (read-only, this machine's gh login, public PRs, previous head): mbedTLS: Update to 4.1.0, PSA Crypto godotengine/godot#120725 — GitHub refuses the whole diff (354 files) → page 1; renames, deletions, oversized, withheld files, the outdated multiline thread on thirdparty/README.md:691–697 with its hunk, gap expansion from text at the head. feat: add first-class support for CJK excalidraw/excalidraw#8530 — whole diff (288 files, binary woff2 fonts, renames, deletions), PR-body screenshots through the media read.
  • Phone over iroh (this head): the phone example paired to tcode-headless serve (throwaway profile, same two links, --traverse off, direct QUIC on loopback) opened #120725 Conversation and Files and #8530 Conversation. PR replies on the wire, NDJSON line → deflated bytes on the connection's stream: #120725 conversation 10,909 → 3,388 B; files page 1 862,455 → 158,653 B; viewed files 25,703 → 2,164 B (its 15 s re-read on the same connection: 356 B); avatars 11,313 → 8,504 B and 2,953 → 2,000 B. #8530 conversation 109,337 → 11,644 B; avatars 2,237 → 1,536 B and 1,212 → 777 B; body images 279,796 → 83,034 B and 839,884 → 631,108 B. The NDJSON line sizes equal the desktop figures below; only the iroh stream deflates them. The phone figures sit below the desktop's 648 MB because the phone did not open #8530's Files (the 288-file whole diff the desktop lays out in full). Phone peak RSS (/usr/bin/time -l, debug build): 315.5 MB maximum resident (265.1 MB peak footprint) with #120725 Conversation + Files; 363.7 MB maximum resident (335.7 MB peak footprint) in one process holding #8530 Conversation (both decoded screenshots) and #120725 Conversation + Files page 1. Headless host 157 MB RSS. phone --local with #120725 Conversation + Files: 298.5 MB maximum resident.
  • Wire size (encoded NDJSON reply lines, desktop client): #120725 conversation 10.9 KB, files page 1 862 KB, viewed files 25.7 KB, avatars 2.9–11.3 KB; #8530 conversation 109 KB, whole diff 652 KB, viewed files 28.5 KB, body images 280 KB and 840 KB.
  • Peak RSS (desktop, ps): 253 MB at launch → 378 MB with #120725 conversation + 100 files rendered; → 648 MB with #8530 conversation (two large decoded screenshots) + 288 files. The 648 MB includes the eager list build: the shared diff renderer builds the rows of every loaded file up front (highlighted, both unified and split), not only the visible ones, so a whole diff of 288 files is laid out in full.
  • Looked at, not attached (this head): dark theme on desktop and phone; desktop at 700 pt, where the file column folds into the "288 files" jump menu; the Conversation view with inline images (#8530's perf-log screenshot drawn through the media read); Files/Conversation scroll kept across a switch, a Settings round trip and a window resize.
  • Gaps, accepted: layer navigation not seen on a real stacked pull request (none was available; covered by the runtime and fixture tests above); private image, expired signed URL and account switch on real accounts are fixture-only (tests above). Mobile/Web builds run in CI only.

Merge Danger

Door: two-way
Blast Radius: diff
Adds unreleased wire (Query::PullRequest incl. PullRequestMedia::Unsupported, RefreshPullRequest, SetPullRequestFilesViewed; notes above PROTOCOL_VERSION). The Diff tab now renders through the extracted diff/list.rs; its behaviour should be unchanged, but it was only checked by its tests here (the desktop Diff body is empty on main too — the separate bug Q8). Viewed marks write to GitHub as the host's account. Images in a pull request from non-GitHub hosts now cost one host round trip before the client loads them by URL.

Tryanks added 12 commits October 9, 2026 02:22
… or date, reads stack layers the thread shows, and takes the node id from the REST read
…ad stops showing, revalidate media by its expiry, and count viewed files out of the pull request's
@Tryanks
Tryanks marked this pull request as ready for review October 9, 2026 02:12
@Tryanks
Tryanks merged commit 3e8be3d into main Oct 9, 2026
7 checks passed
@Tryanks
Tryanks deleted the feat/pr-client-read branch October 9, 2026 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Source control: pull request client, read side (A4)

1 participant