Adopt graphql_client codegen for typed GraphQL queries - #196
Draft
leynos wants to merge 8 commits into
Draft
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @leynos, your pull request is larger than the review limit of 150000 diff characters
|
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:
Summary
WalkthroughThe PR adds schema-validated GraphQL operations and generated response types. It centralizes client execution and cursor pagination, migrates issue, branch, review, and review-thread flows, and updates resolve fixtures and documentation. ChangesTyped GraphQL migration
Sequence Diagram(s)sequenceDiagram
participant ReviewCommentsFetcher
participant GraphQLClient
participant GitHubGraphQL
ReviewCommentsFetcher->>GraphQLClient: Execute ThreadForCommentQuery
GraphQLClient->>GitHubGraphQL: Send reviewThreads request
GitHubGraphQL-->>GraphQLClient: Return paginated threads and comments
GraphQLClient-->>ReviewCommentsFetcher: Return ThreadPage
ReviewCommentsFetcher->>GraphQLClient: Execute ResolveReviewThreadMutation
GraphQLClient->>GitHubGraphQL: Resolve thread by ID
GitHubGraphQL-->>GraphQLClient: Return mutation response
Possibly related PRs
Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 8 warnings, 2 inconclusive)
✅ Passed checks (7 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Vendor GitHub's published public schema (72,911 lines, from https://docs.github.com/public/fpt/schema.docs.graphql) at `graphql/schema.docs.graphql` with a README recording provenance and the refresh procedure. Add the `graphql_client` 0.16 dependency (default features only; its optional reqwest transports stay off). Clean-build baseline before any codegen derives: 46 s (`cargo build --all-features` on this machine), recorded for the ExecPlan's build-time tolerance.
Move the issue lookup, review-thread listing, per-thread comment
paging, reviews listing, PR-for-branch lookup, and the
`resolveReviewThread` mutation onto `graphql_client` 0.16 codegen.
Every operation now lives in a named `.graphql` document under
`graphql/`, validated against the vendored GitHub schema at compile
time — a malformed query fails the build rather than a runtime
request.
Typed execution surface on `GraphQLClient`:
- `run_operation` executes a codegen'd operation and returns its
generated `ResponseData`;
- `run_operation_as` and `paginate_operation_as` build the
schema-checked envelope from the generated operation but deserialize
into hand-written domain structs, preserving lenient behaviour the
generated types cannot express (notably the documented client-side
default treating threads missing `isOutdated` as current) and
keeping `serde_path_to_error` paths byte-identical;
- a `CursorVariables` trait plus `paginate_operation` drive typed
cursor pagination with the same `MAX_PAGES` cap and discard-on-error
semantics as `paginate_all`, covered by new red-green rstest units
against scripted loopback stubs.
A shared `src/api/scalars.rs` maps GitHub's custom scalars
(`DateTime`, `URI`, `HTML`); the alias names must match the schema
scalars, so tightly-scoped `expect` attributes cover the
capitalized-acronym lint.
`src/graphql_queries.rs` is deleted. The string-based `run_query`
retains one production consumer: the resolve path's review-comments
paging query selects a `reviewComments` field that does not exist in
GitHub's published schema — a latent production bug exposed by the
codegen migration (only mocked tests kept it green). Its redesign onto
`reviewThreads` follows in a separate commit.
Issue-not-found now surfaces as a semantic `VkError::BadResponse`
("issue #N not found") rather than the previous accidental serde
failure; no test pinned the old text (recorded in the ExecPlan
decision log).
The resolve path's paging query selected `PullRequest.reviewComments`, a field that does not exist in GitHub's published GraphQL schema — `vk resolve` could never locate a thread against the live API, and only fully mocked tests kept the path green. Codegen validation exposed the bug the moment the operation moved to a `.graphql` document. Replace it with `ThreadForCommentQuery`, which pages `repository.pullRequest.reviewThreads` (with each thread's first 100 comments) and scans for the requested comment id, returning the owning thread's GraphQL id. Comments are matched by `fullDatabaseId` — the schema deprecates `databaseId` in favour of the 64-bit-safe field — via a new `BigInt` scalar alias carried as a decimal string. The `resolveReviewThread` mutation also moves to a typed operation. Behavioural guarantees are preserved: not-found mapping, the missing-cursor and non-advancing-cursor aborts, and the mockall fetcher seam with its sequence-based unit tests. Accepted limitation, documented in the module: a comment beyond the first 100 comments of a single thread is not found — the same class of cap as the old flat query's page size. `src/graphql_queries.rs` remnants are gone; every operation now lives in a schema-validated document. The string-based query surface (`run_query`, `fetch_page`, `paginate_all`) is retained deliberately as a fully-tested raw escape hatch for the planned shared-crate extraction (see the ExecPlan decision log).
Delete `run_query`, `fetch_page`, `paginate_all`, the `paginate` free function, and the `Query` newtype now that every operation executes through the typed codegen path. This corrects the previous commit's note that the surface would be retained: the removal was completed with every characterization assertion ported intact (recorded in the ExecPlan decision log). The retry, error-detail, and transcript characterization tests now exercise the shared `run_payload` core directly with identical assertions and scripted servers; the cursor-capture test moved to the typed pagination path, asserting the paginator's cursor lands in `variables.cursor` through the `CursorVariables` impl. The non-object-variables rejection test is retired rather than ported: typed `Variables` structs are objects by construction, so the guarded failure mode cannot exist. `operation_name` stays: the transport still derives request-context strings from it.
Extract the codegen derives, envelope structs, `CursorVariables` impls, and domain-type definitions from `src/review_threads.rs` (505 lines) and `src/reviews.rs` (425 lines) into `src/review_threads/wire.rs` and `src/reviews/wire.rs`, bringing every product source back under the 400-line limit. Public paths are preserved through `pub use wire::…` re-exports, so no consumer changes. Pure refactor: no behaviour or signature changes. `src/review_threads/tests.rs` (655 lines, tests-only) already exceeded the limit at HEAD and is left for a follow-up.
Rewrite the design document's networking section for the codegen era: named operation documents under `graphql/` validated at compile time, `run_operation` and the `_as` decoding escape hatches, `CursorVariables` pagination with the 1000-page cap, the scalars module, and the wire submodule pattern. Update the resolve-threads description to the `reviewThreads`/`fullDatabaseId` lookup, noting the latent `reviewComments` bug the migration exposed and the documented first-100-comments-per-thread limitation. Add the `graphql/` directory to the repository layout. Record PR 3 milestones, artifacts, and an interim retrospective in the ExecPlan.
Record the zero-finding CodeRabbit review and the stacked draft PR; set the plan status to COMPLETE pending review and merges.
Resolve dependencies from the rebased manifests so the lockfile records the versions selected against the current `main` dependency baseline.
leynos
force-pushed
the
typed-graphql-queries
branch
from
August 11, 2026 13:04
ffc5917 to
acad4e9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/resolve.rs (1)
44-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated response literal.
The two
format!literals differ only in thepageInfofragment. Build that fragment once, then interpolate it into a single response template. The two branches then cannot drift apart when the selection set changes.♻️ Proposed deduplication
fn body(&self) -> String { // One review-thread page holding a single thread whose sole comment // carries the scripted database id (`fullDatabaseId` is a BigInt // scalar, transported as a string). - self.end_cursor.map_or_else( - || { - format!( - r#"{{"data":{{"repository":{{"pullRequest":{{"reviewThreads":{{"pageInfo":{{"endCursor":null,"hasNextPage":false}},"nodes":[{{"id":"{}","comments":{{"nodes":[{{"fullDatabaseId":"{}"}}]}}}}]}}}}}}}}}}"#, - self.thread_id, - self.comment_id, - ) - }, - |cursor| { - format!( - r#"{{"data":{{"repository":{{"pullRequest":{{"reviewThreads":{{"pageInfo":{{"endCursor":"{cursor}","hasNextPage":true}},"nodes":[{{"id":"{}","comments":{{"nodes":[{{"fullDatabaseId":"{}"}}]}}}}]}}}}}}}}}}"#, - self.thread_id, - self.comment_id, - ) - }, - ) + let page_info = self.end_cursor.map_or_else( + || r#"{"endCursor":null,"hasNextPage":false}"#.to_owned(), + |cursor| format!(r#"{{"endCursor":"{cursor}","hasNextPage":true}}"#), + ); + format!( + r#"{{"data":{{"repository":{{"pullRequest":{{"reviewThreads":{{"pageInfo":{page_info},"nodes":[{{"id":"{}","comments":{{"nodes":[{{"fullDatabaseId":"{}"}}]}}}}]}}}}}}}}}}"#, + self.thread_id, self.comment_id, + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/resolve.rs` around lines 44 - 64, Refactor the body method’s end_cursor branches to construct the differing pageInfo fragment once, then interpolate it into a single response format template containing the shared thread and comment data. Preserve the existing cursor-specific endCursor and hasNextPage values while removing the duplicated response literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/execplans/adopt-octocrab.md`:
- Around line 337-339: Update the sentence in the transport documentation to
explicitly pair “neither” with “nor”, naming both proxy support and redirect
support as unused and untested on the GraphQL path. Preserve the existing
meaning and use en-GB-oxendict grammar.
- Line 721: Complete the outstanding milestone in the execplan by recording the
cargo tree -d report, sample transcript line, and closing test counts near the
“final octocrab feature set” note, or remove that note if those details are
already captured elsewhere. Preserve the existing COMPLETE status and
checked-off roadmap items.
- Around line 207-224: Reorder the dated progress entries in the execution plan
chronologically: move the three 2026-07-09 entries before the 2026-07-28 and
2026-08-03 follow-ups. Preserve their content and maintain the handoff timeline
as a living implementation plan.
In `@graphql/pr_for_branch.graphql`:
- Line 3: Update the fetch_pr_for_branch pullRequests query to order by
UPDATED_AT descending and include pageInfo with endCursor; paginate through
subsequent pages using the cursor until a matching pull request is found or
hasNextPage is false, rather than limiting the search to the initial 10 results.
In `@graphql/README.md`:
- Around line 21-23: Replace the indented refresh-command block in the README
with a fenced Markdown block, using bash as its language identifier and
preserving the existing curl and make commands.
In `@graphql/resolve.graphql`:
- Around line 11-15: Update the comments selection used by get_thread_id to
include the nested pageInfo metadata needed to detect truncated comment results,
while preserving the existing fullDatabaseId selection. Ensure traversal
distinguishes a target absent from the fetched page because pagination ended
from one hidden beyond the first 100 comments, and surfaces the accurate
truncation error instead of VkError::CommentNotFound.
In `@graphql/review_threads.graphql`:
- Around line 9-26: Define a reusable ThreadCommentFields fragment on
PullRequestReviewCommentConnection containing the shared comment fields, then
replace the duplicated inline selections in both comments selections with that
fragment spread. Keep the existing fields and pagination selections unchanged.
In `@src/api/client/mod.rs`:
- Line 308: Update the error text in the VkError::BadResponse construction to
use “serializing” instead of “serialising”, keeping the operation context and
error interpolation unchanged.
In `@src/api/client/pagination.rs`:
- Around line 30-56: Remove the unused paginate_operation wrapper and its
associated item-level dead_code expectation. Update the two documentation
references in paginate_operation_as to describe the remaining API without
referring to the deleted helper.
In `@src/api/client/tests.rs`:
- Around line 357-362: Correct the test reference in the NOTE to
`paginate_operation_sends_cursor_in_request_variables`, which contains the
`overwrites_stale_cursor` case. Leave the rest of the explanation unchanged.
In `@src/api/pagination/tests.rs`:
- Around line 2-6: Add a dedicated test in the pagination tests module that
exercises the MAX_PAGES guard in paginate_operation_as, confirming traversal
stops at the configured page limit and preserves the expected result. Keep the
existing PageInfo invariant tests and traversal coverage unchanged.
In `@src/api/scalars.rs`:
- Around line 15-19: Remove the unused HTML scalar alias and its item-level
expectation from the scalar definitions, unless a tracked issue is explicitly
available to justify retaining it. Update the nearby consumer comment to include
BigInt and its resolve/fullDatabaseId usage, while preserving the existing
DateTime and URI references.
In `@src/issues.rs`:
- Around line 69-75: The GraphQL Int conversion must enforce the signed 32-bit
range consistently. In src/issues.rs lines 69-75, update the issue query
variable conversion to use an i32-bounded conversion widened to i64 and revise
the adjacent comment; apply the same conversion in src/resolve/graphql.rs lines
86-96 before constructing thread_for_comment_query::Variables.
In `@src/main.rs`:
- Line 35: Update the API section in docs/vk-design.md to remove references to
the deleted string-based query and pagination surface, including paginate_all
and the fetch_page example, and describe only the currently exported
GraphQLClient API from src/api/mod.rs.
In `@src/resolve/graphql.rs`:
- Around line 219-241: Update the test fixture helper page so each thread
created from a comment ID receives a distinct ID derived from that comment ID
instead of the constant "t"; adjust finds_thread_owning_the_comment expectations
accordingly. Add a substantive case covering a page with multiple threads where
the target comment belongs to a later thread, ensuring find_thread_in_page does
not incorrectly return the first thread.
In `@src/review_threads/wire.rs`:
- Around line 94-102: Document the public Connection type, its nodes and
page_info fields, and the public CommentConnection alias with rustdoc comments.
Also document ReviewThreadConnection as appropriate for the module’s public API,
keeping the existing types and serialization behavior unchanged.
- Around line 94-99: Update src/review_threads/wire.rs:94-99 and
src/reviews/wire.rs:78-83 so Connection::nodes and ReviewConnection::nodes use a
shared deserializer that converts null lists and null elements into the
established empty/filtered representation, with #[serde(default)] for an absent
or null field. Ensure both hand-written envelopes tolerate GitHub’s nullable
nodes schema consistently.
In `@src/reviews/wire.rs`:
- Around line 47-57: Add a struct-level rustdoc comment to the public
PullRequestReview type describing its purpose, and explicitly document that
state contains the wire value verbatim because the generated enum is bypassed.
Keep the existing field documentation and structure unchanged.
---
Outside diff comments:
In `@tests/resolve.rs`:
- Around line 44-64: Refactor the body method’s end_cursor branches to construct
the differing pageInfo fragment once, then interpolate it into a single response
format template containing the shared thread and comment data. Preserve the
existing cursor-specific endCursor and hasNextPage values while removing the
duplicated response literals.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 32636d98-abba-4de4-abf8-10e7b235582e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
Cargo.tomldocs/execplans/adopt-octocrab.mddocs/repository-layout.mddocs/vk-design.mdgraphql/README.mdgraphql/issue.graphqlgraphql/pr_for_branch.graphqlgraphql/resolve.graphqlgraphql/review_threads.graphqlgraphql/reviews.graphqlgraphql/schema.docs.graphqlgraphql/test_pagination.graphqlsrc/api/client/mod.rssrc/api/client/pagination.rssrc/api/client/tests.rssrc/api/client/types.rssrc/api/mod.rssrc/api/pagination.rssrc/api/pagination/tests.rssrc/api/scalars.rssrc/branch_pr/mod.rssrc/graphql_queries.rssrc/issues.rssrc/main.rssrc/resolve/graphql.rssrc/review_threads.rssrc/review_threads/tests.rssrc/review_threads/wire.rssrc/reviews.rssrc/reviews/wire.rstests/resolve.rs
💤 Files with no reviewable changes (2)
- src/graphql_queries.rs
- src/api/client/types.rs
Comment on lines
+207
to
+224
| - [x] (2026-07-09 21:10Z) PR 3 implementation complete: schema vendored | ||
| (72,911 lines); all six operations in named `.graphql` documents; | ||
| `run_operation`/`run_operation_as`/`paginate_operation_as` plus the | ||
| `CursorVariables` trait (red-green tested); domain structs preserved behind | ||
| conversions; `src/graphql_queries.rs` and the string query surface deleted | ||
| with characterization tests ported; the resolve thread-lookup latent bug fixed | ||
| (`reviewThreads`/`fullDatabaseId`); wire-submodule split restores the | ||
| 400-line limit; compile-fail demonstrated | ||
| (`No field named titleTYPO on Issue`) and reverted; clean build 17 s versus | ||
| the 46 s baseline (well within tolerance); full suite green. | ||
| - [x] (2026-07-09 21:20Z) Documentation pass complete across all PRs: | ||
| `docs/vk-design.md` networking and resolve sections rewritten for the typed | ||
| path, e2e guide MITM correction (PR 2), `docs/repository-layout.md` gains the | ||
| `graphql/` entry; users' guide reviewed, no change needed. | ||
| - [x] (2026-07-09 21:50Z) PR 3 CodeRabbit review completed with zero | ||
| findings against the cumulative diff from main; draft pull request opened as | ||
| leynos/vk#196 (stacked on PR 2). Plan status COMPLETE pending review and | ||
| merges. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Order the progress entries by date.
The changed entries dated 2026-07-09 at Lines 207-224 appear after entries dated 2026-07-28 and 2026-08-03 at Lines 195-199. Move the 2026-07-09 entries before those later follow-ups, or record the actual completion dates. Keep the handoff timeline chronological.
As per path instructions, maintain docs/execplans/ as a living location for implementation plans that survive context handoffs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/execplans/adopt-octocrab.md` around lines 207 - 224, Reorder the dated
progress entries in the execution plan chronologically: move the three
2026-07-09 entries before the 2026-07-28 and 2026-08-03 follow-ups. Preserve
their content and maintain the handoff timeline as a living implementation plan.
Source: Path instructions
Comment on lines
+337
to
+339
| (`HTTP(S)_PROXY`) and redirects are deliberately not supported by the new | ||
| transport — reqwest honoured both by default, but neither is used or tested | ||
| on the GraphQL path; both are documented in the transport module. The |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Pair neither with nor.
Replace but neither is used or tested on the GraphQL path with but neither proxy support nor redirect support is used or tested on the GraphQL path. This makes the two referenced features explicit.
Triage: [type:grammar]
As per path instructions, use en-GB-oxendict spelling and grammar in Markdown documentation.
🧰 Tools
🪛 LanguageTool
[grammar] ~338-~338: Use “nor” with neither.
Context: ...ed both by default, but neither is used or tested on the GraphQL path; both are ...
(NEITHER_NOR)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/execplans/adopt-octocrab.md` around lines 337 - 339, Update the sentence
in the transport documentation to explicitly pair “neither” with “nor”, naming
both proxy support and redirect support as unused and untested on the GraphQL
path. Preserve the existing meaning and use en-GB-oxendict grammar.
Sources: Path instructions, Linters/SAST tools
| - reqwest absence: `cargo tree -i reqwest` reports the package is not | ||
| found in normal, dev, and all-features graphs. | ||
|
|
||
| Also to record as milestones complete: the final octocrab feature set, the |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Complete the outstanding milestone record.
Record the cargo tree -d report, the sample transcript line, and the closing test counts, or remove this “Also to record” note. The plan already declares Status: COMPLETE, so the unfinished action makes the execplan stale.
As per coding guidelines, keep completed roadmap items checked off and keep each execplan current.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/execplans/adopt-octocrab.md` at line 721, Complete the outstanding
milestone in the execplan by recording the cargo tree -d report, sample
transcript line, and closing test counts near the “final octocrab feature set”
note, or remove that note if those details are already captured elsewhere.
Preserve the existing COMPLETE status and checked-off roadmap items.
Source: Coding guidelines
| @@ -0,0 +1,15 @@ | |||
| query PrForBranchQuery($owner: String!, $name: String!, $headRef: String!) { | |||
| repository(owner: $owner, name: $name) { | |||
| pullRequests(headRefName: $headRef, first: 10, states: [OPEN, MERGED]) { | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the branch pull-request selection logic.
fd -t f 'mod.rs' -p 'branch_pr' --exec cat -n {}
rg -n -C5 'PrForBranchQuery|pr_for_branch_query' --type=rustRepository: leynos/vk
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(graphql|.*branch.*pr|.*pr.*branch).*' | head -200
printf '%s\n' '--- query references ---'
rg -n -C6 'PrForBranchQuery|pr_for_branch|pullRequests\(headRefName|headRefName' --glob '*.rs' --glob '*.graphql' .Repository: leynos/vk
Length of output: 21112
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- consumer ---'
cat -n src/branch_pr/mod.rs | sed -n '111,160p'
printf '%s\n' '--- connection arguments ---'
cat -n graphql/schema.docs.graphql | sed -n '4425,4485p'
printf '%s\n' '--- query and related tests ---'
cat -n graphql/pr_for_branch.graphql
cat -n src/branch_pr/tests.rs | sed -n '320,390p'
printf '%s\n' '--- pagination conventions ---'
rg -n -C4 'orderBy:|pageInfo|hasNextPage|first: 10|pullRequests' graphql src tests --glob '*.graphql' --glob '*.rs' | head -240Repository: leynos/vk
Length of output: 19128
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- pullRequests field definitions ---'
rg -n -B18 -A25 '^ pullRequests\(' graphql/schema.docs.graphql
printf '%s\n' '--- IssueOrder definition ---'
rg -n -B8 -A35 '^input IssueOrder|^enum IssueOrderField|^enum OrderDirection' graphql/schema.docs.graphql
printf '%s\n' '--- branch lookup documentation and tests ---'
rg -n -C5 'head_owner|head owner|cross-repository|first PR|first.*PR|branch.*PR' src/branch_pr tests graphql/README.mdRepository: leynos/vk
Length of output: 21794
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository pullRequests arguments ---'
cat -n graphql/schema.docs.graphql | sed -n '53870,53945p'
printf '%s\n' '--- operation schema compatibility probe ---'
python3 - <<'PY'
from pathlib import Path
schema = Path("graphql/schema.docs.graphql").read_text()
query = Path("graphql/pr_for_branch.graphql").read_text()
start = schema.index(" pullRequests(", schema.index("type Repository"))
end = schema.index(" ): PullRequestConnection", start) + len(" ): PullRequestConnection")
field = schema[start:end]
print("query_has_orderBy =", "orderBy:" in query)
print("repository_field_has_orderBy =", "orderBy:" in field)
print("repository_field_has_headRefName =", "headRefName:" in field)
print("repository_field_has_first =", "first:" in field)
print(field)
PYRepository: leynos/vk
Length of output: 3425
🌐 Web query:
GitHub GraphQL API Repository pullRequests headRefName orderBy default ordering pagination
💡 Result:
The GitHub GraphQL API handles repository pull requests through the PullRequestConnection object, which supports ordering and pagination [1]. Ordering: You can specify the ordering of pull requests using the orderBy argument on the pullRequests field [2][3]. This argument accepts an object of type PullRequestOrder [4], which requires a field (e.g., CREATED_AT, UPDATED_AT) and a direction (ASC or DESC) [5][3]. While specific documentation on the absolute default ordering may vary, it is standard practice in GitHub's API to explicitly define your orderBy object to ensure consistent results [5][2][6]. Pagination: GitHub's GraphQL API uses cursor-based pagination for connections [7][8]. 1. Arguments: You must provide either first or last (with a maximum value of 100 per request) [8][9]. 2. Cursor Navigation: To fetch subsequent pages, use the after argument with the endCursor from the current page's pageInfo object [7][8]. If using last, use the before argument with the startCursor to navigate backwards [7][8]. 3. Metadata: The pageInfo object provides the necessary cursors (endCursor, startCursor) and booleans (hasNextPage, hasPreviousPage) to track your position in the collection [7][8]. Regarding headRefName: The headRefName field is a property of the PullRequest object itself [10][2]. It represents the name of the branch where the changes were implemented and is returned as part of the pull request data nodes [10][5][2]. It is not used as an argument for the pullRequests connection and cannot be used to filter or order the pull requests collection [10][1].
Citations:
- 1: https://docs.github.com/en/enterprise-cloud@latest/graphql/reference/pulls
- 2: https://stackoverflow.com/questions/57731122/how-to-extract-open-pull-request-information-from-github-graphql-response
- 3: https://herve.bzh/github-graphql-api-search-for-more-than-1000-pull-requests/
- 4: Wrong type for the 'orderBy' argument of 'pullRequests' field of 'repository' object in the GraphQL documentation github/docs#14854
- 5: https://blog.hashhackers.com/blog/github-graphql-guide/
- 6: https://gist.github.com/MichaelCurrin/f8a7a11451ce4ec055d41000c915b595
- 7: https://github.com/github/docs/blob/main/content/graphql/guides/using-pagination-in-the-graphql-api.md
- 8: https://docs.github.com/en/graphql/guides/using-pagination-in-the-graphql-api
- 9: https://docs.github.com/en/enterprise-cloud@latest/graphql/guides/using-pagination-in-the-graphql-api
- 10: https://docs.github.com/en/graphql/reference/pulls
Order and paginate the branch pull-request lookup.
fetch_pr_for_branch selects a matching node from the first 10 results. Add orderBy: { field: UPDATED_AT, direction: DESC }, request pageInfo, and follow endCursor until a match is found or the connection ends. Ordering alone does not prevent a matching pull request from falling outside the first page.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@graphql/pr_for_branch.graphql` at line 3, Update the fetch_pr_for_branch
pullRequests query to order by UPDATED_AT descending and include pageInfo with
endCursor; paginate through subsequent pages using the cursor until a matching
pull request is found or hasNextPage is false, rather than limiting the search
to the initial 10 results.
Comment on lines
+21
to
+23
| curl -L https://docs.github.com/public/fpt/schema.docs.graphql \ | ||
| -o graphql/schema.docs.graphql | ||
| make lint test |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Replace the indented code block with a fenced block that carries a language identifier.
The refresh commands use a four-space indented block. Markdown code blocks must be fenced and must declare a language identifier.
Triage: [type:syntax/md]
📝 Proposed fix
- curl -L https://docs.github.com/public/fpt/schema.docs.graphql \
- -o graphql/schema.docs.graphql
- make lint test
+```bash
+curl -L https://docs.github.com/public/fpt/schema.docs.graphql \
+ -o graphql/schema.docs.graphql
+make lint test
+```As per coding guidelines: "Follow markdownlint recommendations in documentation" and "Always provide a language identifier for fenced code blocks".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| curl -L https://docs.github.com/public/fpt/schema.docs.graphql \ | |
| -o graphql/schema.docs.graphql | |
| make lint test |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@graphql/README.md` around lines 21 - 23, Replace the indented refresh-command
block in the README with a fenced Markdown block, using bash as its language
identifier and preserving the existing curl and make commands.
Source: Coding guidelines
| } | ||
|
|
||
| pub use crate::api::{GraphQLClient, paginate}; | ||
| pub use crate::api::GraphQLClient; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the design document after removing the paginate re-export.
docs/vk-design.md (Lines 145-160) still states that src/api/mod.rs "exposes the GraphQLClient alongside run_query, fetch_page, and paginate_all helpers", and it shows a fetch_page code example. The PR removes the string-based query and pagination surface, so that section now describes an API that no longer exists.
As per coding guidelines: "Ensure that any API or behavioural changes are reflected in the documentation in docs/".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main.rs` at line 35, Update the API section in docs/vk-design.md to
remove references to the deleted string-based query and pagination surface,
including paginate_all and the fetch_page example, and describe only the
currently exported GraphQLClient API from src/api/mod.rs.
Source: Coding guidelines
Comment on lines
+219
to
241
| /// Build a page of review threads, each holding one comment id, with the | ||
| /// given pagination cursor. | ||
| fn page(comment_ids: Vec<u64>, end_cursor: Option<&str>, has_next: bool) -> ThreadPage { | ||
| ResponseData { | ||
| repository: Some(ThreadForCommentQueryRepository { | ||
| pull_request: Some(ThreadForCommentQueryRepositoryPullRequest { | ||
| review_threads: ThreadForCommentQueryRepositoryPullRequestReviewThreads { | ||
| page_info: | ||
| ThreadForCommentQueryRepositoryPullRequestReviewThreadsPageInfo { | ||
| end_cursor: end_cursor.map(ToOwned::to_owned), | ||
| has_next_page: has_next, | ||
| }, | ||
| nodes: Some( | ||
| comment_ids | ||
| .into_iter() | ||
| .map(|id| Some(thread("t", vec![id]))) | ||
| .collect(), | ||
| ), | ||
| }, | ||
| }), | ||
| }), | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Give each thread in the fixture a distinct id.
page maps every comment id onto a thread built with the constant id "t". finds_thread_owning_the_comment then asserts id == "t", which holds for any thread the traversal returns. The assertion passes even if find_thread_in_page returns the first thread instead of the owning thread.
Derive the thread id from the comment id, and add a case where one page holds several threads and the target comment sits in a later thread. That case fails for the "return the first thread" implementation.
As per coding guidelines: "New functionality and behavioral changes must have substantive tests that fail for plausible incorrect implementations".
💚 Proposed fixture change
fn page(comment_ids: Vec<u64>, end_cursor: Option<&str>, has_next: bool) -> ThreadPage {
@@
nodes: Some(
comment_ids
.into_iter()
- .map(|id| Some(thread("t", vec![id])))
+ .map(|id| Some(thread(&format!("t{id}"), vec![id])))
.collect(),
), async fn finds_thread_owning_the_comment() {
@@
let pages = vec![
- page(vec![1], Some("a"), true),
- // Second page: thread "t" holds the requested comment.
- page(vec![42], None, false),
+ page(vec![1, 2], Some("a"), true),
+ // Second page: the target comment sits in the second thread.
+ page(vec![7, 42], None, false),
];
@@
let id = get_thread_id(&mock, reference).await.expect("thread id");
- assert_eq!(id, "t");
+ assert_eq!(id, "t42");
}Also applies to: 285-313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/resolve/graphql.rs` around lines 219 - 241, Update the test fixture
helper page so each thread created from a comment ID receives a distinct ID
derived from that comment ID instead of the constant "t"; adjust
finds_thread_owning_the_comment expectations accordingly. Add a substantive case
covering a page with multiple threads where the target comment belongs to a
later thread, ensuring find_thread_in_page does not incorrectly return the first
thread.
Source: Coding guidelines
Comment on lines
+94
to
+99
| #[derive(Debug, Deserialize, Default)] | ||
| pub struct Connection<T> { | ||
| pub nodes: Vec<T>, | ||
| #[serde(rename = "pageInfo")] | ||
| pub page_info: PageInfo, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Hand-written connection envelopes drop schema nullability on nodes. GitHub types every connection nodes field as a nullable list of nullable elements, and the generated ResponseData models it as Option<Vec<Option<T>>>. Both envelopes declare a plain Vec<T>, so "nodes": null or a single null element fails with BadResponseSerde and aborts the whole paginated traversal. Compile-time schema validation does not cover these structs, so only a live response reveals the divergence.
src/review_threads/wire.rs#L94-L99: add a sharednodesdeserializer that tolerates a null list and null elements, and apply it toConnection::nodeswith#[serde(default)].src/reviews/wire.rs#L78-L83: apply the same shared deserializer toReviewConnection::nodes.
📍 Affects 2 files
src/review_threads/wire.rs#L94-L99(this comment)src/reviews/wire.rs#L78-L83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/review_threads/wire.rs` around lines 94 - 99, Update
src/review_threads/wire.rs:94-99 and src/reviews/wire.rs:78-83 so
Connection::nodes and ReviewConnection::nodes use a shared deserializer that
converts null lists and null elements into the established empty/filtered
representation, with #[serde(default)] for an absent or null field. Ensure both
hand-written envelopes tolerate GitHub’s nullable nodes schema consistently.
Comment on lines
+94
to
+102
| #[derive(Debug, Deserialize, Default)] | ||
| pub struct Connection<T> { | ||
| pub nodes: Vec<T>, | ||
| #[serde(rename = "pageInfo")] | ||
| pub page_info: PageInfo, | ||
| } | ||
|
|
||
| pub(super) type ReviewThreadConnection = Connection<ReviewThread>; | ||
| pub type CommentConnection = Connection<ReviewComment>; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Document the public Connection type and its aliases.
Connection, its fields, and CommentConnection form part of the public surface. Every other public type in this module carries rustdoc. Add it here so cargo doc output stays complete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/review_threads/wire.rs` around lines 94 - 102, Document the public
Connection type, its nodes and page_info fields, and the public
CommentConnection alias with rustdoc comments. Also document
ReviewThreadConnection as appropriate for the module’s public API, keeping the
existing types and serialization behavior unchanged.
Source: Coding guidelines
Comment on lines
+47
to
+57
| #[derive(Debug, Deserialize, Clone)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct PullRequestReview { | ||
| pub body: String, | ||
| /// Timestamp when the review was formally submitted. | ||
| /// | ||
| /// This may be `None` when the timestamp is missing or unknown. | ||
| pub submitted_at: Option<DateTime>, | ||
| pub state: String, | ||
| pub author: Option<User>, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Document the public PullRequestReview type.
The struct is public and re-exported through the reviews module, but only submitted_at carries rustdoc. Add a struct-level /// comment, and state that state holds the wire value verbatim, because that constraint is the reason the generated enum is bypassed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/reviews/wire.rs` around lines 47 - 57, Add a struct-level rustdoc comment
to the public PullRequestReview type describing its purpose, and explicitly
document that state contains the wire value verbatim because the generated enum
is bypassed. Keep the existing field documentation and structure unchanged.
Source: Coding guidelines
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
This branch delivers PR 3, the final phase of the GitHub API
modernisation governed by
ADR 001:
every GraphQL operation is now a named document under
graphql/,
validated against GitHub's vendored public schema at compile time by
graphql_clientcodegen. A malformed query fails the build rather than aruntime request (demonstrated: a deliberate field typo yields
No field named titleTYPO on Issuefromcargo check).Execplan: docs/execplans/adopt-octocrab.md
— PR 3 of its three phases; stacked on #195 (hyper transport), which
stacks on #194 (octocrab REST).
The migration surfaced and fixes a latent production bug: the resolve
thread-lookup query selected
PullRequest.reviewComments, a field thatdoes not exist in GitHub's published schema —
vk resolvecould neverlocate a thread against the live API, and only fully mocked tests kept it
green. The lookup is redesigned onto
reviewThreads, matching commentsby
fullDatabaseId(the schema deprecatesdatabaseId).Review walkthrough
graphql/
— the vendored schema (72,911 lines, provenance and refresh procedure
in its README) and one document per operation group.
src/api/client/mod.rs
—
run_operation(typed execution returning generatedResponseData) andrun_operation_as(schema-checked query,hand-written deserialization target). The
_asescape hatch is theload-bearing design move: it preserves documented lenient behaviour
the generated types cannot express (threads missing
isOutdatedaretreated as current) and keeps
serde_path_to_errorpathsbyte-identical. The string-based surface (
run_query,fetch_page,paginate_all,Query) is removed, with every characterizationassertion ported to the shared
run_payloadcore.and the
CursorVariablestrait — typed cursor pagination, writtentest-first, preserving the 1,000-page cap and discard-on-error
semantics.
— the redesigned thread lookup, its accepted limitation (a comment
beyond the first 100 comments of one thread is not found; the same
class of cap as the old flat query), and the typed
resolveReviewThreadmutation.(src/review_threads/wire.rs,
src/reviews/wire.rs)
— pure refactor bringing every product source back under the
400-line limit; public paths preserved via re-exports.
Validation
make check-fmt/make lint/make test: pass (215 lib tests plus all integration suites and 11 doctests, 0 failed)cargo test --test e2e -- --ignored e2e_pr_42: pass (transcript replay unchanged)graphql/issue.graphqlfailscargo check; revertedmake markdownlint/make nixie: passcoderabbit review --agent: completed, zero findings (cumulative diff frommain)Notes
ReviewThread,ReviewComment,CommentConnection,PageInfo,PullRequestReview,Issue,User)are unchanged; generated types stay module-private.
issue #N not founderrorrather than the previous accidental serde failure (no test pinned the
old text; recorded in the ExecPlan decision log).
src/review_threads/tests.rs(655 lines, tests-only) already exceededthe 400-line limit before this branch and is left for a follow-up.