patternjavascriptMinor

GitHub link oneboxer for chat

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

chatoneboxergithubforlink

Problem

I've made a userscript that oneboxes links to GitHub repos, issues or pull requests in Chat after seeing the request One-box repositories, issue tickets and such on GitHub in the chat on Meta.

It uses the GitHub API to get any relevant information and displaying it in a onebox-like manner. It uses MutationObserver to check for new messages and looks for GitHub links in those messages.

It contains a few functions:

replaceVars(url, details) - replaces placeholders in url (in the form of {placeholder name}) with their real values from details

useApi(url, details, callback) - sends the GET request to Github to get information from the API. url is the URL the GET request is sent to, and callback is... the callback

extractFromUrlAndGetInfo(link, $obj) - gets the necessary details from the URL someone posted in chat (link is the URL they posted). $obj is the jQuery object which is the actual message (this will be repalced with the oneboxed form of the message)

getInfo(type, details, $element) - uses useApi() to get the details from the API, and depending on the type (repo/issue/pull), uses replaceVars() to replace different placeholders. The details for the replacement are in details and $element is the jQuery object which is the actual message.

My main questions are:

Is my code readable easily? If not, how can I improve it?

Is there a cleaner way to do something (anything!) in the script? For example, to check whether there is another word, I've used indexOf(' ') - is there a more preferable way to check for multiple words?

Can the code be cleaned up anymore?

```
// ==UserScript==
// @name SE Chat Github Oneboxer
// @namespace http://stackexchange.com/users/4337810/
// @version 1.1.1
// @description Oneboxes links to Github repos, issues, or pull requests in Chat
// @author ᔕᖺᘎᕊ (http://stackexchange.com/users/4337810/)
// @match ://chat.stackoverfow.com/
// @match *

Solution

The code in general is a little difficult to read. I really had an hard time. But I will be randomly jumping around on your code and reviewing it. More will be added with time.

MutationObserver:

This one is the hell on Earth to read!

Look at the nesting:

var observer = new MutationObserver(function(mutations) { //MutationObserver
    mutations.forEach(function(mutation) {
        var i;
        for (i = 0; i  -1) { //if the link is to github
                            var link = $addedNode.find('a:last').attr('href'); //get the link
                            extractFromUrlAndGetInfo(link, $addedNode); //pass URL and added node to the function which will go on to call useApi and add the onebox
                        }
                    }
                }
            }
        }
    });
});

I must shake your hand: it took me 10 minutes to understand what the heck was going on!

A few points I will be making about it:

-
You should always declare a variable to store the length.

In this particular case, it may make quite a big difference.

Compare this:

var i;
for (i = 0; i < mutation.addedNodes.length; i++) {

To this:

var length =  mutation.addedNodes.length;
for (var i = 0; i < length; i++) {

This will speed up your code, since it doesn't have to go to a property to go to an array to get the length. The deeper the value is, the slower it will be. Since you only do this slow operation once, you will see good improvements.

-
Repeated selectors.

Selectors are a little slow. Specially this:

$addedNode.find('a:last')

That :last is a kick on your performance. I will describe what happens everytime you run it:

jQuery passes the selector to Sizzle

Sizzle tries to detect if document.querySelectorAll exists

It then runs document.querySelectorAll('a:last') inside a try ... catch

A SyntaxError is thrown

The syntax error is caught

Sizzle runs a regex to extract a

Fetches all the ` elements inside that element

Sizzle runs the same regex to extract :last




Sizzle checks if it is a pseudo-selector



Executes the magic behind it



Extracts the element



Passes the element to jQuery



Quite a lot of stuff going on, right? And this is repeated twice per element. You should do like this:

$lastLink = $addedNode.find('a').last();


You have no idea how fast that is compared to that whole bloat.

To describe what happens, here it is:

jQuery passes the selector to Sizzle

Sizzle tries to detect if document.querySelectorAll exists

It then runs document.querySelectorAll('a') inside a try ... catch



This selector will be optimized by the CSS engine, and will be way faster than

document.getElementsByTagName('a'), which might happen with the selector a:last



Passes the elements to jQuery



jQuery extracts the last element and gives it to you



See how much processing we have cut down?

- 
Use early returns

You currently do this:

$addedNode.hasClass('message')


This is the first condition. You can bail out if it isn't a

.message

.

- 
You have so much nesting going on!

It looks like a

Nest-Fest

!

You have 7 nestes stuffs! Including loops, functions and conditional blocks. That's a lot of nesting. We must reduce it!

- 
No need to check the length of matched elements. jQuery does nothing if there are

This is what I came up with your function:

var observer = new MutationObserver(function(mutations) { //MutationObserver
    mutations.forEach(function(mutation) {
        for (var i = 0, length = mutation.addedNodes.length; i  -1 ) {

                //pass URL and added node to the function which will go on to call useApi and add the onebox
                extractFromUrlAndGetInfo($lastLink.attr('href'), $addedNode);
            }
        }
    });
});


I am not particularly proud about that

, but it is the best I can do.

BUT WAIT, THERE'S MORE!!!

You though I was done beating this poor function? Well, I'm not! As my last point:

TOO MUCH JQUERY!



Yeah, you don't need jQuery for this! Watch this:

var observer = new MutationObserver(function(mutations) { //MutationObserver
var undefined; //Required to avoid exterior changes

//Required for Firefox compatibility
var textProp = addedNode.innerText == undefined ? 'textContent' : 'innerText' ;

mutations.forEach(function(mutation) {
for (var i = 0, length = mutation.addedNodes.length; i

As an alternative to that check for the hostname, you can use last.hostname.match(/^(?:www\.)github\.com$/i) instead.

Oh my! The performance will be MASSIVELLY IMPROVED on that handler. Event handlers should do what they have to do quickly and GTFO as soon as possible. Yours is an old lady doing 40KM/H on a highway, which is illegal.

On another answer by @rolfl, he suggests to wrap the comments in a /** @preserve */ block, like this:

/** @preserve
// ==UserScript==
.....
// ==/UserScript==
*/

This will prevent Firefo

Code Snippets

var observer = new MutationObserver(function(mutations) { //MutationObserver
    mutations.forEach(function(mutation) {
        var i;
        for (i = 0; i < mutation.addedNodes.length; i++) {
            var $addedNode = $(mutation.addedNodes[i]);
            if ($addedNode.hasClass('message')) { //if the new node is a message
                if ($addedNode.find('a').length) { //if there is a link in the message
                    if($addedNode.text().trim().indexOf(' ') == -1) { //if there is no space (ie. nothing other than the link)
                        if ($addedNode.find('a:last').attr('href').indexOf('github') > -1) { //if the link is to github
                            var link = $addedNode.find('a:last').attr('href'); //get the link
                            extractFromUrlAndGetInfo(link, $addedNode); //pass URL and added node to the function which will go on to call useApi and add the onebox
                        }
                    }
                }
            }
        }
    });
});

var i;
for (i = 0; i < mutation.addedNodes.length; i++) {

var length =  mutation.addedNodes.length;
for (var i = 0; i < length; i++) {

$addedNode.find('a:last')

$lastLink = $addedNode.find('a').last();

Context

StackExchange Code Review Q#100940, answer score: 8

Revisions (0)

No revisions yet.