Skip to content

feat: add option to add custom blocked domains#41

Open
Justin24506 wants to merge 3 commits intoShantanugupta43:mainfrom
Justin24506:feature/better-blocked-domains
Open

feat: add option to add custom blocked domains#41
Justin24506 wants to merge 3 commits intoShantanugupta43:mainfrom
Justin24506:feature/better-blocked-domains

Conversation

@Justin24506
Copy link
Copy Markdown

@Justin24506 Justin24506 commented Apr 3, 2026

Foundation laid for custom blocked domains.

Need to

    • Strip http:// / https:// from pasted URLs

and more.

@Shantanugupta43
Copy link
Copy Markdown
Owner

Shantanugupta43 commented Apr 3, 2026

Hey I am curious what you are working on. If possible can you also include the description in issues tab? Cheers

@Justin24506
Copy link
Copy Markdown
Author

#42 Done.

@Justin24506 Justin24506 marked this pull request as ready for review April 4, 2026 16:47
Copilot AI review requested due to automatic review settings April 4, 2026 16:47
@Justin24506 Justin24506 changed the title feat(WIP): add option to add custom blocked domains feat: add option to add custom blocked domains Apr 4, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an initial “custom blocked domains” feature to the extension so users can configure domains where suggestions should be disabled.

Changes:

  • Adds a “Blocked Sites” UI in the popup settings with tag-style add/remove behavior.
  • Persists blockedDomains into extension config and returns it via getConfig.
  • Updates the content script to disable itself on blocked domains (now intended to be configurable).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
src/popup/popup.js Adds blocked domain input handling, domain sanitization, and tag rendering; saves blockedDomains into config.
src/popup/popup.html Adds the “Blocked Sites” section and container markup in Settings.
src/popup/popup.css Adds styling for the blocked domain tags/input container.
src/content/content-script.js Attempts to load blocked domains from config and short-circuit initialization on blocked sites.
src/config/config-manager.js Adds blockedDomains to stored config initialization defaults.
src/background/service-worker.js Exposes blockedDomains via getConfig response.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 6 to 7
import configManager from "../config/config-manager.js";

Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import configManager ... will throw in a Manifest V3 content script unless the content script is loaded as an ES module (otherwise you get “Cannot use import statement outside a module”). Either update the manifest content_scripts entry to load this file as a module, or remove the import and fetch blockedDomains via chrome.runtime.sendMessage / chrome.storage directly.

Suggested change
import configManager from "../config/config-manager.js";
const configManager = {
async get(key, defaultValue = null) {
try {
const syncResult = await chrome.storage.sync.get(key);
if (syncResult[key] !== undefined) {
return syncResult[key];
}
const localResult = await chrome.storage.local.get(key);
if (localResult[key] !== undefined) {
return localResult[key];
}
} catch (error) {
console.warn("Failed to read config from extension storage:", error);
}
return defaultValue;
},
async getBlockedDomains() {
const blockedDomains = await this.get("blockedDomains", []);
return Array.isArray(blockedDomains) ? blockedDomains : [];
},
};

Copilot uses AI. Check for mistakes.
return;
async function initialize() {
try {
const blockedDomains = (await configManager.get("blockedDomains")) || [];
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocked domains are read via configManager.get("blockedDomains") without calling await configManager.initialize(). Since get() just reads this.config and this.config is null until initialized, this will always fall back to [] and the blocking feature won’t work. Initialize the config manager first (and you can drop the unnecessary await on get).

Suggested change
const blockedDomains = (await configManager.get("blockedDomains")) || [];
await configManager.initialize();
const blockedDomains = configManager.get("blockedDomains") || [];

Copilot uses AI. Check for mistakes.
if (!Array.isArray(blockedDomains)) return false;
const host = window.location.hostname.toLowerCase();
return BLOCKED_DOMAINS.some(domain => host.includes(domain));
return blockedDomains.some((domain) => host.includes(domain));
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

host.includes(domain) will produce false positives (e.g. blocking in.com blocks linkedin.com, blocking linkedin.com blocks notlinkedin.com). Domain matching should be based on exact host match or a subdomain suffix check (e.g. host === domain || host.endsWith('.' + domain)).

Suggested change
return blockedDomains.some((domain) => host.includes(domain));
return blockedDomains.some((domain) => {
if (typeof domain !== "string") return false;
const normalizedDomain = domain.trim().toLowerCase().replace(/^\.+/, "");
if (!normalizedDomain) return false;
return (
host === normalizedDomain || host.endsWith(`.${normalizedDomain}`)
);
});

Copilot uses AI. Check for mistakes.
derivationEl.className = 'suggestion-derivation';
const derivationEl = document.createElement("div");
derivationEl.className = "suggestion-derivation";
derivationEl.innerHTML = `<strong>Why:</strong> ${derivation}`;
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

derivationEl.innerHTML = ... ${derivation} injects untrusted LLM-provided text into the extension popup as HTML, which is an XSS vector (LLM output can contain tags). Prefer setting textContent for the derivation portion (or sanitize/escape before assigning to innerHTML).

Suggested change
derivationEl.innerHTML = `<strong>Why:</strong> ${derivation}`;
const derivationLabelEl = document.createElement("strong");
derivationLabelEl.textContent = "Why:";
derivationEl.appendChild(derivationLabelEl);
derivationEl.appendChild(document.createTextNode(` ${derivation}`));

Copilot uses AI. Check for mistakes.
Comment on lines +663 to +675
.tag {
background: var(--primary);
color: white;
padding: 4px 10px;
border-radius: 6px;
display: inline-flex;
align-items: center;
gap: 6px;
font-size: 13px;
font-weight: 500;
user-select: none;
animation: fadeIn 0.2s ease;
}
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.tag uses animation: fadeIn 0.2s ease; but there is no @keyframes fadeIn defined in this stylesheet, so the animation won’t run (and can generate warnings in devtools). Define the keyframes or remove the animation reference.

Copilot uses AI. Check for mistakes.
@Justin24506
Copy link
Copy Markdown
Author

Justin24506 commented Apr 4, 2026

I've made some fixes:

  • urls are stripped of https:// and only the hostname is added
  • invalid urls are handled

I'll now try to fix the errors pointed out by Copliot.

- Replaced content script import with inline config to fix Manifest V3 module errors
- Swapped .innerHTML for text nodes to prevent XSS in popup
- Added aria-labels and focus outlines for better accessibility
@Justin24506
Copy link
Copy Markdown
Author

Justin24506 commented Apr 4, 2026

Sorry for cluttering this up, didn't realize Copilot's automatic review was on.

@Shantanugupta43
Copy link
Copy Markdown
Owner

Since you did a draft PR, it triggered copilots auto reviewer. It doesn't trigger on default pull requests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants