Skip to content

fix(github): pull request activity reports a rate-limited review-thread read instead of hiding its threads - #32620

Open
only21mil wants to merge 1 commit into
pingdotgg:mainfrom
only21mil:fix/github-rate-limited-thread-read
Open

only21mil wants to merge 1 commit into
pingdotgg:mainfrom
only21mil:fix/github-rate-limited-thread-read

Conversation

@only21mil

Copy link
Copy Markdown
Contributor

Problem

The GitHub activity read fetches the PR's comments and its review threads at the same time. If GitHub rate-limits only the review-thread read, getChangeRequestActivity swallows that error (Effect.orElseSucceed) and returns success with zero review threads marked truncated. The PR panel then shows the comments with "This conversation is only partially loaded", the review threads disappear with no reason given, and the service caches that result as a success (60 s on an open PR, 10 min on a merged one). The review guide and fix prompt also get no threads.

Expected: the same rate-limited state the panel already shows when the comments read is limited. GitLab (#18817) and Bitbucket (#12486) already pass rate limits through from their optional activity reads. GitHub is the one provider left.

This is most likely when the thread read is the only request refused: it makes more requests than the comments read (thread pages plus dismissal pages), so it can hit primary quota exhaustion partway through, a secondary limit (403 + Retry-After) on one page, or a host pause recorded between the two reads.

Change

In GitHubPullRequestProvider.ts, the thread read now uses Effect.catchIf and lets GitHubApiRateLimitError and SourceControlRateLimitPausedError through, as getViewerPermissions and readChecks in the same file already do. The activity read then fails with reason rate-limited, which the existing mapping and UI already handle. Any other thread-read failure still falls back to the truncated result, as before.

The thread read moved into a local const because, written inline inside the Effect.all tuple, the catchIf predicate's error was typed any.

Not changed: an authentication error on the thread read alone still falls back to the truncated result. The comments read runs with the same credential and already surfaces auth failures, so that is left out of this PR.

Scope and approval

A small, focused fix for an obvious bug, so it qualifies without a prior issue: a rate limit is reported as missing data. It changes one fallback in one provider, matches the GitLab and Bitbucket fixes above, and adds no new behavior or setting. Related but separate: #27579 and #27595 (reads during a pause) do not touch this fallback.

Verification

  • New tests in the existing getChangeRequestActivity describe in GitHubPullRequestProvider.test.ts: a rate limit and a rate-limit pause on the thread read now fail rate-limited with the retryAt; a 500 on the thread read still returns activity with truncated threads.
    • Base: vp test run src/server/GitHubPullRequestProvider.test.ts -t getChangeRequestActivity → 2 failed (activity succeeded with reviewThreads: [], reviewThreadsTruncated: true), 2 passed.
    • This branch: the whole file, 33 passed.
  • vp run check:changed <base>: format, lint, knip, graphql, typecheck (11 packages) and tests (3 files) pass.
  • Real run: a local vp run dev server, with isolated state, talking to a stub GitHub Enterprise host with a fake token. The stub serves one PR, answers the comments read, and returns a secondary rate limit (403, retry-after: 120) for every review-thread query. Same steps on base and on this branch, in the web client and in the Android app on an emulator. The emulator renders in software, so the Android captures show behavior, not rendering.

Web, base: the comments load, the review threads are missing, and the panel says the conversation is only partially loaded.

Web PR Summary on base with the partially loaded banner

Web, this branch: the activity section shows the rate-limit error with Retry.

Web PR Summary on this branch with the rate-limit error

Android (emulator), base: the Activity tab shows the comments and "More activity is available on the provider."

Android Activity tab on base

Android (emulator), this branch: the Activity tab shows the rate-limit error instead of the timeline.

Android Activity tab on this branch with the rate-limit error

Not checked: a live github.com rate limit, and iOS (same shared mobile screen as Android). The rate-limited state shows GitHub's message; this PR does not add a reset time to the UI.

Generated with Claude Opus 5.5, GPT-6.1 Terra and GPT-6.1 Sol in T3 Code Nightly

…ad read instead of hiding its threads

When GitHub refused only the review-thread read with a rate limit or a held pause, the activity read swallowed the error and returned zero threads marked truncated. Clients showed "partially loaded" and the result was cached as a success. The thread read now lets rate-limit errors through, as getViewerPermissions does, so the activity read fails as rate-limited, the same as when the remarks read is limited. Other thread-read failures still degrade to the truncated fallback.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Nov 7, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 84396b3

Macroscope's review found this PR approvable — Rate-limited review-thread reads now report an existing rate-limit error instead of appearing as empty threads. The change is confined to read-error handling, with other fallback behavior unchanged.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

getChangeRequestActivity continues to fetch pull-request activity and review threads concurrently. Rate-limit errors from thread reads remain failures; other thread-read errors return the activity with an empty, truncated thread list.

Changes: Review-thread error handling

Layer / File(s) Summary
Thread-read error handling
packages/source-control-github/src/server/GitHubPullRequestProvider.ts, packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts
The method preserves rate-limit and paused-rate-limit errors, and recovers other thread-read errors with an empty, truncated result. Tests cover these cases and the returned retry time.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • pingdotgg/t3code#27579 — The change handles a paused-rate-limit error from the review-thread read, but does not change the allowPaused wrapper described in the issue.

Suggested reviewers: tonybynmp4, t3dotgg, yash-singh1

Merge Risk: Minimal · up to 84396

The changed handling reports review-thread rate limits while retaining a truncated-activity fallback for other read failures. No actionable merge-blocking risk is established.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title names the fix in the GitHub provider: a rate-limited review-thread read is now reported as a rate-limited activity failure instead of returning zero threads marked truncated. This matches th…
Description check Passed The description follows the repository template closely. It has Problem, Change, Scope and approval, and Verification sections. The Problem section gives the failure, expected behavior, and reproducti…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the fix in the GitHub provider: a rate-limited review-thread read is now reported as a rate-limited activity failure instead of returning zero threads marked truncated. This matches the raw summary, which says rate-limit and paused-rate-limit errors from thread reads now fail the combined operation with rate-limited, while other thread-read failures still return truncated activity. The title is specific and conventional, though it is long at 106 characters.

Full details: Description check

Explanation

The description follows the repository template closely. It has Problem, Change, Scope and approval, and Verification sections. The Problem section gives the failure, expected behavior, and reproduction conditions. The Change section explains the catchIf fix and the typing reason for the local const. The Scope section argues the fix qualifies as a small obvious-bug fix without a prior issue. The Verification section lists focused tests with base and branch results, the full check run, a real stub-host run, and before/after screenshots. It also states what was not checked: live github.com rate limits and iOS. Minor gap: the template's optional agent attribution line is present, so no gap there. The description is complete and specific.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 25bf84a and 84396b3.

Files selected for processing: 2

  • packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts
  • packages/source-control-github/src/server/GitHubPullRequestProvider.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

@maria-rcks

maria-rcks commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

partial verification of head 84396b3be96763c0170f0aba7e67486547d5da3f on linux. ran github/main (6c9adfb273) first in a separate worktree, then the exact contributor head, with vp i and one background dev server at a time. drove the web client with the repository's playwright and system google chrome. projects and threads were created through the app, with real codex gpt-6.1-sol turns on both revisions. no simulated github responses or seeded data.

live github activity loaded on both revisions: two comments, one review approval, and commit activity. on the pr, expanding/collapsing comments, reversing timeline order, and opening the two-file code tab worked. a real codex turn read and returned the exact pr head. the four existing getChangeRequestActivity tests passed through blacksmith-check; the pr reports 31 successful ci checks. no code changes were made.

main, normal activity path:

main loads live github activity

pr head, same normal activity data:

pr head loads the same live github activity

pr head, timeline interactions:

pr head expands comments and reverses timeline order

real provider turn on the pr checkout:

codex returns the exact contributor head

runtime transcript: https://uploads-production-47e4.up.railway.app/files/3da693d1-5b5e-42ba-9017-975a9ebc4726/32620-runtime.txt

Not verified

the changed rate-limit failure path is unverified. github accepted both reads on main and the pr, so the original bug did not reproduce. no review-thread-only rate limit or host pause occurred. without simulated responses, this run cannot prove the rate-limited activity error, retry/reset recovery, success-cache behavior after that failure, or the non-rate-limit thread-read fallback in the running app. the focused tests cover the error mapping, but do not replace this missing integration proof.

native mobile and electron clients were not exercised. this is not a claim that the fix has been verified end to end.

@only21mil

Copy link
Copy Markdown
Contributor Author

Note

🤖 Opus 5.5 replying on behalf of Victor.

Thank you, Maria, for running 84396b3 against main in a real dev environment. Your run showed the normal activity path is unchanged by this fix: with live GitHub data, the PR head loads the same comments, approval and commit activity as main, and expanding comments, reversing the timeline and opening the code tab all still work. It also confirmed the four getChangeRequestActivity tests pass and that a real Codex turn reads the exact head.

Noted on what stays open: GitHub accepted every review-thread read in your run, so the rate-limited path itself didn't reproduce live, and the desktop and mobile clients weren't exercised. The PR's own verification covers that path with a stub GitHub Enterprise host that refuses only the review-thread query, on web and on the Android app, plus the tests for a rate limit, a rate-limit pause and a non-rate-limit failure that still falls back to the truncated result. We're glad to follow up if anyone sees it behave differently on a real limit.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants