Post an explanatory comment when a PR has only skippable files#77
Conversation
When a PR contained only files DiffSentry skips (a minified bundle, lockfile, or generated output), the review path hit the empty-files early return: it set a green commit status and returned without posting anything, leaving the "…reviewing… hang tight" status comment on the PR forever. getPRContext now captures the filenames dropped by the operator ignore list into a new PRContext.ignoredFiles field instead of discarding them. The empty-files branch uses that (plus path-filter and trivial/empty patches) to distinguish "only skippable files" from a genuinely empty diff, and in the former case upserts a status comment naming the skipped files grouped by reason. Because it upserts on STATUS_MARKER, this also resolves the stale in-review comment. Genuinely empty diffs and incremental no-ops (filesSkippedSimilar) stay silent — status only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
📝 WalkthroughWalkthroughFixed the empty-files early return so PRs with only skippable files now get an explanatory status comment listing those files by skip reason, which also resolves the stale in-review hang-tight comment. Changes
Sequence Diagram(s)sequenceDiagram
participant Reviewer
participant GitHubClient
participant PR
Reviewer->>GitHubClient: getPRContext
GitHubClient-->>Reviewer: files empty, ignoredFiles/cappedFiles set
Reviewer->>Reviewer: group skippable files by reason
Reviewer->>GitHubClient: upsertComment STATUS_MARKER
GitHubClient->>PR: create or update status comment
Reviewer->>GitHubClient: setCommitStatus success
Estimated code review effort🎯 2 (Simple) | ⏱️ ~20 minutes Suggested Labels
Risk AssessmentScore: 15/100 — 🟡 Moderate
Test Coverage Signal🔴 91 line(s) of production code added with no test changes.
🧭 Description Drift
Compares PR description claims to the actual diff. Update the description (or the code) so they tell the same story. 📊 Confidence breakdown2 findings — 🟢 1 high · 🟡 1 medium · 🔴 0 low. Treat the medium / low ones as hypotheses to verify. 🚥 Pre-merge checks | ✅ 5 | ❌ 0✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in your ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Generate docstrings (beta)
🧹 Simplify (beta)
🪄 Autofix unresolved comments (beta)
Comment |
![diffsentry[bot]](https://avatars.githubusercontent.com/in/3360270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
The PR correctly captures operator-ignored files into PRContext.ignoredFiles and posts a clarifying status comment when a PR has only skippable content, which resolves the stale "reviewing…" comment. One real bug remains: getPRContext still slices the filtered list to fileCap without recording the overflow, so a PR whose only reviewable files were dropped by the max-files cap is treated as a silent empty diff instead of being explained.
🧹 Nitpick comments (1)
src/reviewer.ts (1)
Line 605: Long skipped-file lists can blow past GitHub's comment body limit.🛠️ Refactor suggestion | 🟡 Minor
Long skipped-file lists can blow past GitHub's comment body limit.
🤔 Medium confidence — verify against intent before acting.
The new status body dumps every skipped path into nested
<details>blocks with no cap. A monorepo PR that only touches hundreds of lockfiles /dist/**artifacts can easily exceed GitHub's ~65 KB issue-comment limit and makeupsertCommentfail — which this code already catches and only warns on, so the PR is back to a green check with the permanent "reviewing…" comment.Cap each group (or the total body) and append an "and N more…" line so the upsert always has a chance to succeed and clear the in-review marker.
🔧 Proposed fix
const MAX_FILES_PER_GROUP = 50; skippedGroups .map((g) => { const shown = g.files.slice(0, MAX_FILES_PER_GROUP); const overflow = g.files.length - shown.length; return ( `<details><summary>${g.label} (${g.files.length})</summary>\n\n` + shown.map((f) => `- \`${f}\``).join("\n") + (overflow > 0 ? `\n- …and ${overflow} more` : "") + `\n</details>` ); }) .join("\n\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/reviewer.ts around line 605, when building the no-reviewable-files status comment body, cap the number of file paths rendered per skipped group (e.g. 50) and append an "…and N more" line for any overflow so the comment stays under GitHub's issue-comment size limit and the STATUS_MARKER upsert can still clear the in-review comment.♻️ Suggested fix
const MAX_FILES_PER_GROUP = 50; skippedGroups .map((g) => { const shown = g.files.slice(0, MAX_FILES_PER_GROUP); const overflow = g.files.length - shown.length; return ( `<details><summary>${g.label} (${g.files.length})</summary>\n\n` + shown.map((f) => `- \`${f}\``).join("\n") + (overflow > 0 ? `\n- …and ${overflow} more` : "") + `\n</details>` ); }) .join("\n\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/reviewer.ts around line 605, when building the no-reviewable-files status comment body, cap the number of file paths rendered per skipped group (e.g. 50) and append an "…and N more" line for any overflow so the comment stays under GitHub's issue-comment size limit and the STATUS_MARKER upsert can still clear the in-review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `src/reviewer.ts`:
- Line 605: In src/reviewer.ts around line 605, when building the no-reviewable-files status comment body, cap the number of file paths rendered per skipped group (e.g. 50) and append an "…and N more" line for any overflow so the comment stays under GitHub's issue-comment size limit and the STATUS_MARKER upsert can still clear the in-review comment.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 219181f4-bf36-4a67-a422-30aebc940f79
📥 Commits
Reviewing files at 8fa3858 (base SHA unavailable).
📒 Files selected for processing (3)
src/github.tssrc/reviewer.tssrc/types.ts
📌 Status — last updated
|
| Risk score | 15/100 (Moderate) ▁▄█▄ |
| Unresolved threads | 0 |
| Failing checks | 0 |
| Pending checks | 1 |
| Files reviewed | 3 |
| Updated | 2026-07-09 21:06Z |
Live-updated by DiffSentry on every push. Use @diffsentry ship for a verdict, @diffsentry timeline for full history.
Two fixes from reviewing PR #77's own review logs: - upsertComment hardcoded "Created/Updated ... walkthrough comment" even though it backs the status, walkthrough, sticky and issue-summary comments alike. Distinct comments therefore looked like duplicate walkthroughs in the logs. Derive a label from the marker and log the created comment id too. - Cap each skipped-file group in the no-reviewable-files status comment at 50 entries with an "…and N more" overflow line, so a monorepo PR touching hundreds of lockfiles/dist artifacts can't exceed GitHub's ~65 KB comment limit and make the upsert throw — which would leave the stale "reviewing…" comment this branch exists to resolve. (Addresses DiffSentry finding on #77.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
The change correctly threads operator-ignored filenames through PRContext and posts a STATUS_MARKER comment when a PR is empty only because of skippable files, which also resolves the stale in-review comment. The overflow cap, try/catch, and exclusion of filesSkippedSimilar all look intentional and sound. Two real bugs remain: the :next_track_button: emoji shortcode will render as literal text on GitHub (should be the ⏭️ unicode used in the PR description), and the createComment path in upsertComment now assumes created.data.id exists without a guard.
🔎 Issues not tied to a specific line (1)
These findings concern the change as a whole (for example, the diff versus the PR description) and can't be attached to a single changed line.
Description claims types.ts change that is absent from the diff
🤔 Medium confidence — verify against intent before acting.
The PR description and DiffSentry Summary both state that src/types.ts adds ignoredFiles?: string[] to PRContext, but src/types.ts does not appear among the changed files. Only src/github.ts and src/reviewer.ts are modified. If the type was not updated, tsc --noEmit would normally reject the new ignoredFiles field on the object returned from getPRContext.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
Reconcile the PR description with the actual diff. Description claims types.ts change that is absent from the diff The PR description and DiffSentry Summary both state that `src/types.ts` adds `ignoredFiles?: string[]` to `PRContext`, but `src/types.ts` does not appear among the changed files. Only `src/github.ts` and `src/reviewer.ts` are modified. If the type was not updated, `tsc --noEmit` would normally reject the new `ignoredFiles` field on the object returned from `getPRContext`.
🧹 Nitpick comments (2)
src/reviewer.ts (1)
Line 611: GitHub will not render the ⏭️ shortcode.
⚠️ Potential issue | 🟡 MinorGitHub will not render the ⏭️ shortcode.
The comment body builds
> :next_track_button: **DiffSentry** — no reviewable changes.... GitHub issue/PR markdown does not expand emoji shortcodes the way Slack does, so reviewers will see the literal:next_track_button:text rather than the skip icon. The PR description's own example uses the unicode⏭️glyph, which is the right form here — and matches how other status comments in this codebase typically render icons.🔧 Proposed fix
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` +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/reviewer.ts around line 611, replace the `:next_track_button:` emoji shortcode in the no-reviewable-files status comment body with the unicode `⏭️` glyph. GitHub issue/PR markdown does not expand shortcodes, so the shortcode currently renders as literal text. Keep the rest of the template string unchanged.♻️ Suggested fix
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` +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/reviewer.ts around line 611, replace the `:next_track_button:` emoji shortcode in the no-reviewable-files status comment body with the unicode `⏭️` glyph. GitHub issue/PR markdown does not expand shortcodes, so the shortcode currently renders as literal text. Keep the rest of the template string unchanged.src/github.ts (1)
Line 804: created.data.id may throw if the createComment response shape is unexpected.
⚠️ Potential issue | 🟡 Minorcreated.data.id may throw if the createComment response shape is unexpected.
🤔 Medium confidence — verify against intent before acting.
The create path now logs
created.data.idimmediately afteroctokit.issues.createComment(...). If the response is missingdata(rare network/proxy/mock edge cases), this becomes a TypeError after the comment was already created successfully — turning a successful write into a thrown error for the caller. The update path already usesexisting.idfrom a prior listing, so only the create branch is newly exposed.Either keep the previous string-only log for create, or read the id defensively:
log.info({ commentId: created?.data?.id, comment: commentLabel }, "Created new comment");🔧 Proposed fix
const created = await octokit.issues.createComment({ owner, repo, issue_number: pullNumber, body, }); log.info({ commentId: created?.data?.id, comment: commentLabel }, "Created new comment");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/github.ts around line 804, make the createComment success log resilient to a missing response payload. Prefer optional chaining when reading the new comment id (`created?.data?.id`) so a successful create cannot throw solely because the log line assumes `created.data.id` is always present.♻️ Suggested fix
const created = await octokit.issues.createComment({ owner, repo, issue_number: pullNumber, body, }); log.info({ commentId: created?.data?.id, comment: commentLabel }, "Created new comment");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/github.ts around line 804, make the createComment success log resilient to a missing response payload. Prefer optional chaining when reading the new comment id (`created?.data?.id`) so a successful create cannot throw solely because the log line assumes `created.data.id` is always present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `src/reviewer.ts`:
- Line 611: In src/reviewer.ts around line 611, replace the `:next_track_button:` emoji shortcode in the no-reviewable-files status comment body with the unicode `⏭️` glyph. GitHub issue/PR markdown does not expand shortcodes, so the shortcode currently renders as literal text. Keep the rest of the template string unchanged.
In `src/github.ts`:
- Line 804: In src/github.ts around line 804, make the createComment success log resilient to a missing response payload. Prefer optional chaining when reading the new comment id (`created?.data?.id`) so a successful create cannot throw solely because the log line assumes `created.data.id` is always present.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 6d34a6a3-3cff-45a6-99de-e55c5c93a00d
📥 Commits
Reviewing files that changed from 8fa3858 to 8073c78. Previously-reviewed commits are not re-reviewed.
📒 Files selected for processing (2)
src/github.tssrc/reviewer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
src/types.ts
Address two DiffSentry findings on #77: - Replace the ⏭️ emoji shortcode in the no-reviewable-files status comment with the literal ⏭️ glyph, which is guaranteed to render on GitHub and matches the PR-description example. - Guard the new "Created new comment" log with optional chaining (created?.data?.id) so an unexpected createComment response shape can't turn a successful write into a thrown TypeError from a log line. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
The PR correctly captures operator-ignored filenames into PRContext and uses them in the empty-files branch to post a STATUS_MARKER comment that both explains the skip and clears the stale hang-tight comment. One logic gap remains: files that were dropped by maxFilesPerReview after the ignore filter never appear in files, ignoredFiles, path-filter, or trivial lists, so a PR that is only-skippable because of that cap (or a mix of ignored + capped) can still take the silent green-status path and leave the reviewing comment in place. Logging and typing changes look solid; the comment body already caps per-group size and wraps the upsert in try/catch.
🧹 Nitpick comments (1)
src/github.ts (1)
Line 787: Marker-to-label stripping is a bit fragile for non-standard markers.🧹 Nitpick | 🟢 Trivial
Marker-to-label stripping is a bit fragile for non-standard markers.
🤔 Medium confidence — verify against intent before acting.
marker.replace(/<!--\s*/, "").replace(/\s*-->/, "").trim()only peels a leading<!--and a trailing-->. It is fine for the currentSTATUS_MARKER/ walkthrough-style constants, but a marker with extra markup or no HTML comment wrapper will log a noisy raw string. A small helper that also falls back to the full marker (already present) is enough; optional nicety would be to normalize internal whitespace or take an explicit label argument onupsertComment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/github.ts around line 787, optionally harden commentLabel derivation (e.g. match a full <!-- ... --> block, or accept an explicit label param on upsertComment) so non-HTML markers still log cleanly; keep the existing fallback to the raw marker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In src/github.ts around line 787, optionally harden commentLabel derivation (e.g. match a full <!-- ... --> block, or accept an explicit label param on upsertComment) so non-HTML markers still log cleanly; keep the existing fallback to the raw marker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
In `src/github.ts`:
- Line 425: In src/github.ts around line 425, after filtering with isIgnored and before/while applying .slice(0, fileCap), capture filenames dropped solely by the maxFilesPerReview cap (e.g. cappedOutFiles or truncatedFiles) onto PRContext alongside ignoredFiles. In src/types.ts add the matching optional field. In src/reviewer.ts around the empty-files skippedGroups construction, include those capped-out filenames as another skipped group (or at least count them toward totalSkipped) so a PR that is empty only because of the file cap still gets a STATUS_MARKER explanation and clears the hang-tight comment.
- Line 787: In src/github.ts around line 787, optionally harden commentLabel derivation (e.g. match a full <!-- ... --> block, or accept an explicit label param on upsertComment) so non-HTML markers still log cleanly; keep the existing fallback to the raw marker.
In `src/reviewer.ts`:
- Line 596: In src/reviewer.ts around line 596, extend skippedGroups to include files (or a count) dropped by maxFilesPerReview once PRContext exposes them from getPRContext. Keep the existing labels for ignored / path-filter / trivial, and only post when at least one group is non-empty so genuine empty diffs and filesSkippedSimilar no-ops stay silent.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 5a2d8e15-4817-4f97-8d4a-066305879d2f
📥 Commits
Reviewing files at 312bfc2 (base SHA unavailable).
📒 Files selected for processing (3)
src/github.tssrc/reviewer.tssrc/types.ts
Address DiffSentry findings on #77: files removed by the maxFilesPerReview cap (the post-ignore overflow beyond .slice(0, fileCap)) were recorded nowhere, so a >cap PR whose remaining reviewable files were all trivial/ similar could still hit the silent "no reviewable files" green-status path — the exact failure mode this PR exists to fix, via a different drop reason. getPRContext now captures that overflow into PRContext.cappedFiles, and the empty-files branch adds a "Beyond the max-files-per-review cap" skipped group so the explanatory status comment fires (and clears the hang-tight comment) whenever the PR had files and every one was deliberately dropped. Report-only: capped files remain unreviewed (the cap is intentional), they are just no longer silently discarded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
The change correctly captures operator-ignored and max-files-capped filenames on PRContext and posts a STATUS_MARKER comment when a PR has only skippable files, which also resolves the stale hang-tight comment. The empty-files branch is well structured with list caps and error handling. One major issue: cappedFiles is added to the returned context and documented in types, but PRContext never declares the field, so TypeScript should reject this under tsc --noEmit.
🔎 Issues not tied to a specific line (2)
These findings concern the change as a whole (for example, the diff versus the PR description) and can't be attached to a single changed line.
📝 Documentation | 🟡 Minor
PR description omits cappedFiles, but the diff also records max-files overflow.
The PR description and DiffSentry summary only mention ignoredFiles and the skippable-files status comment. The actual diff also introduces cappedFiles end-to-end (capture in getPRContext, type field — currently missing — and a "Beyond the max-files-per-review cap" group in the empty-files comment). That is a useful extension of the same problem, but it should be called out in the PR body so reviewers and future bisect notes know the cap path is intentional, not drive-by.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
Update the PR description / summary to mention that getPRContext also records files dropped by maxFilesPerReview into PRContext.cappedFiles, and that the empty-files status comment groups those under a "Beyond the max-files-per-review cap" section alongside operator-ignored, path-filter, and trivial skips.
cappedFiles tracking and status group omitted from description
🤔 Medium confidence — verify against intent before acting.
The PR description only claims capturing ignoredFiles and grouping skips as operator-ignored / path-filter-excluded / trivial-or-empty. The diff also adds cappedFiles in getPRContext (src/github.ts), PRContext.cappedFiles in src/types.ts, and a fourth skipped group labeled "Beyond the max-files-per-review cap" in src/reviewer.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
Reconcile the PR description with the actual diff. cappedFiles tracking and status group omitted from description The PR description only claims capturing `ignoredFiles` and grouping skips as operator-ignored / path-filter-excluded / trivial-or-empty. The diff also adds `cappedFiles` in `getPRContext` (`src/github.ts`), `PRContext.cappedFiles` in `src/types.ts`, and a fourth skipped group labeled "Beyond the max-files-per-review cap" in `src/reviewer.ts`.
🪄 Autofix (Beta)
Fix all unresolved DiffSentry comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: .diffsentry.yaml
Review profile: ASSERTIVE
Run ID: 463c70a9-0f08-4ce9-af96-ba77aed209be
📥 Commits
Reviewing files that changed from 312bfc2 to cc66f4b. Previously-reviewed commits are not re-reviewed.
📒 Files selected for processing (3)
src/github.tssrc/reviewer.tssrc/types.ts
|
Addressed the latest review (documentation findings):
No code changes in this step — description only. |
📓 Changelog Entry### Fixed
- Posted an explanatory status comment listing skipped files by reason when a PR only contains skippable files (minified bundles, lockfiles, generated output, path-filtered, or beyond the max-files cap), and cleared the stale “reviewing… hang tight” comment that previously remained forever (#77)Drop into your CHANGELOG.md. Re-run with |
|
@diffsentry release-notes |
📣 Release Notes✨ What's new
🛠 Improvements
🐛 Fixes
Marketing-speak version of this PR. Re-run with |
1 participant
What changed
DiffSentry posted nothing when a PR contained only files it skips — a minified bundle, lockfile, or generated output. The review path hit the empty-files early return: it set a green commit status and returned without a comment, leaving the "…reviewing… hang tight" status comment on the PR forever.
Now, when a PR has files but every one was deliberately dropped, DiffSentry upserts an explanatory status comment (on
STATUS_MARKER, so it also clears the stale hang-tight comment) that names the skipped files grouped by reason:Core change
src/github.ts(getPRContext): capture the filenames the review path used to discard — both operator-ignored files (*.min.js,*.map,dist/**, lockfiles, …) and files dropped by themaxFilesPerReviewcap — intoPRContext.ignoredFiles/PRContext.cappedFiles. The reviewer previously never learned these files existed.src/types.ts: addignoredFiles?: string[]andcappedFiles?: string[]toPRContext.src/reviewer.ts: the empty-files branch distinguishes "only skippable files" from a genuinely empty diff, and upserts the status comment listing skipped files grouped by reason — operator-ignored, path-filter-excluded, beyond the max-files-per-review cap, and trivial/empty-patch. Each group is capped at 50 entries with an "…and N more" overflow line so the body can't blow past GitHub's ~65 KB comment limit (which would make the upsert throw and resurrect the hang-tight comment).Follow-up polish (from review on this branch)
upsertCommentno longer hardcodes "walkthrough comment" for every comment type — it backs the status, walkthrough, sticky and issue-summary comments alike, so those log lines made distinct comments look like duplicate walkthroughs. It now derives a label from the marker and logs the created comment id. The create-comment log is guarded withcreated?.data?.idso a log line can't throw on an unexpected response shape.⏭️glyph rather than a:next_track_button:shortcode.Why
A green check with no explanation (and a perpetual "reviewing…" comment) looked like DiffSentry was broken or stuck. Now it says plainly why there was nothing to review, for every deliberate-skip reason.
Scope / non-goals
filesSkippedSimilar— nothing new changed) stay silent (status only), by design.maxFilesPerReviewcap is intentional; this change just makes the drop visible instead of silent.Verification
tsc --noEmitcleanreviewPR(deeply integrated with the GitHub client, AI, and DB).🤖 Generated with Claude Code
Summary
Fixed the empty-files early return so PRs with only skippable files now get an explanatory status comment listing those files by skip reason, which also resolves the stale in-review hang-tight comment.
Changes
src/github.tssrc/reviewer.tssrc/types.ts