Fix: Replace invalid .link() usage in checkBranchPush.js with Markdown link and improve error handling#358
Open
Manas-Dikshit wants to merge 2 commits into
Open
Fix: Replace invalid .link() usage in checkBranchPush.js with Markdown link and improve error handling#358Manas-Dikshit wants to merge 2 commits into
Manas-Dikshit wants to merge 2 commits into
Conversation
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.
The Oppiabot module checkBranchPush.js handles checks for branches that are force-pushed.
While reviewing the file, I noticed that the code was using the JavaScript string method .link() to generate an HTML link in a comment body.
Issue
The .link() method is a deprecated browser-only API (String.prototype.link) and is not supported in Node.js, which Oppiabot runs on (via Probot/Heroku).
As a result, this would throw a TypeError: 'link' is not a function during runtime, causing the force-push handler to fail silently without commenting or closing the PR.
Additionally, the function:
Used a slightly confusing variable name (isNotPrBranch)
Did not include try/catch blocks around API calls, which could cause the bot to crash if the GitHub API rate-limited or returned an error
Used a smart apostrophe (’) that can cause encoding issues in some environments
Fix Implemented
Replaced the invalid .link() call with a valid Markdown link, which renders correctly in GitHub comments:
const link = 'here (point 5)';
Renamed isNotPrBranch → isWhitelistedBranch for better readability.
Added try/catch wrappers around GitHub API calls to prevent runtime crashes.
Replaced curly apostrophe with a straight one for consistency and reliability.
Added inline comments and minor readability improvements.
Impact
Before this fix:
The bot would throw an error and fail to respond when a force push occurred on a non-whitelisted branch.
No explanatory comment would be posted to the PR, and the PR wouldn’t be closed automatically.
After this fix:
The bot correctly identifies the PR associated with a force push.
It comments politely explaining why force pushes are disallowed, with a valid link to the contributing guide.
It safely closes the PR without crashing.
Improved stability and maintainability of the Oppiabot codebase.
Testing
Verified that npm run dev runs successfully with the fix.
Simulated a force-push event on a test PR; Oppiabot commented and closed the PR as expected.
Checked Markdown rendering of the link in the PR comment on GitHub — displays correctly.
Checklist
Fixed runtime compatibility issue
Added robust error handling
Improved code readability and comments
Verified Markdown link rendering
All tests passing locally (npm test)