fix: replace ternary side effect with if/else and fix null Authorizat…#10931
Merged
jasonsaayman merged 4 commits intoJun 1, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 4/5
- This PR is likely safe to merge, with only a moderate-risk behavior change noted (severity 5/10, confidence 7/10).
- In
scripts/axios-build-instance.js, conditionally omittingAuthorizationmay stop explicitly clearing inherited default headers, which can lead to unintended auth headers being sent in some request paths. - The impact appears scoped to header-construction behavior rather than a broad functional break, so risk looks manageable with a focused verification pass.
- Pay close attention to
scripts/axios-build-instance.js- ensureAuthorizationis explicitly unset when required to avoid inherited default header leakage.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
fb4594a to
f42d845
Compare
jasonsaayman
previously requested changes
May 27, 2026
Member
jasonsaayman
left a comment
There was a problem hiding this comment.
Just a couple changes before this one can land.
Contributor
Author
|
Done, both changes applied. Thanks for the review! |
jasonsaayman
approved these changes
May 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two issues found in
scripts/axios-build-instance.js:Ternary used for side effects
The contributing guidelines reference the node-style-guide which discourages using ternary operators purely for side effects with no return value. Replaced with an if/else for clarity.
Null Authorization header
When GITHUB_TOKEN is undefined, the header was being set to
nullrather than being omitted entirely. Setting a header to null can result in the string "null" being sent in some environments. Fixed using conditional object spreading so the header is simply absent when no token is provided.Note:
npm run testshows 1049/1050 tests passing. The single unhandled error is a webkit browser session timeout unrelated to this change.Summary by cubic
Fixes the GitHub
axiosclient to omit theAuthorizationheader whenGITHUB_TOKENis missing and replaces a ternary side effect with a clear if/else. Prevents sending a "null" header; behavior is unchanged when a token is provided.Changes
scripts/axios-build-instance.js, replace ternary side effect with if/else for logging.AuthorizationwhenGITHUB_TOKENexists using conditional object spread.v1.x(no functional changes).Notes
/docs/to clarify the header is added only when a token is set.Written for commit b525733. Summary will update on new commits.