EandrewJones commented on code in PR #410:
URL: https://github.com/apache/flagon-useralejs/pull/410#discussion_r1482242294
##########
build/UserALEWebExtension/background.js:
##########
@@ -1131,28 +1155,48 @@ var defaultConfig = {
authHeader: null,
toolName: 'useralePlugin',
version: version
+ },
+ pluginConfig: {
+ // Default to a regex that will match no string
+ urlWhitelist: '(?!x)x'
}
};
-browser.storage.local.get(defaultConfig, function (res) {
- options(res.useraleConfig);
-});
+var urlWhitelist;
+function updateConfig(config) {
+ urlWhitelist = new RegExp(config.pluginConfig.urlWhitelist);
+ options(config.useraleConfig);
+ dispatchTabMessage(config.useraleConfig);
+}
function dispatchTabMessage(message) {
browser.tabs.query({}, function (tabs) {
tabs.forEach(function (tab) {
browser.tabs.sendMessage(tab.id, message);
});
});
}
+function filterUrl(log) {
+ if (urlWhitelist.test(log.pageUrl)) {
+ return log;
+ }
+ return false;
+}
+browser.storage.local.get(defaultConfig, function (res) {
+ addCallbacks({
Review Comment:
Again please add brief explanation of why we need add the filterUrl as a
callback since in the lines right below we apply the function directly in the
browser event handler.
##########
build/UserALEWebExtension/background.js:
##########
@@ -428,6 +428,30 @@ var intervalCounter;
var intervalLog;
var cbHandlers = {};
+/**
+ * Adds named callbacks to be executed when logging.
+ * @param {Object } newCallbacks An object containing named callback
functions.
+ */
+function addCallbacks() {
Review Comment:
Please add some comments to this function to help describe what's happening
here at a business logic level.
##########
build/UserALEWebExtension/options.js:
##########
@@ -1142,13 +1142,17 @@ function setConfig() {
if (config.userId && password) {
config.authHeader = "Basic " + btoa("".concat(config.userId,
":").concat(password));
}
- browser.storage.local.set({
- useraleConfig: config
- }, function () {
+ var payload = {
+ useraleConfig: config,
+ pluginConfig: {
+ urlWhitelist: document.getElementById("filter").value
+ }
+ };
+ browser.storage.local.set(payload, function () {
options(config);
browser.runtime.sendMessage({
type: CONFIG_CHANGE,
- payload: config
+ payload: payload
Review Comment:
FYI: JS will automatically infer the key if your value of the same name.
So `{type: CONFIG_CHANGE, payload }` is perfectly valid. Saves you a few
characters of typing.
##########
build/UserALEWebExtension/background.js:
##########
@@ -1131,28 +1155,48 @@ var defaultConfig = {
authHeader: null,
toolName: 'useralePlugin',
version: version
+ },
+ pluginConfig: {
+ // Default to a regex that will match no string
+ urlWhitelist: '(?!x)x'
}
};
-browser.storage.local.get(defaultConfig, function (res) {
- options(res.useraleConfig);
-});
+var urlWhitelist;
+function updateConfig(config) {
+ urlWhitelist = new RegExp(config.pluginConfig.urlWhitelist);
+ options(config.useraleConfig);
+ dispatchTabMessage(config.useraleConfig);
+}
function dispatchTabMessage(message) {
browser.tabs.query({}, function (tabs) {
tabs.forEach(function (tab) {
browser.tabs.sendMessage(tab.id, message);
});
});
}
+function filterUrl(log) {
Review Comment:
Code smell to have two different return types depending on conditional.
Since you're not actually modifying the log, you don't need to return the log.
Better to have function name like `isUrlWhitelisted` which only returns bool.
##########
build/UserALEWebExtension/background.js:
##########
@@ -1131,28 +1155,48 @@ var defaultConfig = {
authHeader: null,
toolName: 'useralePlugin',
version: version
+ },
+ pluginConfig: {
+ // Default to a regex that will match no string
+ urlWhitelist: '(?!x)x'
}
};
-browser.storage.local.get(defaultConfig, function (res) {
- options(res.useraleConfig);
-});
+var urlWhitelist;
+function updateConfig(config) {
+ urlWhitelist = new RegExp(config.pluginConfig.urlWhitelist);
+ options(config.useraleConfig);
+ dispatchTabMessage(config.useraleConfig);
+}
function dispatchTabMessage(message) {
browser.tabs.query({}, function (tabs) {
tabs.forEach(function (tab) {
browser.tabs.sendMessage(tab.id, message);
});
});
}
+function filterUrl(log) {
+ if (urlWhitelist.test(log.pageUrl)) {
+ return log;
+ }
+ return false;
+}
+browser.storage.local.get(defaultConfig, function (res) {
+ addCallbacks({
+ filterUrl: filterUrl
+ });
+ updateConfig(res);
+});
browser.runtime.onMessage.addListener(function (message) {
switch (message.type) {
// Handles logs rerouted from content and option scripts
case ADD_LOG:
- log(message.payload);
+ var log$1 = filterUrl(message.payload);
Review Comment:
Then here, you just check if it's whitelisted. if not, return early,
otherwise proceed.
```js
if !isUrlWhitelisted(message.payload) return
log(message.payload)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@flagon.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@flagon.apache.org
For additional commands, e-mail: notifications-h...@flagon.apache.org
