Fetch all pages of Charity Navigator results

[bickelj-agent]

Why

The Charity Navigator importer only fetched page 1 of results. The GraphQL query declared $perPage but never passed page/perPage to the nonprofitsPublic field, and the caller passed an undeclared resultSize variable the API ignored. Because the Charity Navigator API defaults to 10 results per page, lookups of more than 10 EINs silently imported only the first page (see #275).

What changed

• Declare $page and $perPage in the query and pass them to nonprofitsPublic(filter: $filter, page: $page, perPage: $perPage).
• In getCharityNavigatorProfiles, loop page from 1 through pageInfo.totalPages, accumulating every page's edges and returning a combined result with the last page's pageInfo.

The existing consumers (lookup and updateAll) read data.nonprofitsPublic.edges, so they continue to work unchanged and now receive all matching nonprofits across every page.

Resolves #275 Support pagination for Charity Navigator import

[bickelj]

This appears to work fine. I think the git comment should take the form "Issue [issue number] [issue title]" though.

Also I would like the LLM to identify itself in git comments and any comments it makes in the code.

[Copilot]

Pull request overview

This PR fixes Charity Navigator GraphQL pagination so imports no longer silently stop at the first page of results when looking up many EINs.

Changes:

• Adds $page and $perPage variables to the NonprofitsPublic GraphQL query and passes them to nonprofitsPublic.
• Updates getCharityNavigatorProfiles to fetch pages sequentially from page=1 through pageInfo.totalPages, accumulating all edges into a single result.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bickelj]

Maybe I shouldn't be impressed but I am impressed that GLM-5.2 was able to get this working on the first try without access to any CN API key.

[bickelj]

I tried it locally at 83edf04 and I was able to see the greater-than-10 (15) results come back from prod with a lookupFromPdc call, also able to run updateAll successfully in the test env (though only 9 are there). Unless copilot comes up with something compelling here, I think this is ready for non-bickelj-non-LLM review.

[bickelj]

Oh, but the order of events should be to first review the earlier cleanup PR on which this is based and merge that before merging this into main (not into that branch).

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/charityNavigator.ts:147

• perPage is currently set to eins.length, which can be 0 (e.g., lookup defaults eins to []) and can also be arbitrarily large. Even if the API ignores the requested page size, sending perPage: 0 (or an extremely large value) risks a server-side validation error and makes the intent of the variable unclear. Use a fixed positive page size constant (e.g., 100) instead of coupling it to the number of EINs requested.

  const allEdges: NonprofitPublic[] = [];
  // The Charity Navigator GraphQL API may limit the number of results returned
  // per page, so a single request may return only the first page of results.
  // Iterate through every page until `totalPages` is reached (GLM-5.2).
  /* eslint-disable no-await-in-loop -- page-based pagination without cursors
  requires sequential awaited requests (GLM-5.2). */
  for (let page = 1; ; page += 1) {
    const variables: NonprofitsPublicVariables = {
      filter: {
        ein: {
          in: eins,
        },
      },
      page,
      perPage: eins.length,
    };

[Copilot]

Pull request overview

Copilot reviewed 23 out of 26 changed files in this pull request and generated 2 comments.

[bickelj]

Locally, I tried updating the const PER_PAGE and found it seemed to work correctly for lookupFromPdc and also updateAll. Like this:

-const PER_PAGE = 100;
+const PER_PAGE = 4;

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[bickelj-agent]

(Here was the prompt that generated this comment, which in turn was prompted by @hminsky2002 during review:

One small doubt: what if the apollo response is actually null, and what if the pageInfo.totalPages is undefined or null by some weird accident? Can you tell me what would happen there (in a comment to the PR) and/or add tests for these cases? Is it handled appropriately and can your test demonstrate that?

)

Good question — I traced both cases, and the totalPages ones were genuine silent-failure modes (the same class of bug as #275), so I added defensive validation plus tests. Identifying as GLM-5.2 per the repo convention.

What would happen without the fix:

1. response.data is null: the existing data === undefined guard only checked undefined, so null fell through to const { nonprofitsPublic } = data, and destructuring null throws a cryptic TypeError: Cannot destructure property 'nonprofitsPublic' of null. Not silent, but not a clear error.

2. pageInfo.totalPages is undefined: page >= undefined evaluates as page >= NaN, which is false for every page — the loop never returns and keeps fetching pages forever. Infinite loop / unbounded API requests.

3. pageInfo.totalPages is null: page >= null coerces null to 0, so 1 >= 0 is true on the first page and the loop returns after page 1 — silently importing a partial result, precisely the bug Support pagination for Charity Navigator import #275 fixed.

4. pageInfo.totalPages is 0: same as null — stops after page 1, silent partial import.

The root cause is that PageInfo.totalPages: number is only a compile-time promise; the GraphQL response is untyped at runtime, so the loop's exit condition trusted a value the type checker couldn't actually guarantee.

What I changed (src/charityNavigator.ts):

• fetchAllPages now validates Number.isInteger(pageInfo.totalPages) && totalPages >= 1 after each fetch and throws a clear error otherwise. This turns the infinite-loop (undefined) and silent-partial (null/0) cases into loud failures.
• Extracted the per-page response handling into a pure extractPageFromResponse(response, page) helper that throws a clear "returned no data" error for both null and undefined data (broadening the old === undefined-only guard), instead of letting destructuring produce a cryptic TypeError.

Tests that demonstrate it (src/charityNavigator.unit.test.ts):

• fetchAllPages throws (and calls the fetcher exactly once — i.e. no infinite loop) when totalPages is undefined, null, or 0.
• extractPageFromResponse returns nonprofitsPublic for valid data, and throws a clear error when data is null or undefined.

eslint --max-warnings=0, prettier --check, tsc --noEmit, and the full Jest suite (18 tests) all pass. Amended commit 4f290cc pushed to this branch. -- GLM-5.2

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[bickelj]

I looked over the changes and tests and they seem reasonable to me. I also tried it locally using a PER_PAGE = 3 for both lookupFromPdc (prod) and updateAll (test) and the commands worked.

[slifty]

@bickelj this PR says "Refs #275" <-- does it merely reference, or does it resolve?

[bickelj]

@bickelj this PR says "Refs #275" <-- does it merely reference, or does it resolve?

It does resolve, I'll update the PR description and close the issue if that doesn't do it.

[slifty]

I didn't read all of the PR thread in huge detail BUT I see a lot of thought has gone into this, and Looking at the code this LGTM