diff --git a/src/github.ts b/src/github.ts index 0f1cc14..0be4189 100644 --- a/src/github.ts +++ b/src/github.ts @@ -422,8 +422,20 @@ export class GitHubClient { ]); const fileCap = maxFiles != null && maxFiles > 0 ? maxFiles : this.config.maxFilesPerReview; - const files: FileChange[] = filesResponse.data - .filter((f) => !this.isIgnored(f.filename)) + const ignoredFiles: string[] = []; + const afterIgnore = filesResponse.data.filter((f) => { + if (this.isIgnored(f.filename)) { + ignoredFiles.push(f.filename); + return false; + } + return true; + }); + // Files beyond the max-files cap are dropped from review. Record them (like + // ignoredFiles) so the reviewer can surface "N files beyond the review cap" + // instead of silently returning a green status when the cap is the only + // reason nothing was reviewed. + const cappedFiles: string[] = afterIgnore.slice(fileCap).map((f) => f.filename); + const files: FileChange[] = afterIgnore .slice(0, fileCap) .map((f) => ({ filename: f.filename, @@ -445,6 +457,8 @@ export class GitHubClient { headSha: pr.data.head.sha, defaultBranch: pr.data.base.repo.default_branch, files, + ignoredFiles, + cappedFiles, isDraft: pr.data.draft, labels: pr.data.labels.map((l) => l.name ?? ""), author: pr.data.user?.login, @@ -771,6 +785,13 @@ export class GitHubClient { (c) => c.user?.type === "Bot" && c.body?.includes(marker) ); + // upsertComment is generic — it backs the status, walkthrough, sticky, + // issue-summary and pre-merge comments alike. Derive a label from the + // marker so the log names the actual comment type instead of always saying + // "walkthrough comment", which made distinct comments look like duplicate + // walkthroughs in the logs. + const commentLabel = marker.replace(//, "").trim() || marker; + if (existing) { await octokit.issues.updateComment({ owner, @@ -778,15 +799,15 @@ export class GitHubClient { comment_id: existing.id, body, }); - log.info({ commentId: existing.id }, "Updated existing walkthrough comment"); + log.info({ commentId: existing.id, comment: commentLabel }, "Updated existing comment"); } else { - await octokit.issues.createComment({ + const created = await octokit.issues.createComment({ owner, repo, issue_number: pullNumber, body, }); - log.info("Created new walkthrough comment"); + log.info({ commentId: created?.data?.id, comment: commentLabel }, "Created new comment"); } } diff --git a/src/reviewer.ts b/src/reviewer.ts index f3ffa6e..c3e8095 100644 --- a/src/reviewer.ts +++ b/src/reviewer.ts @@ -583,6 +583,57 @@ export class Reviewer { if (context.files.length === 0) { log.info("No reviewable files in PR, skipping"); + + // Distinguish "PR only contains files we deliberately skip (a minified + // bundle, lockfile, generated output)" from a genuinely empty diff. In + // the former case, post an info comment naming the skipped files so the + // PR doesn't just show a green check with no explanation — and so the + // "…reviewing… hang tight" status comment gets resolved rather than + // left hanging forever. `filesSkippedSimilar` is excluded on purpose: + // in incremental mode it means nothing new changed (a true no-op). + const skippedGroups: Array<{ label: string; files: string[] }> = [ + { label: "Ignored (minified, generated, or lockfile)", files: context.ignoredFiles ?? [] }, + { label: "Excluded by path filters", files: filesIgnoredByPathFilter.map((f) => f.path) }, + { label: "Beyond the max-files-per-review cap", files: context.cappedFiles ?? [] }, + { label: "No reviewable changes", files: filesSkippedTrivial }, + ].filter((g) => g.files.length > 0); + + if (skippedGroups.length > 0) { + const totalSkipped = skippedGroups.reduce((n, g) => n + g.files.length, 0); + // Cap each group's rendered list: a monorepo PR touching hundreds of + // lockfiles / dist artifacts could otherwise blow past GitHub's ~65 KB + // comment-body limit, making the upsert throw — which would leave the + // stale "reviewing…" comment in place, the very thing this branch exists + // to resolve. Overflow is summarised as "…and N more". + const MAX_FILES_PER_GROUP = 50; + const body = + STATUS_MARKER + "\n" + + `> ⏭️ **DiffSentry** — no reviewable changes. ` + + `This ${mode === "full" ? "pull request" : "update"} only contains ` + + `${totalSkipped} file${totalSkipped === 1 ? "" : "s"} DiffSentry skips, ` + + `so there's nothing to review.\n\n` + + skippedGroups + .map((g) => { + const shown = g.files.slice(0, MAX_FILES_PER_GROUP); + const overflow = g.files.length - shown.length; + return ( + `
${g.label} (${g.files.length})\n\n` + + shown.map((f) => `- \`${f}\``).join("\n") + + (overflow > 0 ? `\n- …and ${overflow} more` : "") + + `\n
` + ); + }) + .join("\n\n"); + try { + await this.github.upsertComment( + installationId, owner, repo, pullNumber, body, STATUS_MARKER, signal + ); + log.info({ totalSkipped }, "Posted no-reviewable-files status comment"); + } catch (err) { + log.warn({ err }, "Failed to post no-reviewable-files status comment"); + } + } + if (repoConfig.reviews?.commit_status !== false) { await this.github.setCommitStatus( installationId, owner, repo, context.headSha, diff --git a/src/types.ts b/src/types.ts index 09173cb..b7658f8 100644 --- a/src/types.ts +++ b/src/types.ts @@ -385,6 +385,20 @@ export interface PRContext { /** Repo default branch (e.g. "main"). Authoritative source for .diffsentry.yaml. */ defaultBranch?: string; files: FileChange[]; + /** + * Filenames dropped by the operator-level ignore list (`ignoredPatterns` — + * e.g. *.min.js, *.map, dist/**, lockfiles) before `files` was built. Kept so + * the reviewer can tell "PR only contains ignored files (minified bundle)" + * apart from a genuinely empty diff and surface which files were skipped. + */ + ignoredFiles?: string[]; + /** + * Filenames dropped by the `maxFilesPerReview` cap — the post-`ignoredFiles` + * overflow beyond `.slice(0, fileCap)`. Recorded so the empty-files status + * comment can explain "N files beyond the review cap" rather than silently + * returning when the cap is the only reason nothing was reviewed. + */ + cappedFiles?: string[]; isDraft?: boolean; labels?: string[]; author?: string;