Stack Exchange rev​iew queue notifications

Question

A couple of days ago a user in chat asked us to spend more time in the queues. Kick starting my desire to get notifications for new items in the queues.
Reviewing the queues has been something I've wanted to do for a long time now. But I've never built a habit to check daily, and so I've wanted to use Simon Forsberg's Desktop Notifications for flags and queue items user script for a while now. The only problem is that it doesn't work for me, and given the design changes SE has recently made, it's only more broken.

Originally I planned to fix Simon's user script, however my pre-ES 2015 JavaScript skills are not great. And so I could barely understand Simon's code never mind fix the issues I was having.

To use the script:

1. Install it like you would any other user script.
2. Navigate to Code Review's review page.
3. If you haven't enabled notifications it should prompt you to allow them.
   If it does not prompt you then you can enable them by pressing the options button to the left hand side of the URL.
4. Reload the page.

I feel my code doesn't really follow YAGNI as I have implemented a lot of classes. And currently Notifications is fairly redundant. I think this is acceptable as later on when I've not read the user script for a while all the helpful names and short docstrings should point me in the right direction. Knowing what Review.title() means is, to me, far easier than understanding what foo.children[1].children[0].children[0].innerText is.

The code is also as close to pure JS as I could reasonably get. This means anyone with any compliant browser and any userscript plugin should be able to run the code. Meaning there's no accidental Greasemonkey/Tampermonkey/Violentmonkey incompatibilities.

I think I've followed JavaScript's conventions, however I've not written any in a short while.
Any and all reviews are welcome.

// ==UserScript==
// @name        Review Notifications
// @namespace   Violentmonkey Scripts
// @match       *://*.stackexchange.com/review
// @grant       none
// @version     1.0
// @author      Peilonrayz
// @description Get notifications for new review items.
// ==/UserScript==

// Enum of push states
const Status = Object.freeze({
    NOT_IMPLEMENTED: 1,
    DENIED: 2,
    GRANTED: 3,
    DEFAULT: 4,
    UNKNOWN: 5,
});

function pushStatus(status) {
    // Convert from a string into the enum's value
    if (status === undefined) {
        return Status.NOT_IMPLEMENTED;
    }
    if (status === "denied") {
        return Status.DENIED;
    }
    if (status === "granted") {
        return Status.GRANTED;
    }
    if (status === "default") {
        return Status.DEFAULT;
    }
    return Status.UNKNOWN;
}


class PushNotification {
    // A small wrapper to Notification to expose the observer interface

    static async enable() {
        // Enable notifications if possible
        //
        // This returns the current state regardless if notifications are enabled.
        // This returns if enabled and errors if it is not possible.
        // This allows simple and clean usage with await or `then`.
        let status = pushStatus(Notification.permission);
        // Prompt user to allow us to push notifications
        if (status === Status.DEFAULT) {
            status = pushStatus(await Notification.requestPermission());
        }
        if (status === Status.GRANTED) {
            return status;
        }
        throw status;
    }

    notify(notification) {
        // Push a notification to the system
        new Notification(notification.title, notification);
    }
}

class Notifications {
    // Holds a collections of observers that we can push notifications to.
    //
    // This is here as I may want to implement additional observers.
    // For example alerting if push notifications are unavailable is an option.
    // This allows easy additions without having to change more than just the 
    // creation of objects.
    constructor() {
        this.clients = [];
    }

    add(client) {
        // Add an observer to the subject
        this.clients.push(client);
    }

    notify(notification) {
        // Notify all observers
        for (const observer of this.clients) {
            observer.notify(notification);
        }
    }
}

class Review {
    // Interface to the underlying review information
    constructor(object) {
        this.object = object;
    }

    static* findAll() {
        // Get all reviews on the current page.
        for (let review of $("#content")[0].children[1].children[0].children) {
            if (!review.className.contains("grid")) {
                continue;
            }
            yield new Review(review);
        }
    }

    amount() {
        // Get the current amount of reviews.
        return +this.object.children[0].children[0].innerText;
    }

    title() {
        // Get the queue's title
        return this.object.children[1].children[0].children[0].innerText;
    }
}

function split_once(value, sep) {
    // Helper function to only split by a seperator once.
    //
    // This is not the same as `"abc def ghi".split(" ", 2)` as that would
    // result in ["abc", "def"] rather than ["abc", "def ghi"]
    let index = value.indexOf(sep);
    if (index === -1) {
        return [value, ""];
    }
    return [value.slice(0, index), value.slice(index + sep.length)]
}

class Cookies {
    // A map like interface to the cookies.
    constructor() {
        this.cookies = new Map();
        this.update();
    }

    update() {
        // Update the internal map from the cookies on the page.
        //
        // This is useful when other code on the page changes the cookies but
        // not through this object.
        this.cookies = new Map(document.cookie.split(';').map(c => split_once(c.trim(), "=")));
    }

    get(key) {
        // Get the value of the cookie by its name
        return this.cookies.get(key);
    }

    set(key, value) {
        // Set a cookie to the provided value
        this.cookies.set(key, value);
        document.cookie = key + "=" + value;
    }
}

function findReviews(notifications) {
    // Find and notify the user about new reviews.
    //
    // 1.   This is provided a fully initialized Notifications object.
    // 2.   Initialize a Cookies object to allow comparisions with the previous
    //      page load. This is important as otherwise the code would
    //      continuously notify users of all active reviews.
    // 3.   For each review on the page:
    //      1.  Verify if there are new reviews - comparing with the cookie.
    //      2.  Notify the user if there is a new review.
    //      3.  Update the cookie to the new value.
    // 4.   Reload the page.

    let cookies = new Cookies();
    for (let review of Review.findAll()) {
        let prev = cookies.get(review.title());
        let prevAmount = prev === undefined ? 0 : +prev;
        console.log(review.title(), prevAmount, "->", review.amount(), prevAmount < review.amount());
        if (prevAmount < review.amount()) {
            notifications.notify({
                "title": review.amount() + " reviews in " + review.title(),
                "icon": "https://cdn.sstatic.net/Sites/codereview/img/apple-touch-icon.png?v=0a72875519a4",
            })
        }
        cookies.set(review.title(), review.amount());
    }
    setTimeout(function(){ window.location.reload(); }, 60 * 1000);
}

function main() {
    // Build notifications and find reviews.
    const notifications = new Notifications();
    PushNotification.enable().then(
        () => {
            notifications.add(new PushNotification());
            findReviews(notifications);
        },
        (status) => console.log("Can't notify status code", status),
    );
}

main()

Answer 1

I don't see much benefit to the enum. The Notification.permission string (or the return value of requestPermission()) is already pretty clear: 'denied', 'granted', or 'default'. The specification requires that it be one of those, if window.Notification exists. Passing around a number instead of a more intuitive string seems a bit odd.

In Javascript, at least in my opinion, classes are generally useful when you want to bundle state data with methods. If you're not using instance properties, consider using plain functions (or an object of functions, if you have multiple related ones) instead; the intent of the code will be clearer, and it'll look a bit simpler.

.then(success, fail) is generally considered an antipattern for promises. Unless you deliberately want the somewhat odd control flow it results in, it would be better to use .then followed by .catch, so that your catch can catch errors that may occur in the .then.

But exceptions should be exceptional. They require unwinding the whole call stack and, when you're working with a self-contained script, generally don't provide any control flow benefits. Rather than throwing, it might be preferable to just check the permission result, and if it's not 'granted', log an error and exit.

const canNotify = async () => {
    if (Notification.permission === 'default') {
        await Notification.requestPermission()
    }
    const { permission } = Notification;
    if (permission !== 'granted') {
        console.error(`Notifications not permitted. Permission status: ${permission}`);
        return;
    }
    return true;
};
const makeNotification = (notification) => {
    new Notification(notification.title, notification);
};
async function main() {
    if (!canNotify()) {
        return;
    }
    const notifications = new Notifications();
    notifications.add(makeNotification);
    findReviews(notifications);
}

main()

I'm unsure about the usefulness of the Notifications class. It makes sense if you're expecting to add multiple separate observers, but it sounds like you may only want to notify the user once, using the most preferred method available (whether that's a Notification, an alert, a SpeechSynthesisUtterance or sound, etc). In such a case, rather than having a Notifications and an array of observers, consider making a function that looks at permissions/userscript settings and returns a function that calls the right method.

const makeNotifier = async () => {
  if (canNotify()) {
    return notification => new Notification(notification.title, notification);
  } else if (preferAlerts) {
    return notification => alert(notification.title);
  }
  // etc
};
const notifier = await makeNotifier();
// pass around notifier

Cookies are meant for saving settings associated with a user which get sent to the server with every request. Here, since you're just trying to persist data across pageloads, it would be more appropriate to use localStorage, which doesn't get sent to the server, is much easier to interface with, and has a much larger storage limit.

Remember to always use const whenever possible. When you use let, you're sending the message to other readers of the code: "I may reassign this variable in the future, so watch out, don't take its current value for granted!" Code is generally easier to read when you don't have to worry about reassignment.

Rather than calling review.title() and review.amount() multiple times, you can store their values into variables (which can make things clearer when you want to distinguish the current values from the previous values).

function findReviews(notifications) {
    // Find and notify the user about new reviews.
    //
    // 1.   This is provided a fully initialized Notifications object.
    // 2.   Take data from localStorage to allow comparisions with the previous
    //      page load. This is important as otherwise the code would
    //      continuously notify users of all active reviews.
    // 3.   For each review on the page:
    //      1.  Verify if there are new reviews - comparing with the stored value.
    //      2.  Notify the user if there is a new review.
    //      3.  Update the cookie to the new value.
    // 4.   Save the new review counts in localStorage
    // 5.   Reload the page.
    const storedReviewCounts = JSON.parse(localStorage.reviewNotifications || '{}');
    for (const review of Review.findAll()) {
        const prevAmount = storedReviewCounts[review.title()] || 0;
        const reviewQueueName = review.title();
        const currentAmount = review.amount();
        console.log(reviewQueueName, prevAmount, "->", currentAmount, prevAmount < currentAmount);
        if (prevAmount < currentAmount) {
            notifications.notify({
                "title": currentAmount + " reviews in " + reviewQueueName,
                "icon": "https://cdn.sstatic.net/Sites/codereview/img/apple-touch-icon.png?v=0a72875519a4",
            })
        }
        storedReviewCounts[reviewQueueName] = currentAmount;
    }
    localStorage.reviewNotifications = JSON.stringify(storedReviewCounts);
    setTimeout(function () { window.location.reload(); }, 60 * 1000);
}

In your Review class, rather than using hard-to-read chained .children, you can use querySelector to select the right descendant - or, use a selector to select a child, then navigate upwards to an ancestor with .closest. Look at the elements you want to target in your browser tools, and figure out a CSS selector which can target it. Using the .closest route, you can get to an anchor like <a href="/review/close">Close Votes</a> easily, at which point it's probably easier to just extract the information directly and put it into an object than to navigate to a parent object and later search through it to find the child again.

The selector

#content .fs-subheading [href^="/review/"]

will select elements:

• inside the element with the content
• inside an element with a class name of fs-subheading
• which have an href attribute which starts with /review/

From here, you can get the queue name. Then navigate to the whole right cell container, so you can get to the left cell, so you can get to the review count inside the left cell.

function getReviews() {
    return [...$('#content .fs-subheading [href^="/review/"]')].map((reviewAnchor) => {
        const reviewQueueName = reviewAnchor.textContent;
        const rightCell = reviewAnchor.closest('.grid');
        const leftCell = rightCell.previousElementSibling;
        const count = Number(leftCell.querySelector('[title]').title.replace(/,/g, ''));
        return { reviewQueueName, count };
    });
}

This'll return an array of objects with a reviewQueueName and count properties.

It's probably not an issue for most users now that the bug has been fixed, but I habitually put window before setTimeout to avoid a bug certain versions of Chrome had when running userscripts with setTimeout.

Putting all these ideas together, and you get:

throw new Error('Using Stack Snippet to hide large amount of repeated code. This is not runnable.');

function getReviews() {
    return [...$('#content .fs-subheading [href^="/review/"]')].map((reviewAnchor) => {
        const reviewQueueName = reviewAnchor.textContent;
        const rightCell = reviewAnchor.closest('.grid');
        const leftCell = rightCell.previousElementSibling;
        const count = Number(leftCell.querySelector('[title]').title.replace(/,/g, ''));
        return { reviewQueueName, count };
    });
}

function notifyOnNewReviews(notifier) {
    // Find and notify the user about new reviews.
    //
    // 1.   This is provided a notifier function.
    // 2.   Take data from localStorage to allow comparisions with the previous
    //      page load. This is important as otherwise the code would
    //      continuously notify users of all active reviews.
    // 3.   For each review on the page:
    //      1.  Verify if there are new reviews - comparing with the stored value.
    //      2.  Notify the user if there is a new review.
    //      3.  Update the localStorage object to the new value.
    // 4.   Save the new review counts in localStorage
    // 5.   Reload the page.
    const storedReviewCounts = JSON.parse(localStorage.reviewNotifications || '{}');
    for (const review of getReviews()) {
        const { reviewQueueName, count: currentAmount } = review;
        const prevAmount = storedReviewCounts[reviewQueueName] || 0;
        console.log(reviewQueueName, prevAmount, "->", currentAmount, prevAmount < currentAmount);
        if (prevAmount < currentAmount) {
            notifier({
                title: currentAmount + " reviews in " + reviewQueueName,
                icon: "https://cdn.sstatic.net/Sites/codereview/img/apple-touch-icon.png?v=0a72875519a4",
            })
        }
        storedReviewCounts[reviewQueueName] = currentAmount;
    }
    localStorage.reviewNotifications = JSON.stringify(storedReviewCounts);
    window.setTimeout(function () { window.location.reload(); }, 60 * 1000);
}

async function canNotify() {
    if (Notification.permission === 'default') {
        await Notification.requestPermission();
    }
    const { permission } = Notification;
    if (permission !== 'granted') {
        console.error(`Notifications not permitted. Permission status: ${permission}`);
        return;
    }
    return true;
}

async function makeNotifier () {
    const preferAlerts = true; // or whatever logic you want
    if (await canNotify()) {
        return notification => new Notification(notification.title, notification);
    } else if (preferAlerts) {
        return notification => alert(notification.title);
    }
    // etc
}

async function main() {
    const notifier = await makeNotifier();
    notifyOnNewReviews(notifier);
}
main();

Works on Stack Overflow. It probably works here as well, but since I can't review yet, I'm not 100% sure.

Answer 2

Usability

I ran the script on the SO review dashboard to see how it would perform with various numbers. The biggest thing I noticed was that it didn't properly handle numbers greater than 999 since those are formatted in x.yk format. To properly handle those you may have to look for such a format and strip off any multipliers.

Review

In addition to the points already addressed by CertainPerformance, I noticed a couple other things that could be simplfied:

setTimeout(function(){ window.location.reload(); }, 60 * 1000);

There isn't any need to wrap the reload call in an anonymous/lambda function/closure, since it is a function:

setTimeout(window.location.reload, 60 * 1000);

If you had a need to set the this context for such a call, then use Function.bind()

---

When using an arrow function expression with a single parameter - e.g.

(status) => console.log("Can't notify status code", status),

The parameters don't need to be wrapped in parentheses:

status => console.log("Can't notify status code", status),

---

It is wise to use const for all variables until you determine that re-assignment is necessary- then use let. This helps avoid accidental re-assignment.

For example, in split_once() there is an assignment for index:

let index = value.indexOf(sep);

But that value never gets re-assigned within the function.

There are also three variables in findReviews() that don't appear to be re-assigned that can be declared with const - i.e. cookies, prev and prevAmount.