Skip to content

Optimize peer-review team membership lookups - #112

Merged
roryabraham merged 4 commits into
mainfrom
rory/670058-targeted-reviewer-membership
Oct 28, 2026
Merged

roryabraham merged 4 commits into
mainfrom
rory/670058-targeted-reviewer-membership

Conversation

@roryabraham

@roryabraham roryabraham commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

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.

@roryabraham
roryabraham requested a review from a team September 18, 2026 00:21
@roryabraham
roryabraham requested review from rafecolton and removed request for a team September 18, 2026 01:28
@roryabraham
roryabraham marked this pull request as ready for review September 18, 2026 01:38

@rafecolton rafecolton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good optimization 👍

Couple notes:

  1. Do we still need pagination now that we're expecting to return only 0 or 1 result per team?
  2. 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

Copy link
Copy Markdown
Contributor Author

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

rafecolton
rafecolton previously approved these changes Sep 25, 2026

@rafecolton rafecolton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-27T07:09:12.861969Z 159183e New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@roryabraham

Copy link
Copy Markdown
Contributor Author

@rafecolton this is ready for another review. I agree it's simpler and better after your suggestions.

Comment on lines +38 to +40
const memberQueries = uniqueCandidateLogins.map((login, i) => `member${i}: members(first: 1, query: ${JSON.stringify(login)}) { nodes { login } }`).join('\n');
const teamQueries = uniqueTeamSlugs.map((slug, i) => `team${i}: team(slug: ${JSON.stringify(slug)}) {\n${memberQueries}\n}`).join('\n');
const response = await client.graphql<TeamMembersResponse>(`{\norganization(login: "${EXPENSIFY_ORG}") {\n${teamQueries}\n}\n}`);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB graphql doesn't require or are about newlines, so they are only useful if you're trying to print a human-readable version.

@roryabraham
roryabraham merged commit d392acd into main Oct 28, 2026
9 of 10 checks passed
@roryabraham
roryabraham deleted the rory/670058-targeted-reviewer-membership branch October 28, 2026 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants