GitHub link oneboxer for chat

Question

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        *://chat.meta.stackexchange.com/*
// @match        *://chat.stackexchange.com/*
// @require      http://timeago.yarp.com/jquery.timeago.js
// @grant        none
// ==/UserScript==
$('head').append('<link rel="stylesheet" type="text/css" href="https://cdn.rawgit.com/shu8/SEChat-githubOneboxer/master/style.css">'); //add stylesheet to head (CSS from https://meta.stackexchange.com/q/243259/260841)

function replaceVars(url, details) { //Replace the {placeholders} with their actual values from the details array
    return url.replace(/{username}/, details.username).replace(/{repo_name}/, details.repo_name).replace(/{issue_number}/, details.issue_number).replace(/{pull_number}/, details.pull_number);
}

function useApi(url, details, callback) { //use the Github API to get the info
    $.ajax({
        url: replaceVars(url, details), //the URL should be the replaced version
        headers: {
            "Accept": "application/vnd.github.VERSION.html", //to get HTML versions of body, and not markdown
            "User-Agent": "shu8" //https://developer.github.com/v3/#user-agent-required
        },
        success: function(data) {
            callback(data);
        }
    });
}

function extractFromUrlAndGetInfo(link, $obj) { //to avoid repetition (for existing/new messages), a function to call getInfo with correct parameters 
    var info = link.split('github.com')[1];
    var username = info.split('/')[1],
        repo_name = info.split('/')[2],
        issue_number, pull_number;

    if (link.indexOf('issues') > -1) { //for issues
        issue_number = info.split('/')[4];
        getInfo('issue', { //get issue info via the API
            username: username, //with the variables
            repo_name: repo_name,
            issue_number: issue_number
        }, $obj); //pass on the jQuery element we need to onebox
    } else if (link.indexOf('pull') > -1) { //for pull requests
        pull_number = info.split('/')[4];
        getInfo('pull', { //get pull info via the API
            username: username, //with the variables
            repo_name: repo_name,
            pull_number: pull_number
        }, $obj); //pass on the jQuery element we need to onebox
    } else {
        getInfo('repo', { //get repo info via the API
            username: username, //with the variables
            repo_name: repo_name
        }, $obj); //pass on the jQuery element we need to onebox
    }
}

function getInfo(type, details, $element) {
    switch (type) {
        case 'issue': //for issues
            useApi("https://api.github.com/repos/{username}/{repo_name}/issues/{issue_number}", details, function(data) { //sends URL with placeholders to useApi and a callback follows:
                var body = $(data.body_html).find('img').remove().end().text(), //remove images from the body
                    title = data.title,
                    number = data.number,
                    opener = data.user.login,
                    creationTime = data.created_at,
                    url = data.html_url,
                    comments = data.comments,
                    avatar = data.user.avatar_url,
                    assignee = (data.assignee == null ? '<span class="milestone">not yet assigned</span>' : '<span class="milestone">assigned to <a href="'+data.assignee.url+'">'+data.assignee.login+'</a></span>'), //not a milestone, but same CSS, so same class as milestone!
                    labels = (data.labels == null ? '' : data.labels),
                    milestone = (data.milestone == null ? '<span class="milestone">no milestone</span>' : '<span class="milestone">'+data.milestone.title+' milestone</span>'); //get milestones; surround with span

                var labelSpan = ''; //get labels and suround them with spans for their own colour
                if(labels!='') {
                    $.each(labels, function(i,o) {
                        labelSpan += "<span class='label' style='background-color:#"+o.color+";'>"+o.name+"</span>"; 
                    });
                }

                if (body.length > 519) { //trim the body if it's >= 520
                    body = body.substr(0, 520);
                };
                $element.find('.content').html("<div class='ob-github ob-github-main'>" + //make the onebox
                    "<img title='" + opener + "' src='" + avatar + "'>" +
                    "<a href='" + url + "' class='title'>" +
                    "<span class='title'>" + title + "</span></a>" +
                    "&nbsp;<span class='id'>#" + number + "</span>" + (labelSpan!='' ? labelSpan + milestone : milestone) + //if no labels, show milestone, else, show both
                    "<div class='ob-github-main-info'>" +
                    "<span class='author'>" + opener + "</span> opened this issue " +
                    "<time class='timeago' datetime='" + creationTime + "' is='relative-time'></time>." +
                    assignee + "<br><p>" +
                    body + "<span class='comments'>&nbsp;" + comments + " comments</span></p></div>" +
                    "</div>");
                $("time.timeago").timeago();
            });
            break;
        case 'repo': //for repos
            useApi("https://api.github.com/repos/{username}/{repo_name}", details, function(data) {
                var owner = data.owner.login,
                    name = data.name,
                    description = data.description,
                    url = data.url,
                    avatar = data.owner.avatar_url,
                    creationTime = data.created_at;
                $element.find('.content').html("<div class='ob-github ob-github-main'>" + //make the onebox
                    "<img title='" + owner + "' src='" + avatar + "'>" +
                    "<a href='" + url + "' class='title'>" +
                    "<span class='title'>" + name + "</span></a>" +
                    "<div class='ob-github-main-info'>" +
                    "<span class='author'>" + owner + "</span> made this repo " +
                    "<time class='timeago' datetime='" + creationTime + "' is='relative-time'></time><p>" +
                    "<p>" + description + "</p></div>" +
                    "</div>");
                $("time.timeago").timeago();
            });
            break;
        case 'pull': //for pull requests
            useApi("https://api.github.com/repos/{username}/{repo_name}/pulls/{pull_number}", details, function(data) {
                var title = data.title,
                    body = $(data.body_html).find('img').remove().end().text(), //remove images from the body
                    number = data.number,
                    url = data.url,
                    creator = data.user.login,
                    creationTime = data.created_at,
                    avatar = data.head.user.avatar_url,
                    comments = data.comments;

                $element.find('.content').html("<div class='ob-github ob-github-main'>" +
                    "<img title='" + creator + "' src='" + avatar + "'>" +
                    "<a href='" + url + "' class='title'>" +
                    "<span class='title'>" + title + "</span></a>" +
                    "&nbsp;<span class='id'>#" + number + "</span>" +
                    "<div class='ob-github-main-info'>" +
                    "<span class='author'>" + creator + "</span> submitted this pull request " +
                    "<time class='timeago' datetime='" + creationTime + "' is='relative-time'></time><br><p>" +
                    body + "<span class='comments'>&nbsp;" + comments + " comments</span></p></div>" +
                    "</div>");
                $("time.timeago").timeago();
            });
            break;
    }
}

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
                        }
                    }
                }
            }
        }
    });
});

setTimeout(function() {
    $('.message').each(function() { //loop through EXISTING messages to find oneboxable messages
        if ($(this).find('a[href*="github.com"]').length) { //if there is a link to github
            if($(this).text().trim().indexOf(' ') == -1) { //if there is no space (ie. nothing other than the link)
                var link = $(this).find('a[href*="github.com"]').attr('href');
                extractFromUrlAndGetInfo(link, $(this)); //pass URL and message to the function which will go on to call useApi and add the onebox
            }
        }
    });
    setTimeout(function() { //use the timeago plugin to add relative times to the onebox for EXISTING messages
        $("time.timeago").timeago();
    }, 1000);

    observer.observe($('#chat')[0], { //observe with the mutation observer for NEW messages
        childList: true,
        subtree: true
    });
}, 1000);

I've posted this on StackApps where you can install this if you like, but I will update it when I get any feedback!

A link to Atom Editor's repo, oneboxed:

If you're interested, test it here, where I did.

Answer 1

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 < 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
                        }
                    }
                }
            }
        }
    });
});

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 <a> 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 < length; i++) {
            var $addedNode = $(mutation.addedNodes[i]);

            //If it isn't a message, bail out
            if ( !$addedNode.hasClass('message') ) {
                return;
            }

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

            //If there aren't links, bail out
            if( !$lastLink.length ) {
                return;
            }

            // if there's nothing else but a link to github
            if( $addedNode.text().trim().indexOf(' ') == -1 
                && $lastLink.attr('href').indexOf('github') > -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 if, 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 < length; i++) {
            var addedNode = mutation.addedNodes[i];

            //If it isn't a message, bail out
            if ( !(' ' + addedNode.className + ' ').indexOf(' message ') ) {
                return;
            }

            var links = addedNode.querySelectorAll('a');

            //If there aren't links, bail out
            if( !links.length ) {
                return;
            }

            var last = links[links.length - 1]; //Last link

            // If there's nothing else but a link to github
            // More info on http://stackoverflow.com/a/9488834
            if( addedNode[textProp].trim().indexOf(' ') == -1 
                    && (last.hostname == 'github.com' || last.hostname == 'www.github.com') ) {

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

_{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 Firefox from deleting your comments, and still allows to parse them as part of the userscript.

---

setTimeout: (at the very end of your code)

I must say that the stuff here is a lot more readable! Thank you!

But there's still some things:

• Please, think about performance!!! Please!!!

  Not everybody has an Intel Xeon 14-core 2.6GHz CPU! Sometimes you have a little phone with a single core. You have to love performance! Marry performance!

  You have this:

  $('.message').each(function() { //loop through EXISTING messages to find oneboxable messages
  

  On my Memer project (at the time it was just a chat "translator") I do this:

  $('#chat .message:not([data-checked="1"])').foreach( [...] );
  

  Now you say: "But you said not to use pseudo-selectors!". To which I say: Correct, but this one will still be optimized by the browser because it runs on document.querySelectorAll. At least in the browsers we care about, like Chrome and Firefox. You can remove the #chat bit there if your tests show increase in speed.

  This avoids you processing the same messages over and over again.

  In fact, what I have on memer is this:

  $('#chat .message:not([data-checked="1"])')
      .attr('data-checked', 1)
      .find('>.content')
      .filter(':not(:has(.onebox))')
  

  Which goes even further to check if it isn't a oneboxed thing already. Yes, it is slower than your code, but in this case we will cheat on performance in exchange of correctness and readability. Slow and steady on this one!

• Don't mix APIs!

  A few lines below, almost at the end, you have this:

  observer.observe($('#chat')[0], { //observe with the mutation observer for NEW messages
      childList: true,
      subtree: true
  });
  

  Sure, jQuery will optimize $('#chat') into document.getElementById('chat') but it still has quite an high cost. This is pure laziness. Just write this:

  //observe with the mutation observer for NEW messages
  observer.observe(document.getElementById('chat'), {
      childList: true,
      subtree: true
  });
  

  In fact, I would do even better:

  var chatElem = document.getElementById('chat');
  setTimeout(function() {
  
      [...]
  
      //observe with the mutation observer for NEW messages
      observer.observe(chatElem, {
          childList: true,
          subtree: true
      });
  });
  

  The #chat won't go away! You can store it there and you are done! In fact, you can ever speed a bit our last point:

  $('.message:not([data-checked="1"])', chatElem)
      .attr('data-checked', 1)
      .find('>.content')
      .filter(':not(:has(.onebox))')
  

  Oh yeah! We are back on performance!

• I REPEAT: YOU HAVE TOO MUCH JQUERY!!!

  I will not re-iterate over this if. I will just de-jQuerylize(?) that whole .each():

    [...].each(function() {
        var links = this.querySelectorAll('a[href*="github.com"]');

        //Bail out if there are no links, could replace `> -1` with `~`
        if (!links.length || this[textProp].trim().indexOf(' ') > -1) {
            return;
        }

        //pass URL and message to the function which will go on to call useApi and add the onebox
        extractFromUrlAndGetInfo(links[links.length - 1].href, $(this));
    });

Answer 2

Stacking .replaces is bad practice; use an object instead.

return url.replace(/{username}/, details.username).replace(/{repo_name}/, details.repo_name).replace(/{issue_number}/, details.issue_number).replace(/{pull_number}/, details.pull_number);

into:

var itemsToReplace = {
              /{username}/: details.username,
              /{repo_name}/: details.repo_name,
              /{issue_number}/: details.issue_number,
              /{pull_number}/: details.pull_number
          };

for (var item in itemsToReplace){
    if (typeof itemsToReplace[item] !== 'function') {
         url = url.replace(item, itemsToReplace[item]);
    }
}
return url;

---

The HTML building seems like it should be improved, but, I can't think of anything much...

Although, you can put the ending +s of each line at the beginning of the next line, to increase readability.

The same thing can apply to commas in objects/arrays, which improves readability, and also being able to comment out lines easier, in case you want to temporarily enable or disable a feature.

---

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

Your code is mostly readable, but, the massively nested block and the HTML building are a bit of an eyesore.

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?

Hmm... Perhaps: string.split(' ').length > 1 instead?

Answer 3

RE the replace, here's something I came up with which has no destructive assignment or side effects. I might be able to go into more depth later on with regards to the rest of your code, but right now I'm at work.

function replaceUrl(originalUrl, details) {
  var itemsToReplace = [
    { test: /{username}/, with: details.username },
    { test: /{repo_name}/, with: details.repo_name },
    { test: /{issue_number}/, with: details.issue_number },
    { test: /{pull_number}/, with: details.pull_number },
  ];
  var url = itemsToReplace.reduce(function(previous, current) {
    return previous.replace(current.test, current.with);
  }, originalUrl);
}

You'll need to set the original url to originalUrl, of course. This doesn't use side effects and treats originalUrl as if it's immutable, which is useful if you're using ES6 because then you can declare it as const as opposed to let/var