fix(shadcn): UI improvements and favicon service refactoring

[amitsingh-007]

Greptile Summary

Refactored favicon service to be configurable instead of hardcoded, allowing the extension to use Google's favicon API and the web app to use Yandex's API. Added UI improvements including updated button styling, text selection colors using CSS Color Level 5 syntax, and removed the Chrome extension download button from the header. Simplified the AuthProvider by removing the async initialization wrapper and directly calling onAuthStateChange. Consolidated test utilities into a shared test-helpers.ts file and improved test fixtures for better isolation.

Confidence Score: 4/5

• This PR is mostly safe to merge with minor concerns around error handling and browser compatibility
• The refactoring is well-structured with the favicon service properly threaded through components. However, there are existing issues from previous threads (error handling in sign-out flow, button state management) that remain unaddressed. The new CSS Color Level 5 syntax may have limited browser support. The core favicon refactoring is solid.
• Pay attention to apps/web/src/app/web-ext/page.tsx for the error handling and button state issues mentioned in previous reviews, and packages/ui/src/styles/globals.css for browser compatibility of the rgba(from ...) syntax

_{Last reviewed commit: 5d348f9}

[coderabbitai]

📝 Walkthrough

Walkthrough

Renames favicon helper from getFaviconProxyUrl to getGoogleFaviconUrl, adds getYandexFaviconUrl, and exposes favicon resolution via a new getFaviconUrl prop on Bookmark/Favicon/BookmarksList/Persons; updates call sites. Removes turbopack SVG rule and the @svgr/webpack devDependency. Moves global ::selection styling into packages/ui. Refactors AuthProvider to use onAuthStateChange with unsubscribe. Simplifies AppHeader markup and adjusts PageHeader button weight. Adds isSigningOut UI state on web-ext page. Introduces shared Playwright test helpers and refactors many tests, fixtures, and utilities.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The PR description is missing. The author provided only a Greptile AI summary with confidence score, but no actual pull request description following the required template.	Add a proper pull request description that addresses the template requirement: explicitly state whether the extension requires a version change (checkbox). The provided Greptile comment is not a substitute for a human-authored PR description.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main changes: shadcn-related UI improvements and favicon service refactoring, accurately reflecting the primary objectives of this PR.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch f-shadcn-fixes-and-improvements

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

apps/extension/src/entrypoints/popup/panels/BookmarksPanel/components/BookmarkRow.tsx (1)

15-18: Consider wrapping onOpenLink in useCallback to preserve referential stability.

onOpenLink is recreated on every BookmarkRow render. Since Bookmark is also memo'd, each new reference defeats memoization and causes an unnecessary re-render of Bookmark. startHistoryMonitor from Zustand is a stable selector, so the dependency array is effectively static.

♻️ Proposed refactor

-import { memo } from 'react';
+import { memo, useCallback } from 'react';

 const BookmarkRow = memo<Omit<BookmarkProps, 'onOpenLink' | 'getFaviconUrl'>>(
   (props) => {
     const startHistoryMonitor = useHistoryStore(
       (state) => state.startHistoryMonitor
     );

-    const onOpenLink = (url: string) => {
+    const onOpenLink = useCallback((url: string) => {
       startHistoryMonitor();
       browser.tabs.create({ url, active: false });
-    };
+    }, [startHistoryMonitor]);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@apps/extension/src/entrypoints/popup/panels/BookmarksPanel/components/BookmarkRow.tsx`
around lines 15 - 18, onOpenLink in BookmarkRow is recreated on every render
which breaks memoization of the memo'd Bookmark component; wrap onOpenLink in
React.useCallback to preserve referential stability (e.g., memoize the handler
that calls startHistoryMonitor() and browser.tabs.create({...})) and list
appropriate dependencies (startHistoryMonitor is a stable selector so the
dependency array can be empty or include only truly dynamic values), ensuring
the memo'd Bookmark benefits from a stable prop reference.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@packages/shared/src/components/Bookmarks/components/Favicon.tsx`:
- Around line 30-36: The async initFavicon inside the useEffect can throw
unhandled rejections because getFaviconUrl(url) uses new URL(...) and will throw
for empty/malformed URLs; wrap the logic in initFavicon with a try/catch,
validate that url is non-empty (or guard with a safe URL parse) before calling
getFaviconUrl and getBlobUrl, and on error catch and handle it (e.g., call
setFaviconUrl(null) and/or log the error) so the promise rejection is handled;
update the initFavicon function referenced in the useEffect (initFavicon,
getFaviconUrl, getBlobUrl, setFaviconUrl) accordingly.

---

Nitpick comments:
In
`@apps/extension/src/entrypoints/popup/panels/BookmarksPanel/components/BookmarkRow.tsx`:
- Around line 15-18: onOpenLink in BookmarkRow is recreated on every render
which breaks memoization of the memo'd Bookmark component; wrap onOpenLink in
React.useCallback to preserve referential stability (e.g., memoize the handler
that calls startHistoryMonitor() and browser.tabs.create({...})) and list
appropriate dependencies (startHistoryMonitor is a stable selector so the
dependency array can be empty or include only truly dynamic values), ensuring
the memo'd Bookmark benefits from a stable prop reference.

[amitsingh-007]

/gemini review

[coderabbitai]

♻️ Duplicate comments (1)

apps/web/src/app/provider/AuthProvider.tsx (1)

36-49: Previous auth-listener churn issue resolved — LGTM.

isRestrictedPath as the dep correctly scopes listener lifecycle to restricted ↔ non-restricted transitions, and return unsubscribe is now properly wired. The const (vs. useMemo) is fine here since RESTRICTED_PATHS.has(pathname) returns a primitive boolean that the deps array compares by value.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/app/provider/AuthProvider.tsx` around lines 36 - 49, No code
change required: the useEffect correctly scopes the auth listener lifecycle
using the boolean isRestrictedPath (computed from
RESTRICTED_PATHS.has(pathname)), returns the unsubscribe function, and updates
state via onAuthStateChange -> setUser and setIsInitialized; keep the current
const isRestrictedPath and dependency [isRestrictedPath] so the listener is only
created/removed on restricted ↔ non-restricted transitions.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@apps/web/src/app/provider/AuthProvider.tsx`:
- Around line 36-49: No code change required: the useEffect correctly scopes the
auth listener lifecycle using the boolean isRestrictedPath (computed from
RESTRICTED_PATHS.has(pathname)), returns the unsubscribe function, and updates
state via onAuthStateChange -> setUser and setIsInitialized; keep the current
const isRestrictedPath and dependency [isRestrictedPath] so the listener is only
created/removed on restricted ↔ non-restricted transitions.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 2813db5 and 425d8d9.

📒 Files selected for processing (1)

• apps/web/src/app/provider/AuthProvider.tsx

[amitsingh-007]

/gemini review

[webext-bot]

Extension version is updated from 24.0.0 to 24.1.0

[greptile-apps]

_{48 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}