Skip to content

Fix GitHubReview state values to match the GitHub API - #1525

Open
Socialpranker wants to merge 1 commit into
danger:mainfrom
Socialpranker:fix/1443-review-state-values
Open

Fix GitHubReview state values to match the GitHub API#1525
Socialpranker wants to merge 1 commit into
danger:mainfrom
Socialpranker:fix/1443-review-state-values

Conversation

@Socialpranker

Copy link
Copy Markdown
Contributor

GitHubReview.state was typed as "APPROVED" | "REQUEST_CHANGES" | "COMMENT" | "PENDING", but the reviews API doesn't send REQUEST_CHANGES or COMMENT. It sends CHANGES_REQUESTED and COMMENTED, which is why the check in #1443 silently never matches:

reviews.find(review => review.state === 'REQUEST_CHANGES')

DISMISSED was missing too, so a dismissed review had no valid type either.

On the shape of the fix: @orta suggested keeping the literal array and adding | string, and checking GitHub's own OpenAPI description makes that the right call rather than just a lenient one. pull-request-review.state there is a plain type: string with CHANGES_REQUESTED as the example, with no enum, so GitHub deliberately doesn't commit to a closed set. A fully exact union would still be wrong even with the names corrected. The literals give you autocomplete, and | string stops the type from claiming a guarantee GitHub doesn't make.

One side effect worth flagging: source/danger-incoming-process-schema.json is generated by typescript-json-schema, and adding | string drops the enum from that property, leaving "type": "string". That's the honest description now, since the old enum listed two values GitHub never sends and would reject the two it does. Nothing validates against that schema at runtime; it's there for other danger-process implementations.

The regenerated danger.d.ts and schema are included. source/dsl/GitHubDSL.ts also picked up two unrelated formatting hunks (author_association indentation and a trailing space) because lint-staged runs prettier over the whole staged file on commit.

Ran the full suite: 682 passing. Note that the tests don't run on Node 25 at all on main, since buffer-equal-constant-time reads SlowBuffer.prototype which Node 25 removed, so 32 suites fail to load before any test executes. On Node 22 everything is green.

Written with Claude Code (Claude Opus 5); I reviewed and tested everything before submitting.

The type listed REQUEST_CHANGES and COMMENT, but the reviews API returns
CHANGES_REQUESTED and COMMENTED, so checks like

  reviews.find(review => review.state === 'REQUEST_CHANGES')

never matched. DISMISSED was missing entirely.

GitHub's own OpenAPI description types this field as a plain string with
no enum, so a closed union would be wrong even with the names corrected.
The literals stay for autocomplete and `| string` keeps the type honest
about values GitHub may add later.

Fixes danger#1443
@Socialpranker

Copy link
Copy Markdown
Contributor Author

The red test job here isn't from this PR. It fails in the "Make a small babel app" step while installing a fresh create-react-app:

error @testing-library/jest-dom@6.10.0: The engine "node" is incompatible with this module. Expected version ">=22". Got "20.20.2"

.github/workflows/CI.yml pins node-version: "20", and that transitive dependency now requires Node 22, so the step fails before Danger runs. #1524 hits the identical error, so it's independent of what either PR changes. Happy to bump the workflow to Node 22 in a separate PR if that's useful.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants