Add noopener, modernize vars, fix favicon logic#212
Open
itz-rj-here wants to merge 1 commit into
Open
Conversation
Security and code quality updates across the UI and scripts: - index.html: Add rel="noopener noreferrer" to external links opened with target="_blank". - scripts/clock.js, scripts/search-suggestions.js, scripts/search.js, scripts/bookmarks.js: Replace function-scoped var with const/let to modernize code and reduce hoisting-related issues; small formatting/semicolon fixes. - scripts/bookmarks.js: Fix favicon selection logic to correctly exclude both Firefox and Opera (use && instead of ||) and initialize bookmarksAPI with let. - scripts/backup-restore.js: Remove noisy console.log after backup completion. - scripts/search.js: Attach mouseenter/mouseleave handlers to dropdown items once (prevent repeated listener registration) and keep updateSelection focused on keyboard navigation. These changes improve security, correctness, and performance/readability.
📝 WalkthroughWalkthroughThis PR applies three categories of improvements: security hardening on external links by adding ChangesSecurity, scoping, and logic improvements
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/search.js`:
- Around line 262-270: The mouseenter handler on dropdownItems currently only
adds the "selected" class to the hovered item, allowing multiple items to remain
selected and breaking lookup by document.querySelector(".selected"); update the
handlers for the dropdownItems loop so that on "mouseenter" you first remove
"selected" from any currently selected item (e.g., document.querySelectorAll or
document.querySelector(".selected") and remove the class) and then add
"selected" to the hovered item, and ensure "mouseleave" still removes the class
from the hovered item; reference the dropdownItems.forEach loop and the
"mouseenter"/"mouseleave" handlers to locate where to change behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1510df5-a062-465c-9d82-bc3bd4d616e0
📒 Files selected for processing (6)
index.htmlscripts/backup-restore.jsscripts/bookmarks.jsscripts/clock.jsscripts/search-suggestions.jsscripts/search.js
💤 Files with no reviewable changes (1)
- scripts/backup-restore.js
Comment on lines
+262
to
+270
| // Attach mouse hover listeners once (not on every keydown) | ||
| dropdownItems.forEach((item) => { | ||
| item.addEventListener("mouseenter", () => { | ||
| item.classList.add("selected"); | ||
| }); | ||
| item.addEventListener("mouseleave", () => { | ||
| item.classList.remove("selected"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Clear existing .selected before applying mouse hover selection.
Hover now only adds/removes class per item, so multiple dropdown items can stay .selected; Enter may then pick the wrong one via querySelector(".selected").
Suggested fix
dropdownItems.forEach((item) => {
item.addEventListener("mouseenter", () => {
+ dropdownItems.forEach((el) => el.classList.remove("selected"));
item.classList.add("selected");
+ selectedIndex = Array.from(dropdownItems).indexOf(item);
});
item.addEventListener("mouseleave", () => {
item.classList.remove("selected");
});
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/search.js` around lines 262 - 270, The mouseenter handler on
dropdownItems currently only adds the "selected" class to the hovered item,
allowing multiple items to remain selected and breaking lookup by
document.querySelector(".selected"); update the handlers for the dropdownItems
loop so that on "mouseenter" you first remove "selected" from any currently
selected item (e.g., document.querySelectorAll or
document.querySelector(".selected") and remove the class) and then add
"selected" to the hovered item, and ensure "mouseleave" still removes the class
from the hovered item; reference the dropdownItems.forEach loop and the
"mouseenter"/"mouseleave" handlers to locate where to change behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 Description
This PR applies security hardening, bug fixes, and code quality improvements across the extension's HTML and JavaScript files:
Security:
rel="noopener noreferrer"to all 4 externaltarget="_blank"links inindex.htmlto prevent reverse tabnapping attacks, where an opened page could accesswindow.openerand redirect the extension tab.Bug Fixes:
bookmarks.js— Fixed favicon selection logic: the condition!isFirefox || !isOperawas alwaystrue(De Morgan's law), so Chrome's_faviconAPI was incorrectly attempted on Opera and Firefox. Changed to!isFirefox && !isOperato correctly fall back to Google's favicon service for non-Chromium browsers.search.js— Fixed a memory leak inupdateSelection()wheremouseenter/mouseleaveevent listeners were being re-attached to every dropdown item on every keystroke (called from thekeydownhandler). Listeners are now attached once during initialization, andupdateSelection()is focused solely on keyboard navigation state.Code Quality:
vardeclarations withconst/letacrossclock.js(11 occurrences),search.js(2),search-suggestions.js(1), andbookmarks.js(1) to use block scoping and prevent hoisting-related issues.console.log("Backup completed successfully!")frombackup-restore.jsthat shouldn't be in production code.🎨 Visual Changes (Screenshots / Videos)
No visual changes — all modifications are under-the-hood security, correctness, and performance improvements.
🔗 Related Issues
✅ Checklist
🤖 AI Assistance (Coding)
Security Hardening
Added
rel="noopener noreferrer"to four external links withtarget="_blank"inindex.html(Host your own proxy, Privacy policy, Learn more, and Firefox informational link) to prevent reverse tabnapping attacks.Bug Fixes
bookmarks.js: Fixed favicon selection logic by correcting the condition from
!isFirefox || !isOpera(which would always evaluate to true) to!isFirefox && !isOpera. This prevents the Chromium-specific_faviconAPI from being incorrectly invoked on Firefox and Opera browsers.search.js: Fixed a memory leak by refactoring dropdown item highlight handling. Mouse event listeners (
mouseenter/mouseleave) are now attached once during initialization rather than being repeatedly re-registered. TheupdateSelection()function now handles only keyboard navigation state and DOM updates without re-attaching event listeners.Code Modernization
Replaced
vardeclarations withconst/letacross multiple files for improved block scoping and to avoid hoisting issues:initializeClock(),updateDate(), andupdateanalogclock())fallbackUrlandsearchUrl)selectedOption)bookmarksAPI)Cleanup
Removed leftover debug statement
console.log("Backup completed successfully!")fromscripts/backup-restore.js.