Mobile: GitHub pull request review#4621
Open
iscekic wants to merge 19 commits into
Open
Conversation
- getPullRequest: propagate raw 401 from GraphQL enrichment to the token retry boundary instead of silently degrading - Checks/Files/Discussion: dedicated reconnect state (re-check connection) instead of an ineffective same-query retry - classifyPrReviewMutationError: forbidden (terminal) and reconnect classes; applied to comment/reply/submit/merge surfaces - de-duplicate the diff syntax token-color palette into syntax-colors.ts - explicit useThemeColors colors for merge Lucide icons - drop the dead side-by-side selection path (read-only)
…wThreads The mobile useInfiniteQuery integration injects a direction discriminator into the procedure input; the .strict() schemas rejected it, 400-ing every files/ threads page. Accept it explicitly and add regression coverage.
Href<typeof PATH> with a group-prefixed literal fails the generated typed-routes constraint; match the working plain-Href pattern used elsewhere in the feature.
octokit.request('POST /graphql', {query, ...vars}) sends variables at the top
level, where GitHub ignores them (all $variables resolve null). This silently
dropped nested comment pages and would break thread pagination cursors and every
GraphQL mutation (resolve/unresolve/reactions/auto-merge) against real GitHub.
Nest under `variables` at all four call sites; update the follow-up test. E2E
against a strict GraphQL mock now returns the second comment page.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryAll 6 previously flagged WARNINGs (side-by-side gap layout, malformed-patch fallback, path-traversal-like owner/repo capture, inconsistent UNAUTHORIZED classification, no-op merge method default, and unpersisted terminal_rejection) are fixed at the current commit, each with a regression test; no new issues were introduced in the incremental diff. Files Reviewed (13 files, incremental)
Previous Review Summary (commit 73905d0)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 73905d0)Status: 6 Issues Found | Recommendation: Address before merge Executive SummaryThe highest-risk issue is a logic gap in the mobile diff-list builder ( Overview
Issue Details (click to expand)WARNING
Files Reviewed (139 files)
Reviewed by claude-sonnet-5 · Input: 24 · Output: 3.6K · Cached: 606.7K Review guidance: REVIEW.md from base branch |
- github-pr-url: restrict owner/repo to GitHub's charset and reject dot-only segments (no path-traversal-like values) - classify query state: UNAUTHORIZED -> reconnect (matches mutation path); FORBIDDEN stays terminal permission - merge-commit-defaults: drop the no-op method param - diff list builder: route non-empty-but-unparsable patches through the patch-missing fallback; render gap/expand rows correctly in side-by-side - git-token-service: persist revocation on terminal_rejection during selection; rewrite the OAuth base-URL regex to avoid polynomial backtracking (ReDoS)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a full GitHub pull request review experience to the mobile app: paste or tap a PR link, then review the PR end to end — overview + checks, file diffs with syntax highlighting, line-level comments with a client-side pending-review queue, discussion threads with replies/resolve/reactions, and merge / auto-merge / update-branch — all backed by the user's GitHub App authorization.
What's included
Mobile (
apps/mobile)[owner]/[repo]/[number]layout + Overview/Files/Discussion tabs, and comment-composer / review-submit / file-navigator / merge form-sheets), gated behind a GitHub connect gate.lowlight, expand-context, viewed tracking, file navigator + search, tablet side-by-side), Discussion (threads grouped by file, replies, optimistic resolve/unresolve + reactions), and a PR-scoped pending-review queue with a single batched submission.Web (
apps/web) —githubPrReviewtRPC router (reads: getPullRequest, listChecks, listFiles, getFileLines, listReviewThreads; mutations: create/reply/submit review, resolve/unresolve, add/remove reaction, merge, update-branch, enable/disable auto-merge), proxying GitHub REST + GraphQL through a single 401→rotate→retry→report boundary.git-token-service — internal user-token endpoint that fetches/rotates/reports the caller's GitHub App user token, scoped to a dedicated internal-JWT audience.
Testing
check:unusedclean.E2E surfaced and fixed three real bugs, each with regression coverage:
listFiles/listReviewThreads.strict()inputs rejected tRPC's injected infinite-querydirectionparam (every page 400'd).Href<typeof PATH>with group-prefixed route literals failed the generated typed-routes constraint.octokit.request('POST /graphql', …)instead of nested undervariables, so they never reached GitHub — silently dropping nested comment pages and breaking all pagination cursors + GraphQL mutations against real GitHub.