fix(forge): support PRs with over 300 changed files - #475
Conversation
|
Hi @agavra, I'm pretty unsure if this approach is even a mediocre idea. Problem I want to solve is that I have a PR that have gone a little overboard that I want to review and I can't right now. Noted the never comments about using local repo as source of truth - not sure how dead set on that you are though? Would be neat to be able to review PRs of any size since I enjoy the traceability it gives my team to work against the PR comments. Will leave this as draft, let me know what you think in general before I put more time (and tokens) into it. |
4822589 to
e8ff9d3
Compare
|
Fresh reproduction on current That PR currently reports 1,022 changed files. The pull-request files API returns ten full 100-file pages and a final 22-file page, which also confirms why that API would need bounded pagination and still retain its 3,000-file ceiling. I found no other open PR in this repository implementing the >300-file fallback; this remains the active implementation for #521. |
|
@agavra any thoughts? |
|
Not sure why you would want to review PRs that large. Shouldn't those PRs be split into multiple PRs? |
|
Thanks for the patience getting back to you guys on this one, I'm in agreement with @benja2998 I'm not super eager to add a bunch of complicated logic to support diffs with over 300 changed files. I doubt people are really reviewing those by hand anyway. What's your use case for this? |
tuicr cannot open pull requests with more than 300 changed files. GitHub stops serving the combined diff at that point, and patches large enough to hit the limit also made file and commit navigation slow.
This PR removes that limit and keeps large reviews usable.
Approach
Normal PRs still use
gh pr diff. The fallback runs only for GitHub's specific 300-file error.When tuicr is running inside a checkout for the same repository, it checks that the current branch name and
HEADmatch the PR metadata exactly. If the base commit is also available, Git can build the merge-base diff from the local object database. The command compares two commits, so dirty files, staged changes, and untracked files are not included.If any check fails, tuicr creates a blobless bare repository under the system temporary directory. It can borrow objects from the matching local repository, including from a linked worktree's common Git directory, then fetches the missing base and PR objects into the temporary repository. The temporary repository is deleted when the diff is ready.
External diff, textconv, and replace objects are disabled for locally generated diffs. Other
gh pr differrors are returned as before.Other approaches tried and ruled out
/tmp: A worktree is useful when files must be edited or tested. This path only needs a commit-to-commit diff. Adding a worktree still writes administrative state into the user's repository, can leave stale entries after interruption, and does not solve missing objects without a fetch.The current hybrid keeps the common checked-out-PR case local and read-only. Cases that cannot prove they have the exact commits use the isolated fallback instead.
Large-review rendering keeps only visible rows plus overscan. File and hunk positions are cached, and parsing, syntax highlighting, range saves, and cleanup no longer block the input loop. This follows the background-work direction from #288 without exposing partially built diffs to the UI.
The real test case changes 592 files and produces a 408,004-line patch. With the PR branch checked out, startup app initialization dropped from about 16 seconds to 8.4 seconds and did not run
gh repo clone. File navigation takes 10-11 ms, cached commit-range changes take about 6 ms, and the 400,000-line release benchmark averages 165 microseconds per navigation frame.The fallback tests cover exact and stale PR heads, dirty and staged state isolation, detached heads, shallow clones missing the base, advanced base branches, linked worktrees, local object borrowing, no-checkout clones, merge-base behavior, external diff and textconv suppression, replace objects, cleanup, and the 301-file end-to-end case. Local formatting, Clippy, 1,106 tests, the release performance benchmark, CodeGraph sync, and the security review pass.