fix: accepted footer values for submit pr review#15673
Conversation
…mpatibility The Copilot PR reviewer on github#15643 identified that submit_pull_request_review.footer is still emitted as a boolean by the Go compiler (via AddBoolPtr/getEffectiveFooter), but setFooterMode() in JavaScript only accepts strings after the PR review footer refactoring. This caused footer: false to be silently ignored, defaulting back to 'always' (the opposite of the user's intent). Fix: Add boolean normalization in setFooterMode() so that: - false maps to 'none' (disable footer) - true maps to 'always' (enable footer) This also fixes the setIncludeFooter() backward-compatibility alias, which previously would reject boolean values passed through the old API contract. Added 3 new test cases covering: - setFooterMode(false) -> 'none' - setFooterMode(true) -> 'always' - setIncludeFooter(false) backward compatibility Co-authored-by: Bill Easton <williamseaston@gmail.com>
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where boolean footer configuration values from Go's SubmitPullRequestReviewConfig.Footer (typed as *bool) were being rejected by the JavaScript setFooterMode() function, which only accepted string values ("always", "none", "if-body"). The fix adds boolean support to normalize boolean values to their corresponding string modes: false → "none" and true → "always".
Changes:
- Added boolean value normalization in
setFooterMode()to converttrue/falseto "always"/"none" with info logging - Updated JSDoc to document backward compatibility with boolean values and clarify which config sources emit booleans vs strings
- Added comprehensive tests for boolean normalization (both
trueandfalseviasetFooterModeandsetIncludeFooterbackward compatibility alias)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| actions/setup/js/pr_review_buffer.cjs | Added boolean handling in setFooterMode() to normalize true/false to "always"/"none" modes with logging, and updated documentation |
| actions/setup/js/pr_review_buffer.test.cjs | Added three test cases covering boolean normalization (false → "none", true → "always") and backward compatibility via setIncludeFooter(false) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(callArgs.body).toBe("Review body"); | ||
| expect(callArgs.body).not.toContain("test-workflow"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Consider adding a test case for setIncludeFooter(true) to ensure complete coverage of the backward compatibility alias with both boolean values. While setIncludeFooter is just an alias to setFooterMode (which already has tests for both true and false), having explicit tests for both boolean values through the backward compatibility API would make the test suite more complete and self-documenting.
| it("should handle setIncludeFooter(true) backward compatibility", async () => { | |
| buffer.addComment({ path: "test.js", line: 1, body: "comment" }); | |
| buffer.setReviewMetadata("Review body", "COMMENT"); | |
| buffer.setReviewContext({ | |
| repo: "owner/repo", | |
| repoParts: { owner: "owner", repo: "repo" }, | |
| pullRequestNumber: 42, | |
| pullRequest: { head: { sha: "abc123" } }, | |
| }); | |
| buffer.setFooterContext({ | |
| workflowName: "test-workflow", | |
| runUrl: "https://github.com/owner/repo/actions/runs/123", | |
| workflowSource: "owner/repo/workflows/test.md@v1", | |
| workflowSourceURL: "https://github.com/owner/repo/blob/main/test.md", | |
| }); | |
| // Use the backward-compatible alias with boolean (the original API contract) | |
| buffer.setIncludeFooter(true); | |
| mockGithub.rest.pulls.createReview.mockResolvedValue({ | |
| data: { | |
| id: 509, | |
| html_url: "https://github.com/owner/repo/pull/42#pullrequestreview-509", | |
| }, | |
| }); | |
| const result = await buffer.submitReview(); | |
| expect(result.success).toBe(true); | |
| const callArgs = mockGithub.rest.pulls.createReview.mock.calls[0][0]; | |
| // Footer SHOULD be included because true maps to "always" | |
| expect(callArgs.body).toContain("test-workflow"); | |
| }); |
Fixes #15671