Skip to content

Commit 9ddefc6

Browse files
committed
Pagination fake + split CommittersDetails into Committer / SigningComment
Item 1: prove listComments pagination is actually walking pages. - fakeGithubCore.RouteResult gains an optional headers map; both the MockAgent adapter (fakeGithub.ts) and the http.Server adapter (fakeGithubHttp.ts) forward those headers back to the client. - The /issues/:n/comments route now honors ?page / ?per_page, slices accordingly, and emits a RFC 5988 Link header with rel='next' when more pages remain — the contract octokit.paginate follows. - New pagination.test.ts puts the sign-phrase comment in page 2 of a 150-comment PR and asserts signatureComment picks it up. Item 2: split the overloaded CommittersDetails type. - New Committer = {name, id, pullRequestNo?} — what GraphQL returns. - New SigningComment extends Committer with {comment_id, body, created_at, repoId} — a parsed PR comment carrying identity + persistable metadata. - Signature stays unchanged as the persisted shape. - Drop the back-compat alias and migrate all call sites: graphql.ts returns Committer[]; checkAllowList filters Committer[]; signatureComment uses SigningComment[] for listOfPRComments; setupClaCheck + pullRequestComment use Committer wherever the committer identity (not comment metadata) is what matters. - Bonus: removed the context.payload.repository!.id and prComment.user! non-null bangs in signatureComment.ts in favour of real type narrowing — skip a comment that has no user rather than assume it.
1 parent 41a6c7f commit 9ddefc6

12 files changed

Lines changed: 184 additions & 59 deletions

File tree

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
/**
2+
* Proves that octokit.paginate() is actually walking Link-header pages for the
3+
* endpoints the action cares about, not just reading page 1. Without this,
4+
* PRs with >30 comments or >100 commits would silently drop data.
5+
*
6+
* The fake now honors ?page / ?per_page query params and emits a
7+
* `Link: <...>; rel="next"` header when more pages remain; octokit.paginate
8+
* follows the Link header to fetch all pages.
9+
*/
10+
import { installFakeGitHub, FakeGitHub } from '../testHelpers/fakeGithub'
11+
import { resetEnv, setDefaultInputs } from '../testHelpers/env'
12+
import { reloadOctokit, setContext } from '../testHelpers/context'
13+
14+
function loadSignatureComment() {
15+
reloadOctokit()
16+
for (const path of Object.keys(require.cache)) {
17+
if (path.includes('/src/')) delete require.cache[path]
18+
}
19+
return require('../../src/pullrequest/signatureComment')
20+
.default as typeof import('../../src/pullrequest/signatureComment').default
21+
}
22+
23+
describe('pagination', () => {
24+
let fake: FakeGitHub
25+
26+
beforeEach(() => {
27+
setDefaultInputs()
28+
fake = installFakeGitHub()
29+
setContext({
30+
issueNumber: 7,
31+
payload: { repository: { id: 5555 } }
32+
})
33+
})
34+
35+
afterEach(async () => {
36+
await fake.close()
37+
resetEnv()
38+
})
39+
40+
it('signatureComment listComments walks every page', async () => {
41+
// Add 150 dummy comments (spans 2 pages at per_page=100).
42+
for (let i = 0; i < 150; i++) {
43+
fake.repo('acme', 'widgets').addComment(7, {
44+
body: 'noise',
45+
user: { login: `user${i}`, id: 10000 + i }
46+
})
47+
}
48+
// Insert the sign phrase at index 140 — unreachable without pagination.
49+
fake.repo('acme', 'widgets').addComment(7, {
50+
body: 'i have read the cla document and i hereby sign the cla',
51+
user: { login: 'alice', id: 1001 }
52+
})
53+
54+
const signatureWithPRComment = loadSignatureComment()
55+
const result = await signatureWithPRComment(
56+
{
57+
signed: [],
58+
notSigned: [{ name: 'alice', id: 1001, pullRequestNo: 7 }],
59+
unknown: []
60+
},
61+
[{ name: 'alice', id: 1001, pullRequestNo: 7 }]
62+
)
63+
// alice's signing comment lives on page 2. If pagination were broken,
64+
// newSigned would be empty.
65+
expect(result.newSigned.map((c: { name: string }) => c.name)).toEqual(['alice'])
66+
})
67+
68+
it('fake emits rel="next" only when there are more pages', async () => {
69+
for (let i = 0; i < 50; i++) {
70+
fake.repo('acme', 'widgets').addComment(7, {
71+
body: `c${i}`,
72+
user: { login: `u${i}`, id: i }
73+
})
74+
}
75+
const all = fake.repo('acme', 'widgets').listComments(7)
76+
expect(all).toHaveLength(50)
77+
})
78+
})

__tests__/testHelpers/fakeGithub.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,13 @@ export function installFakeGitHub(): FakeGitHub {
2929
function makeReply(method: string) {
3030
return (opts: any) => {
3131
const rawBody = typeof opts.body === 'string' ? opts.body : ''
32-
const { status, body } = core.route(method, opts.path, rawBody)
32+
const { status, body, headers } = core.route(method, opts.path, rawBody)
3333
return {
3434
statusCode: status,
3535
data: body,
36-
responseOptions: { headers: { 'content-type': 'application/json' } }
36+
responseOptions: {
37+
headers: { 'content-type': 'application/json', ...(headers || {}) }
38+
}
3739
}
3840
}
3941
}

__tests__/testHelpers/fakeGithubCore.ts

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ export interface FakeRepoHandle {
6565
export interface RouteResult {
6666
status: number
6767
body: string // serialized JSON (or empty string)
68+
headers?: Record<string, string>
6869
}
6970

7071
export interface FakeGitHubCore {
@@ -112,6 +113,32 @@ export function createFakeGitHubCore(): FakeGitHubCore {
112113
return json(404, { message: msg })
113114
}
114115

116+
/**
117+
* Paginate a full list per the caller's page/per_page query parameters and
118+
* add a RFC 5988 Link header with rel="next" when more pages remain.
119+
* Mirrors GitHub's REST pagination contract that octokit.paginate follows.
120+
*/
121+
function paginate<T>(
122+
all: T[],
123+
query: URLSearchParams,
124+
pathForLink: string
125+
): RouteResult {
126+
const perPage = Math.max(
127+
1,
128+
Math.min(100, parseInt(query.get('per_page') || '30', 10))
129+
)
130+
const page = Math.max(1, parseInt(query.get('page') || '1', 10))
131+
const start = (page - 1) * perPage
132+
const slice = all.slice(start, start + perPage)
133+
const hasNext = start + perPage < all.length
134+
const headers: Record<string, string> = {}
135+
if (hasNext) {
136+
const nextUrl = `https://api.github.com${pathForLink}?page=${page + 1}&per_page=${perPage}`
137+
headers['link'] = `<${nextUrl}>; rel="next"`
138+
}
139+
return { status: 200, body: JSON.stringify(slice), headers }
140+
}
141+
115142
type Handler = (
116143
match: RegExpMatchArray,
117144
body: string,
@@ -177,11 +204,12 @@ export function createFakeGitHubCore(): FakeGitHubCore {
177204
path
178205
})
179206
})
180-
addRoute(getRoutes, '/repos/:owner/:repo/issues/:num/comments', m => {
207+
addRoute(getRoutes, '/repos/:owner/:repo/issues/:num/comments', (m, _body, query) => {
181208
const owner = decodeURIComponent(m[1]!)
182209
const name = decodeURIComponent(m[2]!)
183210
const num = parseInt(m[3]!, 10)
184-
return json(200, getRepo(owner, name).comments.get(num) || [])
211+
const all = getRepo(owner, name).comments.get(num) || []
212+
return paginate(all, query, `/repos/${owner}/${name}/issues/${num}/comments`)
185213
})
186214
addRoute(getRoutes, '/repos/:owner/:repo/pulls/:num', m => {
187215
const owner = decodeURIComponent(m[1]!)

__tests__/testHelpers/fakeGithubHttp.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ export async function startFakeGitHubHttp(): Promise<FakeGitHubHttp> {
3030
req.on('data', c => chunks.push(c))
3131
req.on('end', () => {
3232
const body = Buffer.concat(chunks).toString('utf-8')
33-
const { status, body: out } = core.route(
33+
const { status, body: out, headers } = core.route(
3434
req.method || 'GET',
3535
req.url || '/',
3636
body
@@ -42,6 +42,9 @@ export async function startFakeGitHubHttp(): Promise<FakeGitHubHttp> {
4242
})
4343
res.statusCode = status
4444
res.setHeader('content-type', 'application/json')
45+
if (headers) {
46+
for (const [k, v] of Object.entries(headers)) res.setHeader(k, v)
47+
}
4548
res.end(out)
4649
})
4750
})

__tests__/unit/checkAllowList.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
import { resetEnv, setInput } from '../testHelpers/env'
22
import { checkAllowList } from '../../src/checkAllowList'
3-
import { CommittersDetails } from '../../src/interfaces'
3+
import { Committer } from '../../src/interfaces'
44

5-
function committer(name: string): CommittersDetails {
5+
function committer(name: string): Committer {
66
return { name, id: 0, pullRequestNo: 1 }
77
}
88

@@ -71,8 +71,8 @@ describe('checkAllowList', () => {
7171
setInput('allowlist', '')
7272
const result = checkAllowList([
7373
committer('alice'),
74-
null as unknown as CommittersDetails,
75-
undefined as unknown as CommittersDetails
74+
null as unknown as Committer,
75+
undefined as unknown as Committer
7676
])
7777
expect(result.map(c => c.name)).toEqual(['alice'])
7878
})

dist/index.js

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -949,24 +949,26 @@ const github_1 = __nccwpck_require__(3228);
949949
const getInputs_1 = __nccwpck_require__(7189);
950950
function signatureWithPRComment(committerMap, committers) {
951951
return __awaiter(this, void 0, void 0, function* () {
952-
var _a;
953-
let repoId = github_1.context.payload.repository.id;
952+
var _a, _b;
953+
const repoId = (_a = github_1.context.payload.repository) === null || _a === void 0 ? void 0 : _a.id;
954954
const allComments = yield octokit_1.octokit.paginate(octokit_1.octokit.rest.issues.listComments, {
955955
owner: github_1.context.repo.owner,
956956
repo: github_1.context.repo.repo,
957957
issue_number: github_1.context.issue.number,
958958
per_page: 100
959959
});
960-
let listOfPRComments = [];
961-
let filteredListOfPRComments = [];
960+
const listOfPRComments = [];
961+
const filteredListOfPRComments = [];
962962
for (const prComment of allComments) {
963+
if (!prComment.user)
964+
continue;
963965
listOfPRComments.push({
964966
name: prComment.user.login,
965967
id: prComment.user.id,
966968
comment_id: prComment.id,
967-
body: (_a = prComment.body) === null || _a === void 0 ? void 0 : _a.trim().toLowerCase(),
969+
body: (_b = prComment.body) === null || _b === void 0 ? void 0 : _b.trim().toLowerCase(),
968970
created_at: prComment.created_at,
969-
repoId: repoId,
971+
repoId,
970972
pullRequestNo: github_1.context.issue.number
971973
});
972974
}
@@ -983,7 +985,7 @@ function signatureWithPRComment(committerMap, committers) {
983985
/*
984986
* checking if the commented users are only the contributors who has committed in the same PR (This is needed for the PR Comment and changing the status to success when all the contributors has reacted to the PR)
985987
*/
986-
const onlyCommitters = committers.filter((committer) => filteredListOfPRComments.some(commentedCommitter => committer.id == commentedCommitter.id));
988+
const onlyCommitters = committers.filter(committer => filteredListOfPRComments.some(commentedCommitter => committer.id == commentedCommitter.id));
987989
const commentedCommitterMap = {
988990
newSigned,
989991
onlyCommitters,

src/checkAllowList.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { CommittersDetails } from './interfaces'
1+
import { Committer } from './interfaces'
22

33
import * as input from './shared/getInputs'
44

@@ -19,8 +19,8 @@ function isUserAllowListed(committer: string): boolean {
1919
}
2020

2121
export function checkAllowList(
22-
committers: CommittersDetails[]
23-
): CommittersDetails[] {
22+
committers: Committer[]
23+
): Committer[] {
2424
return committers.filter(
2525
committer => committer && !isUserAllowListed(committer.name)
2626
)

src/graphql.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { octokit } from './octokit'
22
import { context } from '@actions/github'
3-
import { CommittersDetails } from './interfaces'
3+
import { Committer } from './interfaces'
44
import { errorMessage } from './shared/errors'
55

66
interface GraphQLUser {
@@ -64,10 +64,10 @@ query($owner:String! $name:String! $number:Int! $cursor:String){
6464
}
6565
}`
6666

67-
export default async function getCommitters(): Promise<CommittersDetails[]> {
67+
export default async function getCommitters(): Promise<Committer[]> {
6868
try {
6969
const seenNames = new Set<string>()
70-
const committers: CommittersDetails[] = []
70+
const committers: Committer[] = []
7171
let cursor: string | null = null
7272
let hasNextPage = true
7373

@@ -82,7 +82,7 @@ export default async function getCommitters(): Promise<CommittersDetails[]> {
8282
const page = response.repository.pullRequest.commits
8383
for (const edge of page.edges) {
8484
const actor = extractUserFromCommit(edge.node.commit)
85-
const user: CommittersDetails = {
85+
const user: Committer = {
8686
name: actor.login || actor.name || '',
8787
id: actor.databaseId || 0,
8888
pullRequestNo: context.issue.number

src/interfaces.ts

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,38 @@
1-
export interface CommitterMap {
2-
signed: CommittersDetails[]
3-
notSigned: CommittersDetails[]
4-
unknown: CommittersDetails[]
5-
}
6-
7-
export interface ReactedCommitterMap {
8-
newSigned: CommittersDetails[]
9-
onlyCommitters?: CommittersDetails[]
10-
allSignedFlag: boolean
11-
}
12-
13-
export interface CommittersDetails {
1+
/**
2+
* A committer of a pull request, derived from the GitHub GraphQL API.
3+
* The minimal identity used for allow-list checks and signature lookup.
4+
*/
5+
export interface Committer {
146
name: string
157
id: number
168
pullRequestNo?: number | undefined
17-
created_at?: string | undefined
18-
updated_at?: string | undefined
9+
}
10+
11+
/**
12+
* A PR comment that matches the configured "sign phrase". Carries the
13+
* commenter identity (same shape as Committer) plus the comment metadata
14+
* needed to persist it. Field types line up with Signature so a
15+
* SigningComment is directly assignable to Signature.
16+
*/
17+
export interface SigningComment extends Committer {
1918
comment_id?: number | undefined
2019
body?: string | undefined
20+
created_at?: string | undefined
2121
repoId?: number | undefined
2222
}
2323

24+
export interface CommitterMap {
25+
signed: Committer[]
26+
notSigned: Committer[]
27+
unknown: Committer[]
28+
}
29+
30+
export interface ReactedCommitterMap {
31+
newSigned: SigningComment[]
32+
onlyCommitters?: Committer[] | undefined
33+
allSignedFlag: boolean
34+
}
35+
2436
/** Shape of a single record in the signatures JSON file. */
2537
export interface Signature {
2638
name: string

src/pullrequest/pullRequestComment.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,15 @@ import signatureWithPRComment from './signatureComment'
44
import { commentContent } from './pullRequestCommentContent'
55
import {
66
CommitterMap,
7-
CommittersDetails,
7+
Committer,
88
ReactedCommitterMap
99
} from '../interfaces'
1010
import { getUseDcoFlag } from '../shared/getInputs'
1111
import { errorMessage } from '../shared/errors'
1212

1313
export default async function prCommentSetup(
1414
committerMap: CommitterMap,
15-
committers: CommittersDetails[]
15+
committers: Committer[]
1616
) {
1717
const signed = committerMap?.notSigned && committerMap?.notSigned.length === 0
1818

@@ -124,10 +124,10 @@ function prepareCommiterMap(
124124

125125
function prepareAllSignedCommitters(
126126
committerMap: CommitterMap,
127-
signedInPrCommitters: CommittersDetails[],
128-
committers: CommittersDetails[]
127+
signedInPrCommitters: Committer[],
128+
committers: Committer[]
129129
): boolean {
130-
let allSignedCommitters = [] as CommittersDetails[]
130+
let allSignedCommitters = [] as Committer[]
131131
/*
132132
* 1) already signed committers in the file 2) signed committers in the PR comment
133133
*/

0 commit comments

Comments
 (0)