perf: make getSupportedBranches faster - #209
Conversation
Assisted-by: GPT-5.6 Sol
There was a problem hiding this comment.
I reviewed this PR and didn't find any concrete bugs. Because it replaces a well-established REST-based branch lookup with a new GraphQL query whose fuzzy -x-y query matching, dropped protected: true filter, and hardcoded first: 100 (no pagination) are hard to fully validate without hitting the live GitHub API, a human look would still be worthwhile before merging.
What was reviewed: the rewritten getSupportedBranches GraphQL query and its input validation, the removal of the old REST protected: true filter (branches no longer need to be protected to count as "supported"), the unpaginated first: 100 fetch versus the old unbounded github.paginate, and the updated call sites in chromium-handler.ts, node-handler.ts, and get-target-branch-labels.ts plus their test coverage.
Extended reasoning...
Overview
This PR replaces the REST-based github.repos.listBranches/getBranch pagination in getSupportedBranches with a single GraphQL query against repository.refs, changes the function to be async and take an Octokit client directly, adds numSupportedVersions bounds validation, and introduces a lightweight Branch type used across chromium-handler.ts, node-handler.ts, and get-target-branch-labels.ts. Tests were updated accordingly, including a new get-supported-branches.spec.ts that mocks octokit.graphql.
Security risks
No injection, auth-bypass, or data-exposure concerns: the GraphQL query uses parameterized variables ($owner, $repo, $branchQuery), and the repo/owner are hardcoded constants, not user input. The numSupportedVersions validation is a reasonable input-sanity check, not a security boundary.
Level of scrutiny
This warrants more than a cursory pass because it changes core semantics of a function used to decide which branches get rolled/labeled, not just a performance tweak. Two behavior changes stand out from reading the diff directly: (1) the old REST path always fetched branches with protected: true, so only protected branches were ever eligible to be "supported"; the new GraphQL query has no such constraint, so any branch matching the release-name regex (protected or not) is now included. (2) the query is capped at first: 100 with no pageInfo/cursor follow-up, unlike the old github.paginate which fetched every branch. Both were already surfaced as candidate issues and investigated/ruled out in this run's bug-hunting pass, but I could not independently confirm GitHub's exact fuzzy-match semantics for the refs(query:) argument (e.g., whether it requires a literal substring or a subsequence, which affects whether legacy N-M-x freeze branches would still match -x-y) since outbound network access to the GitHub API is blocked in this sandbox.
Other factors
Test coverage for the new function is reasonable (mocks graphql, covers filtering and count-limiting), but since the mocks stand in for the live GraphQL API, they can't validate the real fuzzy-match behavior of refs(query: ...), which is exactly the part of this change that's hardest to reason about statically. Given that, plus the semantic changes noted above touching a function that gates which branches get rolled, I think a human with access to test against the live API (as the PR author says they did, citing an ~800ms measurement) is warranted before merging, even though no independent findings are being raised here beyond what was already investigated.
Assisted-by: GPT-5.6 Sol
|
Added pagination just to be safe - the longer term goal here is to switch to using |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The follow-up commit adding manual GraphQL pagination directly addresses the earlier concern about only fetching a single page (100) of branches, and is now covered by a dedicated pagination test. Because this replaces the REST-based protected-branch listing with a GraphQL name-pattern query across several call sites (chromium-handler, node-handler, roll.ts), a human look is still worthwhile.
What was reviewed:
- Pagination loop in
getSupportedBranches(cursor handling,hasNextPage/endCursor) and its new test coverage for multi-page results. - Dropping the
protected: truefilter in favor of the-x-yGraphQL search plus local regex filtering — checked whether this could admit unprotected/non-release branches; ruled out. - Whether the hardcoded
branchQuery: '-x-y'server-side filter could exclude validN-M-xbranches still accepted by the local regex — ruled out. - Signature change from sync
getSupportedBranches(branches, n)to asyncgetSupportedBranches(github, n)and the updated call sites/tests.
Extended reasoning...
This PR replaces REST-based (listBranches + paginate) discovery of Electron's supported release branches with a GraphQL query against electron/electron's refs, changing getSupportedBranches from a synchronous filter over a caller-supplied branch list to an async function that itself queries and paginates GitHub. It touches chromium-handler.ts, node-handler.ts, get-target-branch-labels.ts, roll.ts, and types.ts (new Branch type replacing two octokit REST response types), plus corresponding test updates across three spec files.
Security risk is low: the GraphQL query uses parameterized variables (owner, repo, branchQuery, cursor), not string interpolation, so there's no injection concern, and the repo/owner are hardcoded rather than user-influenced.
A prior version of this PR was reviewed and raised a pagination concern — the original GraphQL query fetched only a single page (first: 100) with no cursor handling, which could silently drop supported branches once the repo has more than 100 matching refs. The commit pushed after that review (6dba230, "chore: manually paginate GraphQL query") adds a cursor-based pagination loop and a new test (fetches every page of release branches) that exercises it, directly resolving that concern. This run's bug hunt investigated the removal of the old protected: true filter and a possible mismatch between the server-side -x-y search term and the locally-accepted N-M-x regex pattern, and ruled both out (no bugs reported this run).
Given the change alters a widely-used piece of branch-discovery logic (now called from multiple handlers with a different async signature) and swaps out a protection-based filter for a name-pattern-based one, I think the correct scrutiny level is a human sanity check even though the automated review found nothing actionable — this is a meaningful behavioral/architectural change rather than a mechanical one, even though it's now well covered by tests and the identified pagination gap has been closed.
Follow-up to #207, which added more calls of
getSupportedBranchesto the common code paths. Same general change as electron/sudowoodo#419, just adapted to this codebase.Validated locally as only taking around 800ms now to fetch supported branches, while from Datadog log entries it looks like it's currently taking 25+ seconds per call.