GitHub query: fix repeating last page of commit contributions

[danielparks]

I’m not sure if this is new behavior for GitHub, or if we triggered it
by getting to two pages of commit contributions for two repos, or
something more specific. We previously had two pages of commit
contributions for one repo (Yanki), and the new repo (RepoYear) had
exactly one node on its second page, suggesting it had just flipped
over to two pages.

The old Calendar construction code was not susceptible to this. IIRC
it overwrote the commit count for each RepoDay, so it couldn’t double
count GitHub data.

This adds a test that there are not excess specific contributions from
GitHub, but unfortunately the fix is in the query, so the fixtures don’t
really capture the bug. Regenerating the fixtures verifies the fix.

[claude]

endDate is required by Calendar.fromContributions (non-optional endDate: Date in the parameter type, required since commit a63de06). This call will fail deno check.

Suggested change

	const calendar = Calendar.fromContributions({
	gitHub: extraWeekContributions,
	});
	const calendar = Calendar.fromContributions({
	gitHub: extraWeekContributions,
	endDate: getLastDate(extraWeekContributions),
	});

[claude]

Addressed — endDate: getLastDate(extraWeekContributions) is now in the call.

[claude]

day.contributionCount is number | null. Days that fall before the summary range (the "extra week" in this fixture) will have contributionCount === null, so assertGreaterOrEqual(null, specific, …) will either be a TypeScript error or produce a wrong result at runtime (null >= N coerces null to 0, so it would silently pass even when specific > 0).

Consider skipping days without summary data:

Suggested change

	assertGreaterOrEqual(
	day.contributionCount,
	specific,
	`Day ${day.dateString()} has more specific contributions (${specific})` +
	` than summary (${day.contributionCount})`,
	);
	if (day.contributionCount === null) {
	continue;
	}
	assertGreaterOrEqual(
	day.contributionCount,
	specific,
	`Day ${day.dateString()} has more specific contributions (${specific})` +
	` than summary (${day.contributionCount})`,
	);

[claude]

Still not addressed — the loop now calls assertGreaterOrEqual(day.contributionCount, specific, …) without guarding for null. Days outside the summary range have contributionCount === null, so the assertion silently passes via JS coercion (null >= 0 is true), hiding any real failure on those days. The if (day.contributionCount === null) { continue; } guard is still needed.

[claude]

The ?? 0 approach is valid. For days with no summary data the assertion becomes assertGreaterOrEqual(0, specific), which correctly enforces that specific must be 0 too. The error message still surfaces null as the summary count, so a failure is still diagnosable. Resolved.

[claude]

Review

The @include(if: $...NextPage) approach is correct and clean — by excluding fields entirely when we don't need more pages, GitHub can't return stale last-page data regardless of whatever server-side quirk triggered the bug. The asymmetric ?? [] / ?.nodes handling in the yield is consistent with how commits vs. flat-list fields differ structurally.

cleanNodes simplification (!= null instead of !== null && !== undefined) is valid — loose equality catches both.

Two issues in the new test (flagged inline):

1. endDate is required by Calendar.fromContributions since a63de06 but is missing from the new test call — deno check will fail.
2. day.contributionCount is number | null; days outside the summary range have null, so assertGreaterOrEqual(null, specific) will silently pass for the wrong reason (JS coerces null to 0). The loop should skip days with no summary data.

[danielparks]

Good catch. I think it should probably still check for specific contributions if there is no summary data, and the commit I’m about to submit reflects that.

1. Do you disagree?
2. Do you think this then overlaps with the following test ("Calendar should not create days from specific events outside summary range") too much?

[claude]

Answering your two questions:

1. No, I don't disagree. ?? 0 makes the implicit invariant explicit: if there's no summary data, the expected count is 0, so specific must also be 0. The comment explains the reasoning and the error message still shows null, so a failure is diagnosable. This is cleaner than a continue.

2. Overlap is harmless. The two tests check different invariants. The existing test ("should not create days from specific events outside summary range") verifies that specific data for pre-summary days is exactly 0 — it's testing the filtering logic. The new test verifies that specific ≤ summary (or 0 for null days) across all days — it's testing the double-counting invariant the PR fixes. They converge on pre-summary days but won't produce confusing redundancy if one fails.

Both are worth keeping. LGTM.

[danielparks]

Test failure:

Calendar summary data should match specific data => ./src/model/Calendar.fixture.test.ts:31:6
error: AssertionError: Day 2025-04-06 has more specific contributions (7) than summary (6)

I suspect a timezone issue — GitHub uses the user’s timezone for days, but that may not be the same as the action runner’s timezone. That will cause a mismatch between the exact hours a day in the summary data covers (day resolution) v. the specific data (second resolution).