Simplify extension controls and add options page#19
Open
ArsalaBangash wants to merge 3 commits intomainfrom
Open
Simplify extension controls and add options page#19ArsalaBangash wants to merge 3 commits intomainfrom
ArsalaBangash wants to merge 3 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds a new options/settings page and build entry, simplifies the popup to a compact status view, removes the CustomFocus mode (two FocusMode states remain), strips many storage-driven behaviors, and refactors focus/controller logic (LinkedIn/X controllers updated to conceal/restore UI panels/sidebar). Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
34-34:⚠️ Potential issue | 🟡 MinorIncorrect output folder in installation instructions.
The build script outputs to
extension-build, but the README instructs users to select thedistfolder. This will cause the extension to fail to load.📝 Proposed fix
-5. Click "Load unpacked" and select the `dist` folder +5. Click "Load unpacked" and select the `extension-build` folder🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 34, Update the README installation step that currently says to select the `dist` folder: change the instruction to tell users to click "Load unpacked" and select the `extension-build` folder (the actual output of the build script) so the extension loads correctly; locate and update the line that reads `Click "Load unpacked" and select the \`dist\` folder` to reference `extension-build` instead.
🧹 Nitpick comments (10)
src/ts/quotes/index.ts (1)
25-46: Consider removing theasynckeyword since the function no longer awaits anything.The
createQuoteElementfunction is declaredasyncbut contains noawaitcalls after the removal of storage-based font size lookups. The function can be simplified to returnHTMLDivElementdirectly.♻️ Suggested simplification
-const createQuoteElement = async (): Promise<HTMLDivElement> => { +const createQuoteElement = (): HTMLDivElement => { const quote = getRandomQuote(); const quoteDiv = document.createElement('div');Note: This would also require updating callers that currently
awaitthis function, and updating the corresponding test'sbeforeEachto be synchronous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ts/quotes/index.ts` around lines 25 - 46, The createQuoteElement function is marked async but does not await anything; remove the async keyword and change its return type from Promise<HTMLDivElement> to HTMLDivElement in the function signature (createQuoteElement) so it returns the div synchronously, then update any callers/tests that currently use await createQuoteElement() (and the related beforeEach) to call it synchronously and adjust tests to be non-async.src/options/options.ts (1)
1-1: Placeholder module noted.This empty export creates a valid ES module entry point for the build system. Consider adding a TODO comment if this is intended to be expanded with options page logic in the future.
💡 Optional: Add intent comment
+// TODO: Add options page logic (e.g., settings persistence, UI interactions) export {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/options/options.ts` at line 1, This module currently only exports an empty object (export {}), acting as a placeholder; add a short TODO/intent comment at the top of the file indicating that this is an intentional placeholder for future options page logic (e.g., "TODO: implement options page logic" or "Placeholder module for options exports") so maintainers know the empty export is deliberate.src/ts/websites/linkedin/linkedin-controller.ts (1)
129-136: Consider extractingrestoreInlineStyleto the base class or a shared utility.This helper function could be reused by other controllers (e.g.,
XController) if they adopt similar concealment patterns in the future. Currently it's a module-level function, which works but limits reusability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ts/websites/linkedin/linkedin-controller.ts` around lines 129 - 136, The module-level helper restoreInlineStyle should be moved to a shared location so other controllers can reuse it; extract restoreInlineStyle into a shared utility module (or into the common/base controller class used by LinkedInController and future XController) and replace the local function with an import or base-class method call; update LinkedInController to call the new shared function or base method (preserving the same signature restoreInlineStyle(element: HTMLElement, property: string, value: string): void) and adjust other controllers to reuse it to avoid duplication.src/ts/websites/linkedin/linkedin-controller.test.ts (1)
4-12: Mock may be incomplete forhasFeedLoadedandisFeedHidden.The
LinkedInUtilsmock doesn't includehasFeedLoadedorisFeedHidden, which are referenced byLinkedInController.focus()and the feed distraction setup. Whileunfocus()doesn't directly call these (it callssetFeedVisibility(true)which usesshowFeed()), adding these mocks would make the mock more complete and prevent issues if test coverage expands.💡 More complete mock
jest.mock('./linkedin-utils', () => ({ __esModule: true, default: { isHomePage: jest.fn(), getLinkedInFeed: jest.fn(), getLinkedInPanels: jest.fn(), arePanelsHidden: jest.fn(), + hasFeedLoaded: jest.fn(), + isFeedHidden: jest.fn(), }, }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ts/websites/linkedin/linkedin-controller.test.ts` around lines 4 - 12, The LinkedInUtils mock is missing hasFeedLoaded and isFeedHidden which LinkedInController.focus() and feed visibility logic reference; update the jest.mock for LinkedInUtils to include hasFeedLoaded: jest.fn() and isFeedHidden: jest.fn() (add them alongside isHomePage, getLinkedInFeed, getLinkedInPanels, arePanelsHidden) so tests can stub/override their return values when exercising LinkedInController.focus(), unfocus(), setFeedVisibility(true) and showFeed().src/ts/websites/x/x-controller.test.ts (2)
20-39: Consider adding an assertion for feed visibility restoration.The test name states "while restoring the feed" but only asserts the sidebar state. Adding an assertion for the feed element's visibility would make the test more comprehensive and match its description.
💡 Suggested improvement
const controller = new XController() controller.unfocus() expect(sidebar.style.display).toBe('none') + // Verify feed was restored (not hidden) + expect(feed.style.display).not.toBe('none') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ts/websites/x/x-controller.test.ts` around lines 20 - 39, The test currently asserts only that the sidebar remains hidden after controller.unfocus; add an assertion that the feed element returned by XUtils.getXFeed (the feed variable) is restored to visible. After calling controller.unfocus() in the test, add an assertion such as expecting feed.style.display not to be 'none' (or to equal the expected visible value), so the test name "while restoring the feed" matches the verification of XController restoring the feed's visibility.
24-27: Consider cleaning updocument.URLmodification inafterEach.Modifying
document.URLwithObject.definePropertypersists across tests if not reset. This could cause flaky behavior if more tests are added.💡 Suggested cleanup
describe('XController', () => { + const originalURL = document.URL + beforeEach(() => { jest.clearAllMocks() }) + + afterEach(() => { + Object.defineProperty(document, 'URL', { + value: originalURL, + writable: true, + }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ts/websites/x/x-controller.test.ts` around lines 24 - 27, The test mutates document.URL via Object.defineProperty in x-controller.test.ts and doesn't restore it; update the test to capture the original value before modifying (cache original = document.URL), apply Object.defineProperty(document, 'URL', { value: 'https://x.com/home', writable: true }) in the test setup, and add/modify an afterEach hook to restore document.URL by re-applying Object.defineProperty(document, 'URL', { value: original, writable: true }) (use the existing afterEach or add one if missing) so the mutation does not leak between tests.src/ts/background.ts (1)
25-31: Theget()result is unused; consider simplifying.The
get(['appState'])call retrieves stored data but the callback ignores it, immediately overwriting with the baseline. If the intent is simply to reset state on worker restart,set()alone suffices.💡 Simplified initialization
// In MV3 the worker restarts frequently. Reset appState on startup so each // worker begins from the same focused baseline. -browser.storage.local.get(['appState']).then(() => { - browser.storage.local.set({ - appState: appState, - }) +browser.storage.local.set({ + appState: appState, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ts/background.ts` around lines 25 - 31, The storage initialization calls browser.storage.local.get(['appState']) but ignores its result and immediately overwrites the value; remove the unused get call and call browser.storage.local.set({ appState: appState }) directly so the worker simply resets state on startup—update the code that references browser.storage.local.get and browser.storage.local.set and leave the appState variable usage intact.src/options/options.html (1)
2-6: Addlangattribute and move charset meta for better practices.Consider adding the
langattribute to the<html>element for accessibility (screen readers) and moving the charset meta to be first in<head>for proper encoding detection.♻️ Suggested improvements
<!DOCTYPE html> -<html> +<html lang="en"> <head> + <meta charset="UTF-8" /> <title>Focused Browsing Settings</title> <link rel="stylesheet" type="text/css" href="options.css" /> - <meta charset="UTF-8" /> </head>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/options/options.html` around lines 2 - 6, Update the <html> element to include a lang attribute (e.g., lang="en") and move the <meta charset="UTF-8" /> element to be the very first child inside the <head> so encoding is detected early; locate these changes around the existing <html> and <head> tags in options.html and adjust accordingly.src/popup/popup.ts (1)
7-9: Consider adding error handling foropenOptionsPage.If
openOptionsPage()fails (e.g., options page not found), the rejected promise is unhandled. While rare, adding a catch or removing the unnecessaryawaitwould be cleaner.♻️ Option 1: Remove unnecessary await
openOptionsButton.addEventListener('click', async () => { - await browser.runtime.openOptionsPage(); + browser.runtime.openOptionsPage(); });♻️ Option 2: Add error handling
openOptionsButton.addEventListener('click', async () => { - await browser.runtime.openOptionsPage(); + try { + await browser.runtime.openOptionsPage(); + } catch (err) { + console.error('Failed to open options page:', err); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/popup/popup.ts` around lines 7 - 9, The click handler for openOptionsButton currently awaits browser.runtime.openOptionsPage() without error handling; either remove the unnecessary await to let the promise resolve naturally or wrap the call in a try/catch and handle failures (e.g., log the error via console.error or show a user-friendly message). Locate the event listener on openOptionsButton and update it to either call browser.runtime.openOptionsPage() without await or surround the await browser.runtime.openOptionsPage() with try { ... } catch (err) { console.error('Failed to open options page', err); } so rejected promises are handled.src/popup/popup.css (1)
113-117: Reconsider removing outline on focus-visible for accessibility.Setting
outline: noneon:focus-visibleremoves the default focus indicator for keyboard users. While the color change provides some feedback, consider adding a visible focus ring or underline for better keyboard navigation accessibility.♿ Proposed fix to improve focus visibility
.options-button:hover, .options-button:focus-visible { color: var(--ink); - outline: none; + outline: 2px solid var(--accent); + outline-offset: 2px; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/popup/popup.css` around lines 113 - 117, The CSS rule for .options-button:focus-visible currently removes the browser focus indicator (outline: none), harming keyboard accessibility; update the .options-button:hover and .options-button:focus-visible rules to keep hover styling but remove the outline:none and instead add a visible focus style for .options-button:focus-visible (e.g., a clearly contrasting outline or focus ring using outline or box-shadow tied to a CSS variable like --focus or --ink) so keyboard users get a high-contrast, visible indicator while preserving the existing color change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@README.md`:
- Line 34: Update the README installation step that currently says to select the
`dist` folder: change the instruction to tell users to click "Load unpacked" and
select the `extension-build` folder (the actual output of the build script) so
the extension loads correctly; locate and update the line that reads `Click
"Load unpacked" and select the \`dist\` folder` to reference `extension-build`
instead.
---
Nitpick comments:
In `@src/options/options.html`:
- Around line 2-6: Update the <html> element to include a lang attribute (e.g.,
lang="en") and move the <meta charset="UTF-8" /> element to be the very first
child inside the <head> so encoding is detected early; locate these changes
around the existing <html> and <head> tags in options.html and adjust
accordingly.
In `@src/options/options.ts`:
- Line 1: This module currently only exports an empty object (export {}), acting
as a placeholder; add a short TODO/intent comment at the top of the file
indicating that this is an intentional placeholder for future options page logic
(e.g., "TODO: implement options page logic" or "Placeholder module for options
exports") so maintainers know the empty export is deliberate.
In `@src/popup/popup.css`:
- Around line 113-117: The CSS rule for .options-button:focus-visible currently
removes the browser focus indicator (outline: none), harming keyboard
accessibility; update the .options-button:hover and
.options-button:focus-visible rules to keep hover styling but remove the
outline:none and instead add a visible focus style for
.options-button:focus-visible (e.g., a clearly contrasting outline or focus ring
using outline or box-shadow tied to a CSS variable like --focus or --ink) so
keyboard users get a high-contrast, visible indicator while preserving the
existing color change.
In `@src/popup/popup.ts`:
- Around line 7-9: The click handler for openOptionsButton currently awaits
browser.runtime.openOptionsPage() without error handling; either remove the
unnecessary await to let the promise resolve naturally or wrap the call in a
try/catch and handle failures (e.g., log the error via console.error or show a
user-friendly message). Locate the event listener on openOptionsButton and
update it to either call browser.runtime.openOptionsPage() without await or
surround the await browser.runtime.openOptionsPage() with try { ... } catch
(err) { console.error('Failed to open options page', err); } so rejected
promises are handled.
In `@src/ts/background.ts`:
- Around line 25-31: The storage initialization calls
browser.storage.local.get(['appState']) but ignores its result and immediately
overwrites the value; remove the unused get call and call
browser.storage.local.set({ appState: appState }) directly so the worker simply
resets state on startup—update the code that references
browser.storage.local.get and browser.storage.local.set and leave the appState
variable usage intact.
In `@src/ts/quotes/index.ts`:
- Around line 25-46: The createQuoteElement function is marked async but does
not await anything; remove the async keyword and change its return type from
Promise<HTMLDivElement> to HTMLDivElement in the function signature
(createQuoteElement) so it returns the div synchronously, then update any
callers/tests that currently use await createQuoteElement() (and the related
beforeEach) to call it synchronously and adjust tests to be non-async.
In `@src/ts/websites/linkedin/linkedin-controller.test.ts`:
- Around line 4-12: The LinkedInUtils mock is missing hasFeedLoaded and
isFeedHidden which LinkedInController.focus() and feed visibility logic
reference; update the jest.mock for LinkedInUtils to include hasFeedLoaded:
jest.fn() and isFeedHidden: jest.fn() (add them alongside isHomePage,
getLinkedInFeed, getLinkedInPanels, arePanelsHidden) so tests can stub/override
their return values when exercising LinkedInController.focus(), unfocus(),
setFeedVisibility(true) and showFeed().
In `@src/ts/websites/linkedin/linkedin-controller.ts`:
- Around line 129-136: The module-level helper restoreInlineStyle should be
moved to a shared location so other controllers can reuse it; extract
restoreInlineStyle into a shared utility module (or into the common/base
controller class used by LinkedInController and future XController) and replace
the local function with an import or base-class method call; update
LinkedInController to call the new shared function or base method (preserving
the same signature restoreInlineStyle(element: HTMLElement, property: string,
value: string): void) and adjust other controllers to reuse it to avoid
duplication.
In `@src/ts/websites/x/x-controller.test.ts`:
- Around line 20-39: The test currently asserts only that the sidebar remains
hidden after controller.unfocus; add an assertion that the feed element returned
by XUtils.getXFeed (the feed variable) is restored to visible. After calling
controller.unfocus() in the test, add an assertion such as expecting
feed.style.display not to be 'none' (or to equal the expected visible value), so
the test name "while restoring the feed" matches the verification of XController
restoring the feed's visibility.
- Around line 24-27: The test mutates document.URL via Object.defineProperty in
x-controller.test.ts and doesn't restore it; update the test to capture the
original value before modifying (cache original = document.URL), apply
Object.defineProperty(document, 'URL', { value: 'https://x.com/home', writable:
true }) in the test setup, and add/modify an afterEach hook to restore
document.URL by re-applying Object.defineProperty(document, 'URL', { value:
original, writable: true }) (use the existing afterEach or add one if missing)
so the mutation does not leak between tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8518e6f3-f4f4-4974-ad66-8255d6701ac4
📒 Files selected for processing (26)
README.mdbuild.jsreports/selector-health/selector-health-report.jsonsrc/manifest.jsonsrc/options/options.csssrc/options/options.htmlsrc/options/options.html.d.tssrc/options/options.test.tssrc/options/options.tssrc/popup/popup.csssrc/popup/popup.htmlsrc/popup/popup.test.tssrc/popup/popup.tssrc/ts/__mocks__/webextension-polyfill-ts.tssrc/ts/background.tssrc/ts/focus/focus.tssrc/ts/focus/types.tssrc/ts/quotes/index.tssrc/ts/quotes/quotes.test.tssrc/ts/quotes/text-size.tssrc/ts/websites/linkedin/linkedin-controller.test.tssrc/ts/websites/linkedin/linkedin-controller.tssrc/ts/websites/website-config.tssrc/ts/websites/website-controller.tssrc/ts/websites/x/x-controller.test.tssrc/ts/websites/x/x-controller.ts
💤 Files with no reviewable changes (2)
- src/ts/websites/website-config.ts
- src/ts/quotes/text-size.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
README.md (2)
9-12: Clarify YouTube behavior across both modes.LinkedIn and X explain what changes in Focused vs Unfocused, but YouTube only describes one state. In the new two-state UX, that leaves it unclear whether Unfocused behaves differently on YouTube or is intentionally identical.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 9 - 12, Update the "YouTube" bullet in the "Supported sites" section of README.md to describe both Focused and Unfocused modes explicitly: state what Focused mode hides (recommended videos, comments, suggestions, etc.) and what Unfocused mode restores (e.g., brings back recommendations and comments while still preserving video playback) or note if Unfocused is identical to Focused; ensure the bullet matches the style of the LinkedIn and X entries so readers can clearly see the differences between Focused and Unfocused for YouTube.
23-26: Document how users open the Settings page.This reads like a future placeholder, but the PR adds an options page entry point now. Please either add a concrete access path for users or explicitly say the page is scaffold-only today so the README does not undersell or obscure the new entry point.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 23 - 26, Update the "Settings page" README section to either (a) document the concrete access path for users (e.g., explain how to open the Options/Settings page via the extension menu or a specific URL like chrome-extension://<extension-id>/options.html and mention any keyboard/toolbar steps), or (b) explicitly state that the page is scaffold-only today and not yet user-facing; modify the "Settings page" header content accordingly so readers know whether the options page is reachable now or is a future placeholder.src/ts/websites/x/x-controller.test.ts (1)
31-57: Consider adding a focus-mode test case for completeness.The test verifies unfocus behavior, but there's no corresponding test for focus mode (verifying both feed and sidebar are hidden). This would provide better coverage of the two-state behavior mentioned in the PR objectives.
Example additional test
it('hides both feed and sidebar in focused mode', () => { const feed = document.createElement('div') const sidebar = document.createElement('aside') Object.defineProperty(document, 'URL', { value: 'https://x.com/home', writable: true, }) ;(XUtils.isHomePage as jest.Mock).mockReturnValue(true) ;(XUtils.getXFeed as jest.Mock).mockReturnValue(feed) ;(XUtils.getXSidebar as jest.Mock).mockReturnValue(sidebar) ;(XUtils.hasFeedLoaded as jest.Mock).mockReturnValue(true) ;(XUtils.isFeedHidden as jest.Mock).mockReturnValue(false) ;(XUtils.isSidebarHidden as jest.Mock).mockReturnValue(false) const controller = new XController() controller.focus() expect(feed.style.display).toBe('none') expect(sidebar.style.display).toBe('none') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ts/websites/x/x-controller.test.ts` around lines 31 - 57, Add a complementary focused-mode unit test to x-controller.test.ts that mirrors the existing unfocused test: mock document.URL to a home URL and set XUtils.isHomePage, XUtils.getXFeed, XUtils.getXSidebar, XUtils.hasFeedLoaded, XUtils.isFeedHidden and XUtils.isSidebarHidden appropriately, instantiate XController and call controller.focus(), then assert that both the feed and sidebar elements have style.display === 'none' (use XController and its focus() method and the XUtils mocks to locate the behavior to test).src/ts/background.ts (1)
25-29: Consider handling the storage initialization promise.The
browser.storage.local.set()call at module scope doesn't handle potential rejection. While MV3 workers commonly use this pattern, a silent failure during startup could leave the extension in an inconsistent state without any indication.♻️ Optional: Add error handling for visibility
// In MV3 the worker restarts frequently. Reset appState on startup so each // worker begins from the same focused baseline. browser.storage.local.set({ appState: appState, -}) +}).catch((error) => { + console.error('Failed to initialize appState:', error); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ts/background.ts` around lines 25 - 29, The call to browser.storage.local.set({ appState }) at module scope ignores the returned promise and can silently fail; update the startup flow (e.g., in an async init function or an IIFE) to await or attach a .catch to browser.storage.local.set({ appState }) and surface failures via logging (console.error or your extension logger) and optional fallback behavior so errors during the appState initialization are visible and handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/popup/popup.css`:
- Around line 53-59: The .title CSS rule uses a very tight line-height (0.92)
which risks clipping descenders when the title wraps due to the max-width:
250px; update the .title rule to increase line-height to at least 1.0–1.1 (or
remove the explicit line-height to use the browser default) and/or ensure the
title is constrained to a single line (e.g., disable wrapping) so descenders (g,
y, p, q, j) do not overlap; adjust the .title selector accordingly.
- Around line 87-92: The --accent color (`#3b82f6`) does not meet WCAG AA contrast
(4.5:1) against --paper (`#f5f7fa`), causing .index-label and .info-label to be
insufficiently readable; update the --accent CSS variable to a darker hue that
yields at least 4.5:1 contrast against --paper (or alternatively increase
font-weight/size for .index-label and .info-label), then verify contrast with a
tool. Locate the --accent definition (CSS custom property) and change it to a
darker hex that passes the 4.5:1 check, or adjust .index-label and .info-label
styles to ensure their rendered text meets the required contrast threshold.
Ensure you test the final color against --paper to confirm compliance.
In `@src/ts/websites/x/x-controller.test.ts`:
- Line 55: The test is asserting the wrong element — change the assertion to
verify the feed element that the controller manipulates: obtain the element from
getFeedElement() (which wraps XUtils.getXFeed()) or the mocked feed variable and
assert on its style.display after calling setFeedVisibility(), instead of
asserting on feedItem.style.display; update the test to call
XController.getFeedElement() or reference the mocked feed and
expect(feed.style.display).not.toBe('none') (and adjust any related setup to
ensure the feed is the mocked element).
---
Nitpick comments:
In `@README.md`:
- Around line 9-12: Update the "YouTube" bullet in the "Supported sites" section
of README.md to describe both Focused and Unfocused modes explicitly: state what
Focused mode hides (recommended videos, comments, suggestions, etc.) and what
Unfocused mode restores (e.g., brings back recommendations and comments while
still preserving video playback) or note if Unfocused is identical to Focused;
ensure the bullet matches the style of the LinkedIn and X entries so readers can
clearly see the differences between Focused and Unfocused for YouTube.
- Around line 23-26: Update the "Settings page" README section to either (a)
document the concrete access path for users (e.g., explain how to open the
Options/Settings page via the extension menu or a specific URL like
chrome-extension://<extension-id>/options.html and mention any keyboard/toolbar
steps), or (b) explicitly state that the page is scaffold-only today and not yet
user-facing; modify the "Settings page" header content accordingly so readers
know whether the options page is reachable now or is a future placeholder.
In `@src/ts/background.ts`:
- Around line 25-29: The call to browser.storage.local.set({ appState }) at
module scope ignores the returned promise and can silently fail; update the
startup flow (e.g., in an async init function or an IIFE) to await or attach a
.catch to browser.storage.local.set({ appState }) and surface failures via
logging (console.error or your extension logger) and optional fallback behavior
so errors during the appState initialization are visible and handled.
In `@src/ts/websites/x/x-controller.test.ts`:
- Around line 31-57: Add a complementary focused-mode unit test to
x-controller.test.ts that mirrors the existing unfocused test: mock document.URL
to a home URL and set XUtils.isHomePage, XUtils.getXFeed, XUtils.getXSidebar,
XUtils.hasFeedLoaded, XUtils.isFeedHidden and XUtils.isSidebarHidden
appropriately, instantiate XController and call controller.focus(), then assert
that both the feed and sidebar elements have style.display === 'none' (use
XController and its focus() method and the XUtils mocks to locate the behavior
to test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef8f0d28-d0ea-4c2d-89d9-3c068a218755
📒 Files selected for processing (12)
README.mdreports/selector-health/selector-health-report.jsonsrc/options/options.htmlsrc/options/options.tssrc/popup/popup.csssrc/popup/popup.tssrc/ts/background.tssrc/ts/quotes/index.tssrc/ts/quotes/quotes.test.tssrc/ts/websites/linkedin/linkedin-controller.test.tssrc/ts/websites/website-controller.tssrc/ts/websites/x/x-controller.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/ts/websites/website-controller.ts
- reports/selector-health/selector-health-report.json
- src/ts/websites/linkedin/linkedin-controller.test.ts
- src/options/options.ts
- src/options/options.html
| controller.focus() | ||
| controller.unfocus() | ||
|
|
||
| expect(feedItem.style.display).not.toBe('none') |
There was a problem hiding this comment.
Assertion checks wrong element — should verify feed, not feedItem.
Looking at x-controller.ts, getFeedElement() returns the result of XUtils.getXFeed() (which is feed here), and setFeedVisibility() operates on that element. The test asserts on feedItem.style.display, but feedItem is never directly manipulated by the controller. This assertion passes trivially because feedItem's display is never set.
Proposed fix
- expect(feedItem.style.display).not.toBe('none')
+ expect(feed.style.display).not.toBe('none')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(feedItem.style.display).not.toBe('none') | |
| expect(feed.style.display).not.toBe('none') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ts/websites/x/x-controller.test.ts` at line 55, The test is asserting the
wrong element — change the assertion to verify the feed element that the
controller manipulates: obtain the element from getFeedElement() (which wraps
XUtils.getXFeed()) or the mocked feed variable and assert on its style.display
after calling setFeedVisibility(), instead of asserting on
feedItem.style.display; update the test to call XController.getFeedElement() or
reference the mocked feed and expect(feed.style.display).not.toBe('none') (and
adjust any related setup to ensure the feed is the mocked element).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
52-54:⚠️ Potential issue | 🟡 MinorRemove the stale “toggle the extension off and on” troubleshooting step.
This PR removes the old per-site/off controls, so this instruction now points users to an action they can no longer take. Replacing it with a refresh/reload step would keep the docs accurate.
📝 Suggested doc update
**Content not hiding?** -- Try toggling the extension off and on for that site +- Refresh the page, then switch between **Focused** and **Unfocused** once +- If that still fails, reload the extension from the browser's extensions page - The site may have updated its layout — please open an issue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 52 - 54, Update the "Content not hiding?" section in README.md by removing the obsolete bullet "- Try toggling the extension off and on for that site" and replace it with a current troubleshooting step such as "Try refreshing/reloading the page or clearing the site cache" under the same "**Content not hiding?**" heading so the guidance matches the removed per-site/off controls.
🧹 Nitpick comments (2)
src/ts/background.ts (1)
37-37: Fire-and-forget initialization may cause race conditions.Using
void initializeAppState()means the promise is not awaited. WhileaddListeners()on line 160 is synchronous, if a tab event fires immediately after the service worker starts, the storage write may not have completed yet.In practice, this is likely a minor concern since:
- The current listeners don't read
appStatefrom storage directly in this file.- Storage operations are typically fast.
However, if other parts of the codebase read
appStatefrom storage during early tab events, they may see stale or missing data. Consider whether downstream code (e.g., content scripts reading storage) could be affected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ts/background.ts` at line 37, The current fire-and-forget call to initializeAppState() can cause a race where storage writes haven't completed before early tab events; change the startup to await initializeAppState() (or chain .then()) before calling addListeners(), or otherwise ensure listeners that can trigger storage reads are registered only after initializeAppState() completes; update the startup sequence around initializeAppState() and addListeners() so initializeAppState()'s promise is resolved before listeners may observe appState.src/popup/popup.css (1)
94-110: Increase the Settings button hit area a bit.With
padding: 0and 13px text, the popup’s only path to the options page ends up with a pretty small click target. A little vertical padding or a min-height would improve usability without changing the layout much.🎯 Small target-size tweak
.options-button { - padding: 0; + padding: 6px 0; + min-height: 24px; border: 0; background: transparent; color: var(--accent); font: inherit;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/popup/popup.css` around lines 94 - 110, The .options-button has padding: 0 making its hit area too small; update the CSS for .options-button (and adjust .actions if needed) to increase the clickable area by adding modest vertical padding (e.g., padding-top and padding-bottom) or a min-height while preserving layout (keep background: transparent and text styles), ensuring the button still aligns right within .actions; target .options-button and verify it doesn’t break surrounding flex alignment in .actions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@README.md`:
- Around line 52-54: Update the "Content not hiding?" section in README.md by
removing the obsolete bullet "- Try toggling the extension off and on for that
site" and replace it with a current troubleshooting step such as "Try
refreshing/reloading the page or clearing the site cache" under the same
"**Content not hiding?**" heading so the guidance matches the removed
per-site/off controls.
---
Nitpick comments:
In `@src/popup/popup.css`:
- Around line 94-110: The .options-button has padding: 0 making its hit area too
small; update the CSS for .options-button (and adjust .actions if needed) to
increase the clickable area by adding modest vertical padding (e.g., padding-top
and padding-bottom) or a min-height while preserving layout (keep background:
transparent and text styles), ensuring the button still aligns right within
.actions; target .options-button and verify it doesn’t break surrounding flex
alignment in .actions.
In `@src/ts/background.ts`:
- Line 37: The current fire-and-forget call to initializeAppState() can cause a
race where storage writes haven't completed before early tab events; change the
startup to await initializeAppState() (or chain .then()) before calling
addListeners(), or otherwise ensure listeners that can trigger storage reads are
registered only after initializeAppState() completes; update the startup
sequence around initializeAppState() and addListeners() so
initializeAppState()'s promise is resolved before listeners may observe
appState.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5327d3d6-aa95-4860-9e5a-3b27e459aeaf
📒 Files selected for processing (5)
README.mdreports/selector-health/selector-health-report.jsonsrc/popup/popup.csssrc/ts/background.tssrc/ts/websites/x/x-controller.test.ts
✅ Files skipped from review due to trivial changes (1)
- reports/selector-health/selector-health-report.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ts/websites/x/x-controller.test.ts
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.
This simplifies the extension into a more opinionated two-state experience and removes most popup customization and storage plumbing. It replaces the popup with a minimal status surface, adds an MV3 options page entry point, and updates the build and test setup to bundle that page. It removes per-site toggles, custom focus mode, quote display and text-size settings, and the old website config code while keeping the storage-based focus state flow intact. It also makes LinkedIn and X keep side distractions hidden in unfocused mode by default, including the LinkedIn layout-preserving panel concealment fix and regression tests.
Summary by CodeRabbit
New Features
Changes
Chores
Tests