Skip to content

chore: refactor knip CI to fail on issues and simplify comment format#2003

Open
brandon-pereira wants to merge 5 commits intomainfrom
brandon/knip-error
Open

chore: refactor knip CI to fail on issues and simplify comment format#2003
brandon-pereira wants to merge 5 commits intomainfrom
brandon/knip-error

Conversation

@brandon-pereira
Copy link
Copy Markdown
Member

Summary

Refactors the Knip CI workflow to be simpler and stricter:

  • Removed main branch comparison — no longer checks out main or runs knip on it. The workflow only analyzes the PR branch.
  • Comment only on failure — if knip finds 0 issues, no comment is posted. If a stale comment exists from a previous push, it's deleted.
  • Simplified comment format — replaced the comparison table with a flat per-category breakdown listing each issue directly.
  • Job fails on issuescore.setFailed() is called when knip detects issues, so the check goes red.
  • Removed excluded categoriesenumMembers and duplicates are excluded in knip.json, so they were removed from the workflow.
  • Added test fixtures — an unused export and unused file to verify the workflow triggers correctly on this PR.

How to test locally or on Vercel

  1. Check the Knip CI action on this PR — it should fail and post a comment listing the test fixtures.
  2. Verify the comment shows Unused files and Unused exports sections with the test items.
  3. After confirming, the test fixtures (KNIP_TEST_UNUSED_EXPORT in defaults.ts and knipTestUnusedFile.ts) should be removed before merge.

References

- Remove main branch comparison (no more table/diff)
- Only comment when issues are found; delete stale comments when clean
- Refactored comment to a flat per-category breakdown of issues
- Job fails via core.setFailed() when knip finds issues
- Removed excluded categories (enumMembers, duplicates) from workflow
- Added test fixtures (unused export + unused file) to verify workflow
@brandon-pereira brandon-pereira added the ai-generated AI-generated content; review carefully before merging. label Mar 27, 2026
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 27, 2026

⚠️ No Changeset found

Latest commit: aeb51f6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 27, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Mar 30, 2026 8:44pm

Request Review

The knip JSON reporter nests unused files under issues[].files[] (not
a top-level files array). This fixes the parsing so unused files are
correctly reported in the PR comment.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 27, 2026

E2E Test Results

All tests passed • 107 passed • 3 skipped • 1111s

Status Count
✅ Passed 107
❌ Failed 0
⚠️ Flaky 3
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 27, 2026

PR Review

  • data.files (top-level unused files) are never parsed → The old code had a dedicated loop for (const f of data.files || []) to count/collect top-level unused file paths, then skipped files in the per-issue loop. The new parseResults() removes that top-level loop and looks for issue.files inside data.issues, but knip's JSON format puts unused files in the root data.files array as strings, not inside per-file issue objects. Unused files will silently count as 0 in every run.

  • ⚠️ No comparison to main means pre-existing knip issues will block all PRs → If main already has any knip violations, every PR touching any file will fail CI. There's no way to distinguish newly-introduced issues from pre-existing ones. Consider at minimum posting a note when issues exist but none are new, or document that main must be clean before merging this.

  • ⚠️ PR description says test fixtures must be removed before merge (KNIP_TEST_UNUSED_EXPORT in defaults.ts and knipTestUnusedFile.ts) — the diff shows only the workflow file changed, so these may have already been cleaned up, but confirm before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-generated AI-generated content; review carefully before merging.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant