Skip to content

Conversation

jonrohan
Copy link
Member

@jonrohan jonrohan commented Sep 2, 2025

Part of https://github.com/github/primer/issues/5409

This moves the content type error check to before the status !== 200 check. The content type one was never getting hit because it was passing 406 as a status code and getting caught by the not 200 check.

@jonrohan jonrohan marked this pull request as ready for review September 2, 2025 22:18
@jonrohan jonrohan requested a review from a team as a code owner September 2, 2025 22:18
@Copilot Copilot AI review requested due to automatic review settings September 2, 2025 22:18
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the response handling logic in the include-fragment element to check content type before HTTP status code. Previously, content type errors were being masked by status code errors because responses with incorrect content types (like 406 status) were caught by the status check first.

  • Reordered content type validation to occur before status code validation
  • Updated test assertions to reflect the new error message precedence

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/include-fragment-element.ts Moved content type check before status code check in response handling
test/test.js Updated test assertions to expect content type error messages instead of status code errors

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@jonrohan jonrohan requested a review from manuelpuyol September 3, 2025 02:39
@jonrohan jonrohan merged commit fcf132a into main Sep 3, 2025
4 checks passed
@jonrohan jonrohan deleted the jonrohan-patch-1 branch September 3, 2025 15:47
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.

3 participants