mirror of
https://github.com/millionco/react-doctor.git
synced 2026-09-14 20:00:24 +08:00
Aiden Bai
30f05ef532
* refactor(action): move legacy composite action to actions/inspect/ The new root action.yml will host the PR Review pipeline. The existing 'scan + sticky comment' composite action now lives at actions/inspect/action.yml so consumers can opt into either action explicitly: - millionco/react-doctor@v1 -> PR Review (new) - millionco/react-doctor/actions/inspect@v1 -> Legacy scan flow Updates the github-action.test.ts contract test to read from the new path. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com> * feat(action): add React Doctor Review composite action Self-hosted GitHub Action that mirrors the hosted react.review bot's doctor-only review pipeline. No hosted service required — everything runs in-process on the GitHub Actions runner. What the action does on every pull_request (opened/synchronize/reopened): - Reads the event payload from GITHUB_EVENT_PATH and extracts the owner/repo/PR number/head SHA/base ref. Detects fork PRs via head.repo vs base.repo full_name mismatch. - Resolves the merge base via octokit.repos.compareCommitsWithBasehead, using ${headOwner}:${headRef} as the head identifier for fork PRs. - Materializes the base commit alongside the runner's checkout of head via 'git fetch --depth=1 <remote> <baseSha>' + 'git worktree add --detach <baseDir> <baseSha>'. The base remote URL embeds https://x-access-token:<token>@github.com/... so private repos work. - Lists changed files via octokit.pulls.listFiles (paginated) and derives addedLineContents: Map<lineNumber, content> per file from the unified-diff patch — required for inline-comment line resolution. - Runs react-doctor's diagnose() API in-process on both the head checkout and the base worktree. Pre-discovers React subprojects via workspaces in package.json + pnpm-workspace.yaml (using discoverReactSubprojects from @react-doctor/project-info), fans out per project, combines diagnostics with paths remapped to be relative to the repo root, and weight-averages scores by source file count. - Diffs head diagnostics against base diagnostics using a (relativePath, rule, message) key with count deltas (not boolean presence) so duplicate diagnostics are counted correctly. - Inline comment candidates are net-new diagnostics with severity === 'error' whose line landed on a + line in the diff. - Reconciles existing inline threads via GraphQL repository.pullRequest.reviewThreads. Threads whose body contains the INLINE_COMMENT_MARKER and whose threadKey matches a current candidate are skipped. Threads whose key is no longer in the candidate set get PATCHed with '✅ Addressed in <headSha>' and resolved via the resolveReviewThread GraphQL mutation. - Posts remaining candidates via octokit.pulls.createReview({ event: 'COMMENT' }), capped at MAX_INLINE_COMMENTS_COUNT (25) per run. - Upserts the sticky summary comment identified by the STICKY_COMMENT_MARKER HTML marker. Posts a pending-review comment before analysis starts, swaps it for the final regression/no-issues body once diagnostics are computed, and falls back to an analysis- failure body in the catch path. - Creates a check run on the head SHA before analysis, then completes it with the assessment body (score line, new-diagnostics list, project metadata table). Edge cases handled: - Fork PRs: warns when head.repo differs from base.repo and gracefully catches write failures on inline / sticky posts when the workflow token lacks write access. Consumers can pass a PAT or App installation token via the 'token' input to enable fork-PR review. - NoReactDependencyError / PackageJsonNotFoundError / ProjectNotFoundError: concludes the check run as 'skipped' with a 'not a React project' summary and deletes the pending comment instead of loud-failing. - Diagnostic path resolution tries: absolute under root, absolute under project root, relative + project-prefixed, and 'already prefixed' in that order. - Cleans up the base worktree via 'git worktree remove --force' in a finally block. Token strategy (dual-mode): - Default: workflow GITHUB_TOKEN (action.yml default). Comments author as github-actions[bot]. Same-repo PRs work out of the box; fork PRs get warnings and best-effort posts. - Override: pass a PAT or App installation token via the 'token' input for fork-PR support or branded identity. Not in scope (intentionally out): - No LLM / agent layer — doctor-only review. - No Vercel Sandbox dependency — diagnose() runs in-process. - No Upstash rate limit / no-react cache — single-repo CI doesn't need multi-tenant guardrails. - No OIDC token-broker mode — that flow lives in the hosted react.review service; this action is standalone. File layout: - action.yml at the repo root (composite, runs tsx against the entrypoint). Installs only the @react-doctor/review-action and react-doctor workspace closures via pnpm --filter to keep cold-start install footprint minimal. - actions/review/ is a new workspace package (@react-doctor/review-action) with three TypeScript modules: - src/pipeline.ts — pure helpers (diff parser, diagnose-across- workspace, computeDiagnosticsDelta, formatters, inline-candidate builder, check-run assessment). - src/github.ts — Octokit IO (compareCommitsWithBasehead, listFiles, sticky upsert/delete, inline review, GraphQL thread reconciliation, check-run create/complete). - src/index.ts — entrypoint orchestration. - src/constants.ts holds magic numbers (MAX_INLINE_COMMENTS_COUNT, sticky/inline markers, check-run name, GraphQL page size). Dogfood: - .github/workflows/doctor-action.yml runs the action against this repo's own PRs via 'uses: ./'. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com> * docs(readme): document Review and inspect actions Adds a 'PR Review (recommended)' section showing the new root action and keeps the previous scan + sticky comment flow under 'Scan + sticky comment (legacy)' pointing at millionco/react-doctor/actions/inspect@main. Existing references to 'millionco/react-doctor@main' for the legacy inputs (directory/verbose/project/diff/github-token/fail-on/offline/ annotations/node-version) are repointed to the new millionco/react-doctor/actions/inspect@main path so existing recipes keep working without breaking on the new Review action's inputs. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com> * fix(action): disambiguate workspace filter to skip website build Both the workspace root and packages/react-doctor are named 'react-doctor'. 'pnpm --filter react-doctor build' matched the root, whose build script is 'turbo run build' — which fans out to every workspace, including website. website#build then failed in CI because the filtered install (by design) did not pull in Next.js. Switch both pnpm invocations to path-style filters / direct cwd: - Install: --filter './packages/react-doctor...' (instead of bare name). - Build: cd packages/react-doctor && pnpm build (run the package's vp-pack script directly, bypassing the name collision). Verified locally: cd packages/react-doctor && rm -rf dist && pnpm build # OK pnpm install --frozen-lockfile \ --filter '@react-doctor/review-action...' \ --filter './packages/react-doctor...' # Scope: 6 of 9 Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com> * fix(action): build workspace deps via turbo so dist/ resolves at runtime The previous fix ran 'pnpm build' directly in packages/react-doctor, which only bundled that one package. vp pack treats workspace deps as external imports (Could not resolve '@react-doctor/core' ... treating it as an external dependency), so the resulting dist/index.js still 'import { ... } from "@react-doctor/core"' at runtime. The runtime import resolved to packages/core/dist/index.js — which was never built — and the action crashed: Error [ERR_MODULE_NOT_FOUND]: Cannot find module '.../packages/core/dist/index.js' imported from '.../packages/react-doctor/dist/index.js' Use 'turbo run build --filter=./packages/react-doctor' instead. The path filter avoids the workspace name collision (root + packages/ react-doctor share the name 'react-doctor'), and turbo's 'dependsOn: ["^build"]' in turbo.json triggers a topological build of the four upstream workspace deps (@react-doctor/types, /project- info, /core, oxlint-plugin-react-doctor) before react-doctor itself. Verified locally: 'rm -rf packages/*/dist && pnpm exec turbo run build --filter=./packages/react-doctor' produces dist/ in all five packages. Turbo is a root devDependency and gets installed even with the filtered install scope, so no install-step change required. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com> * style: drop em-dashes from new action source and README additions Replace em-dashes (—) with plain ASCII alternatives (':', '(' ')', or '-') in the new action.yml, actions/review/src/index.ts, actions/review/src/pipeline.ts, and the two README lines added for the PR Review / inspect-action documentation. Pre-existing em-dashes elsewhere in the README are not touched — they live in sections this PR does not modify. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com> * fix(action): drop unsafe +++/--- skips from diff parser Bugbot caught a medium-severity bug in parseAddedLineContents: the 'rawLine.startsWith("+++ ")' / 'rawLine.startsWith("--- ")' guards were copy-pasted from full unified-diff parsing but are wrong for GitHub's pulls.listFiles patches. GitHub's patch field begins at the first '@@ ...' hunk header and never includes the '+++ b/file' / '--- a/file' file headers. So the guards do not fire on any legitimate header, but they DO fire on legitimate added/removed lines whose payload begins with '++ ' or '-- ' (e.g. C++ '++ i', '++ x = 1', shell '-- arg'). When they fire on a '+' line, currentNewLine is not incremented, off-by-oneing every subsequent line number in the hunk and silently mis-placing inline review comments. Removed both guards; the '@@' hunk parser is the only thing that should set/reset currentNewLine. Also removed the unused '\\' (no newline at end of file marker) branch since it is a no-op and not emitted by GitHub patches anyway. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com> * fix(action): move @react-doctor/project-info to dependencies Bugbot flagged that pipeline.ts imports 'discoverReactSubprojects' from '@react-doctor/project-info' at runtime, but the manifest listed the package under devDependencies. It worked in CI only because the filtered install does not pass --prod and so dev deps get installed too. Move it to dependencies so a future --prod install does not silently break the action. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com> * refactor(action): dedupe isMissingReactProjectError into pipeline.ts Bugbot flagged that isMissingReactProjectError was identically defined in both pipeline.ts and index.ts with the same imports from react-doctor/api. Export it once from pipeline.ts and import in index.ts so the two copies cannot diverge. Also lets index.ts drop the NoReactDependencyError / PackageJsonNotFoundError / ProjectNotFoundError imports from react-doctor/api since the helper encapsulates the instanceof checks. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com> * fix(action): diff diagnostics at (line, column) granularity Bugbot caught a medium-severity bug in computeDiagnosticsDelta. The prior implementation grouped by (relativePath, rule, message) and sliced 'headOccurrences.slice(baseCount)' to pick the 'new' occurrences. Linters emit diagnostics in line order, so slicing the tail systematically picks the highest-line occurrences as new. When a genuinely new violation appeared at a *lower* line than an existing one with the same key, the old (higher-line) occurrence was labelled new instead, then buildInlineCommentCandidates rejected it (its line was not on a '+' line in the diff) and the inline comment was silently dropped. Diff at the (line, column) sub-key. For each (relativePath, rule, message) key we now build a multiset of base positions, walk the head occurrences, and only flag head positions not present in base as new. Same path for fixed (base positions not present in head). Line-shifted occurrences naturally fall out as one 'fixed' + one 'new' but the +/- line filter in buildInlineCommentCandidates discards the shifted-context noise. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com> * chore(action): drop unused SCORE_GAUGE_DOMAIN / GITHUB_ACTIONS_BOT_LOGIN Bugbot flagged these as dead exports — they were not referenced from anywhere in the codebase. Removing them per the AGENTS.md 'Remove unused code' rule. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com> * fix(action): tolerate spaces in thread-key file paths Bugbot flagged that THREAD_KEY_HEADER_PATTERN matched the key as '([^\s]+)' but buildThreadKey concatenates the raw relativePath. A file path containing a space (say, 'src/My Component.tsx') would yield a thread key like 'src/My Component.tsx:5|rule-name' that the regex truncated to 'src/My' at extraction time. The truncated key never matches any candidate so the next run silently resolves the thread as 'addressed' instead of leaving it active. Loosen to '([^\n]+?)' (non-greedy, single-line). Equivalent for the 99% case (paths without whitespace) and correct for the path-with- spaces case. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com> * refactor(action): single inline marker for ownership + thread key Bugbot flagged duplicated marker logic: formatInlineCommentBody prepended a plain <!-- react-doctor-review-inline --> line, then buildInlineCommentBodyWithKey prepended a separate keyed <!-- react-doctor-review-inline:KEY --> line, and isOwnedReviewThread relied on the plain marker. If the plain prefix ever drifted, thread reconciliation would silently fail because owned threads would no longer match. Collapse to one marker source. INLINE_COMMENT_MARKER becomes INLINE_COMMENT_MARKER_PREFIX (with the trailing ':'); the keyed body builder builds the marker from that constant; ownership detection substring-matches on the same prefix; the threadKey regex extracts everything between the ':' and ' -->'. formatInlineCommentBody no longer needs to seed the body with a marker because the keyed prefix is always prepended at post time. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com> * fix(action): null head repo + don't conflate AmbiguousProjectError with skip (bugbot) Two Bugbot findings on the rebased PR: ### 1. Null head repo crashes context \`pullRequest.head.repo\` is nullable (deleted forks, restricted fork metadata). Earlier code did \`pullRequest.head.repo?.full_name.split("/")[1]\` — \`?.\` only guards \`repo\`, so \`full_name\` would be undefined and \`.split\` crashes before the review runs. The same null case also set \`headRepoFullName\` to the base repo name, so \`isFork\` became \`false\` and fork-specific compare / permission behavior was wrong. Funnel every field through a single \`headRepoMeta\` guard, and make \`isFork\` only true when \`head.repo\` is present AND its \`full_name\` differs from the base. ### 2. ReactDoctorError conflated with skip \`runDiagnoseAcrossWorkspace\` and the orchestrator's catch block were collapsing any \`isReactDoctorError(error)\` to "not a React project" / "skip this workspace entry". \`AmbiguousProjectError\` is also a \`ReactDoctorError\` but means "multiple React roots were found here" — silently treating it as "skip" hides a real misconfiguration and contradicts the PR's stated handling for \`NoReactDependencyError\` / \`PackageJsonNotFoundError\` / \`ProjectNotFoundError\` only. Dropped \`|| isReactDoctorError(error)\` from both catch blocks so only \`isMissingReactProjectError\` suppresses; ambiguous / invalid-config / scan-failure ReactDoctorError instances now propagate as hard errors. Cleaned up the now-unused \`isReactDoctorError\` import in \`index.ts\` and \`pipeline.ts\`. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(action): scope base worktree scan to same INPUT_DIRECTORY as head (bugbot) Bugbot caught: when the action runs against a non-\`.\` \`directory\` input (the monorepo / per-package scoping case), head analysis correctly scoped to that subdirectory (\`resolveHeadDirectory\` resolved \`workspace\` + \`INPUT_DIRECTORY\`) but base analysis ran against the full worktree root. Diagnostic paths and counts then disagreed across the two snapshots, so regressions, sticky summaries, and inline comments on added lines could be wrong or missing. Extracted \`resolveInputDirectory()\` to centralize the \`INPUT_DIRECTORY || "."\` lookup, and added \`resolveBaseScanDirectory(worktreeDirectory)\` that joins \`INPUT_DIRECTORY\` onto the worktree the same way \`resolveHeadDirectory\` joins it onto \`GITHUB_WORKSPACE\`. The base \`runDiagnoseAcrossWorkspace\` call now sees the same subtree as the head call. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(action): anchor diagnostic paths to workspace root so PR matching works under INPUT_DIRECTORY (bugbot) The previous fix corrected the scan directory for the base worktree but exposed a path-key mismatch: \`pulls.listFiles\` returns changed-file keys relative to the **repository root** (e.g. \`packages/my-app/src/App.tsx\`), but \`runDiagnoseAcrossWorkspace\` was emitting \`relativePath\`s relative to the SCAN root (e.g. \`src/App.tsx\` when the scan was scoped to \`packages/my-app\`). \`buildInlineCommentCandidates\`'s \`changedFilesByPath.get(diagnostic.relativePath)\` then always missed for monorepo / scoped-directory configs, so no inline comments got posted and \`reconcileInlineThreads\` saw an empty active set — silently resolving real regressions while the sticky summary still listed them. \`runDiagnoseAcrossWorkspace(rootDirectory, pathBaseDirectory?)\` now accepts a second arg that overrides the directory the output \`relativePath\`s are computed against. Defaults to \`rootDirectory\` for the standalone-CLI case; the action passes \`resolveWorkspaceRoot()\` for HEAD and the worktree root for BASE, so emitted paths line up with the GitHub API's repo-root keys regardless of \`INPUT_DIRECTORY\` scoping. Renamed \`resolveHeadDirectory\` → uses new \`resolveWorkspaceRoot()\` helper for DRY. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(action): keep threads alive for regressions that can't be posted inline (bugbot) \`reconcileInlineThreads\` was being passed only the threadKeys that PASSED the inline-comment filter (errors with non-null \`patch\` on an added line). A real regression that survives the head-vs-base diff but can't be inline-posted — e.g. \`pulls.listFiles\` returned a null \`patch\` (binary / rename-only / large file), the violation line landed on a context line within the hunk instead of a \`+\` line, or the file was modified outside the diff window — would be omitted from \`activeThreadKeys\`. The existing review thread then got the "Addressed" footer and resolved on the next push, even though the regression is unchanged and the sticky summary still lists it. Build \`activeThreadKeysFromRegressions\` from every error-severity entry in \`newDiagnostics\` (using the same \`buildThreadKey\` shape as \`buildInlineCommentCandidates\`), then union with the inline-candidate keys. The set passed to \`reconcileInlineThreads\` now reflects "still a regression" rather than "still inline-postable", so threads stay alive while the underlying violation persists. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(action): scan PR head SHA, not merge commit; prune stale worktrees (bugbot) Two bugbot findings: ### 1. Scans merge commit not head (high severity) \`actions/checkout@v5\` on a \`pull_request\` event checks out the synthetic merge commit (\`refs/pull/N/merge\`) by default, not the PR head. So workspace HEAD is the merge result while inline reviews + the check run annotate \`pull_request.head.sha\` — diagnostic line numbers can disagree with what GitHub renders in the PR diff. Fixes: - README example workflow + repo fixture workflow now pass \`ref: \${{ github.event.pull_request.head.sha }}\` to \`actions/checkout\` so HEAD matches the SHA we annotate. - Action source verifies workspace HEAD == \`pull_request.head.sha\` at startup and logs a loud \`::warning::\` with the exact \`ref: ...\` snippet to add when they disagree. The action still runs (so existing workflows don't break overnight), but the user sees the actionable diagnostic in the log. ### 2. Stale worktree breaks reruns (medium severity) \`materializeBaseWorktree\` was \`fs.rmSync\`-ing the worktree directory before \`git worktree add\`. If a prior run left the worktree registered in \`.git/worktrees/\` (interrupted job, cleanup failed), \`worktree add\` rejects with "already registered" even though the directory is gone. Added \`tryRunGit(["worktree", "prune"], headDirectory)\` between the \`rmSync\` and the \`add\` — drops stale registrations defensively. \`tryRunGit\` because a fresh repo has nothing to prune and we don't want to abort on success. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(action): include message-hash in thread key so same-rule same-line distinct messages stay distinct (bugbot) \`buildThreadKey\` was \`(relativePath, line, rule)\` but the regression-diffing identity in \`computeDiagnosticsDelta\` is \`(relativePath, rule, message)\`. So two net-new errors with the same rule firing on one line but different messages (e.g. a multi-arg rule that complains about each invalid prop separately) would collapse to a single thread key — only one inline comment gets posted, and reconciliation can't tell the second violation ever existed when it gets fixed. Added a compact FNV-1a 32-bit hash of the message (base36-encoded so the key stays short, ~7 chars) and appended it to the thread key as a fourth segment. The hash is deterministic across runs so reconciliation continues to match the same thread on later pushes; multiple distinct messages on the same \`(path, line, rule)\` now keep their own threads. Updated both call sites (\`buildInlineCommentCandidates\` in \`pipeline.ts\` + the active-threadKeys union in \`index.ts\`) to pass \`diagnostic.message\` through to \`buildThreadKey\`. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(action): dedupe inline candidates by threadKey + document line-shift limitation (bugbot) Two related bugbot findings: ### 1. Duplicate inline keys in review (fixed) \`buildInlineCommentCandidates\` could emit multiple candidates with the same \`threadKey\` (the same \`(path, line, rule, message)\` identity firing twice in one scan — e.g. a rule that double-fires on identical source). \`createReview\` would either duplicate-post the comment or reject the whole batch. Switched the candidate collector from an array push to a \`Map<threadKey, candidate>\` keyed by the resolved thread key. First-seen wins; subsequent duplicates are dropped before posting. ### 2. Line shift resolves threads (documented limitation) When a push moves a surviving violation to a different line, the old thread key no longer matches and the previous thread gets the "Addressed" footer + resolution even though the regression persists. This is a pre-existing limitation of the \`(path, line, rule, message)\` thread-key design — removing \`line\` would conflate distinct violations of the same rule across multiple lines, so the fix requires tracking original-line anchors per thread (server-side state), which is out of scope for an in-repo action. Added a long-form \`KNOWN LIMITATION\` comment on \`buildThreadKey\` documenting the tradeoff and what a proper fix would need so future readers don't re-discover the issue from scratch. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>