Optimize peer-review team membership lookups by roryabraham · Pull Request #112 · Expensify/GitHub-Actions

roryabraham · 2026-09-18T00:21:37Z

Details

Avoids downloading the entire team roster (2 graphQL queries, the first with 100 nodes), just to determine whether a few independent approvers are eligible reviewers.

Pass only independent approver logins to getTeamMemberLogins and batch their members(query: ...) searches using GraphQL in one request per eligible team.

Related Issues

For https://github.com/Expensify/Expensify/issues/670058#issuecomment-5720068070 (point 2)

Manual Tests

❯ GH_PAGER=cat gh api graphql \
  -f query='
    query TeamMembers($organization: String!, $teamSlug: String!, $member0: String!, $member1: String!, $member2: String!) {
      organization(login: $organization) {
        team(slug: $teamSlug) {
          member0: members(first: 100, query: $member0) {
            nodes { login }
            pageInfo { hasNextPage endCursor }
          }
          member1: members(first: 100, query: $member1) {
            nodes { login }
            pageInfo { hasNextPage endCursor }
          }
          member2: members(first: 100, query: $member2) {
            nodes { login }
            pageInfo { hasNextPage endCursor }
          }
        }
      }
    }
  ' \
  -f organization='Expensify' \
  -f teamSlug='expensify-expensify' \
  -f member0='roryabraham' \
  -f member1='rafecolton' \
  -f member2='chrispader'

{
  "data": {
    "organization": {
      "team": {
        "member0": {
          "nodes": [
            {
              "login": "roryabraham"
            }
          ],
          "pageInfo": {
            "hasNextPage": false,
            "endCursor": "Y3Vyc29yOnYyOpHOAtPRPA=="
          }
        },
        "member1": {
          "nodes": [
            {
              "login": "rafecolton"
            }
          ],
          "pageInfo": {
            "hasNextPage": false,
            "endCursor": "Y3Vyc29yOnYyOpHOABAmqw=="
          }
        },
        "member2": {
          "nodes": [],
          "pageInfo": {
            "hasNextPage": false,
            "endCursor": null
          }
        }
      }
    }
  }
}

Linked PRs

None.

rafecolton

Good optimization 👍

Couple notes:

Do we still need pagination now that we're expecting to return only 0 or 1 result per team?
It's possible search multiple teams in much the same way as multiple members, so I'm wondering if we should optimize that too and do the whole thing in a single query:

{
  organization(login: "Expensify") {
    expensify: team(slug: "expensify-expensify") {
      rafecolton: members(query: "rafecolton") {
        nodes {
          login
        }
      }
      roryabraham: members(query: "roryabraham") {
        nodes {
          login
        }
      }
      chrispader: members(query: "chrispader") {
        nodes {
          login
        }
      }
    }
    margelo: team(slug: "margelo") {
      rafecolton: members(query: "rafecolton") {
        nodes {
          login
        }
      }
      roryabraham: members(query: "roryabraham") {
        nodes {
          login
        }
      }
      chrispader: members(query: "chrispader") {
        nodes {
          login
        }
      }
    }
  }
}

Produces this:

{
  "data": {
    "organization": {
      "expensify": {
        "rafecolton": {
          "nodes": [
            {
              "login": "rafecolton"
            }
          ]
        },
        "roryabraham": {
          "nodes": [
            {
              "login": "roryabraham"
            }
          ]
        },
        "chrispader": {
          "nodes": []
        }
      },
      "margelo": {
        "rafecolton": {
          "nodes": []
        },
        "roryabraham": {
          "nodes": []
        },
        "chrispader": {
          "nodes": [
            {
              "login": "chrispader"
            }
          ]
        }
      }
    }
  }
}

roryabraham · 2026-09-23T20:56:10Z

Both good suggestions! We could definitely drop pagination for our current use-cases, I seriously considered it and probably should have.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 944a4519ae

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-09-24T16:13:25Z

+    const teamQueries = teamSearches.map((teamSearch) => {
+        const memberQueries = memberSearches.map(
+            (memberSearch) => `
+                ${memberSearch.alias}: members(first: 1, query: ${memberSearch.variable.reference}) {


Preserve all fuzzy search results before rejecting a member

When a candidate's login has another fuzzy match that GitHub returns first, this first: 1 query never sees the exact login. The code itself notes that members(query:) can be fuzzy, but it no longer requests pageInfo or follows subsequent results, so a genuine team member can be omitted from eligibleReviewers and their valid approval will incorrectly fail the peer-review gate. Fetch enough results and continue paginating unresolved searches until an exact login is found or the connection is exhausted.

Useful? React with 👍 / 👎.

I don't think this is a real problem, because GitHub should give exact matches first, before other potential fuzzy matches

I tested with the only users I could find in our org where one user's name is a subset of another. Here were the results:

Here are the conclusions I draw:

The issue the AI reviewer cited is not a real concern - an exact match should be the first result

first: 1 is not strictly necessary. though I wouldn't remove it, since restricting the result set to exactly what you need is kinda the whole point of GraphQL

Despite GH usernames being case-sensitive, this search is not

Whether or not first: 1 is passed, the presence of a result is not sufficient. Comparing the actual username (which you do) is necessary

rafecolton

I find that the more a query is broken up and parameterized, the harder it is to see what the final query is and to test & maintain it. It would be helpful to at least add comments above each block and the final query showing examples. In this case, since there's not really an opportunity for user-provided values to be a problem, I think you could inline almost everything

rafecolton · 2026-09-25T00:01:26Z

+import {createGraphQLVariable} from '../GitHubAPIClient';
+import type {GraphQLVariable} from '../GitHubAPIClient';


I kinda think this is overkill. Variables aren't actually necessary in GraphQL queries, you can inline the values. Variables are nice if:

You're writing documentation

The query is defined in an external lib and the values are user-submitted

The values require sanitization for GraphQL-sensitive characters (e.g. quotes)

In this case, you've jumped through a lot of hoops to structure these variables only to destructure them and interpolate the declarations. So it would be fewer steps and likely more readable to interpolate the values directly.

rafecolton · 2026-09-25T00:05:14Z

-        teamPromises = new Map();
-        teamMemberLoginsPromisesByClient.set(client, teamPromises);
-    }
+async function getTeamMemberLogins(client: GitHubAPIClient, teamSlugs: string[], candidateLogins: string[]): Promise<Set<string>> {


NAB can you add some comments throughout this function?

rafecolton · 2026-09-25T00:22:28Z

+    const teamQueries = teamSearches.map((teamSearch) => {
+        const memberQueries = memberSearches.map(
+            (memberSearch) => `
+                ${memberSearch.alias}: members(first: 1, query: ${memberSearch.variable.reference}) {


I tested with the only users I could find in our org where one user's name is a subset of another. Here were the results:

Here are the conclusions I draw:

The issue the AI reviewer cited is not a real concern - an exact match should be the first result

first: 1 is not strictly necessary. though I wouldn't remove it, since restricting the result set to exactly what you need is kinda the whole point of GraphQL

Despite GH usernames being case-sensitive, this search is not

Whether or not first: 1 is passed, the presence of a result is not sufficient. Comparing the actual username (which you do) is necessary

rafecolton · 2026-09-25T00:34:29Z

+    const organizationVariable = createGraphQLVariable('organization', EXPENSIFY_ORG);
+    const memberSearches: TeamMemberSearch[] = [...uniqueCandidateLogins].map((login, index) => {
+        const alias = `member${index}`;
+        return {
+            login,
+            alias,
+            variable: createGraphQLVariable(alias, login),
+        };
+    });
+    const teamSearches: TeamSearch[] = [...uniqueTeamSlugs].map((slug, index) => {
+        const alias = `team${index}`;
+        return {
+            slug,
+            alias,
+            variable: createGraphQLVariable(`teamSlug${index}`, slug),
+        };
+    });


None of this is necessary IMO. The org can be hardcoded, you don't need variables for the teams or members, and and the aliases can be defined when you loop through them below to insert them into the query.

perf: target peer-review team membership lookups

bfe5d06

roryabraham requested a review from a team September 18, 2026 00:21

melvin-bot Bot assigned roryabraham Sep 18, 2026

refactor: clarify team membership queries

0b5a4f9

roryabraham requested review from rafecolton and removed request for a team September 18, 2026 01:28

roryabraham marked this pull request as ready for review September 18, 2026 01:38

rafecolton reviewed Sep 23, 2026

View reviewed changes

Comment thread scripts/libs/GitHubUtils/getTeamMemberLogins.ts Outdated

perf: batch peer-review team queries

944a451

chatgpt-codex-connector Bot reviewed Sep 24, 2026

View reviewed changes

roryabraham requested a review from rafecolton September 24, 2026 16:16

rafecolton approved these changes Sep 25, 2026

View reviewed changes

Navigation Menu

Repository navigation

Optimize peer-review team membership lookups - #112

Optimize peer-review team membership lookups#112
roryabraham wants to merge 3 commits into
mainfrom
rory/670058-targeted-reviewer-membership

roryabraham commented Sep 18, 2026 •

edited

Loading

Uh oh!

rafecolton left a comment

Uh oh!

Uh oh!

roryabraham commented Sep 23, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Sep 24, 2026

Uh oh!

roryabraham Sep 24, 2026

Uh oh!

rafecolton Sep 25, 2026

Uh oh!

rafecolton left a comment

Uh oh!

rafecolton Sep 25, 2026

Uh oh!

rafecolton Sep 25, 2026

Uh oh!

rafecolton Sep 25, 2026

Uh oh!

rafecolton Sep 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		import {createGraphQLVariable} from '../GitHubAPIClient';
		import type {GraphQLVariable} from '../GitHubAPIClient';

Conversation

roryabraham commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Related Issues

Manual Tests

Linked PRs

Uh oh!

rafecolton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

roryabraham commented Sep 23, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

roryabraham Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

rafecolton Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

rafecolton left a comment

Choose a reason for hiding this comment

Uh oh!

rafecolton Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

rafecolton Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

rafecolton Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

rafecolton Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

roryabraham commented Sep 18, 2026 •

edited

Loading