Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented suggested changes for PR#3079 #3360

Closed
wants to merge 1 commit into from

Conversation

ZainGill45
Copy link

This PR is based on #3079 Show favicons as Bookmarks icons, originally by @z1nc0r3. Since it seems inactive, I’ve picked it up please let me know if anything is wrong with it or if additional changes need to be made.

This PR is based on Flow-Launcher#3079 Show favicons as Bookmarks icons, originally by @z1nc0r3. Since it seems inactive, I’ve picked it up please let me know if anything is wrong with it or if additional changes need to be made.
Copy link

@check-spelling-bot Report

🔴 Please review

See the 📂 files view, the 📜action log, or 📝 job summary for details.

❌ Errors Count
❌ forbidden-pattern 22
⚠️ ignored-expect-variant 1
⚠️ non-alpha-in-dictionary 19

See ❌ Event descriptions for more information.

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

Copy link
Contributor

coderabbitai bot commented Mar 19, 2025

📝 Walkthrough

Walkthrough

The changes update the BrowserBookmark plugin by introducing a static dictionary to cache favicon URLs based on domain names. The Query method now calls a new method GetFaviconPath which extracts the domain from a URL and either retrieves a cached favicon URL or constructs a new one using Google's favicon service. Error handling is added to log issues during URL processing and fall back to a default icon if needed.

Changes

File Path Change Summary
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs - Added a static dictionary faviconCache to store favicon URLs by domain.
- Introduced GetFaviconPath to generate and cache favicons based on the bookmark's URL.
- Modified the Query method to use the new method and added error handling.

Sequence Diagram(s)

sequenceDiagram
    participant Q as Query Method
    participant G as GetFaviconPath Method
    participant C as faviconCache
    participant S as Google Favicon Service

    Q->>G: Call GetFaviconPath(url)
    G->>G: Extract domain from URL
    alt Domain found in cache
        G->>C: Retrieve cached favicon URL
        C-->>G: Return favicon URL
    else Domain not in cache
        G->>S: Construct new favicon URL using Google's service
        S-->>G: Return new favicon URL
        G->>C: Cache the new favicon URL
    end
    G-->>Q: Return favicon URL (or default on error)
Loading

Suggested labels

enhancement

Suggested reviewers

  • VictoriousRaptor

Poem

I'm a rabbit with a cache so bright,
Hopping through domains with sheer delight.
Favicon paths now quick and neat,
Error logs guard every little beat.
My code hops forward, swift and free—
A techy tale of efficiency! 🐰🥕

Tip

⚡🧪 Multi-step agentic review comment chat (experimental)
  • We're introducing multi-step agentic chat in review comments. This experimental feature enhances review discussions with the CodeRabbit agentic chat by enabling advanced interactions, including the ability to create pull requests directly from comments.
    - To enable this feature, set early_access to true under in the settings.
✨ Finishing Touches
  • 📝 Generate Docstrings

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (1)

109-137: The GetFaviconPath implementation is well structured but could be more robust.

The method effectively extracts domains, caches results, and has good error handling. There are a few potential improvements to consider:

  1. The method doesn't handle URLs without protocols (e.g., "example.com" instead of "https://example.com")
  2. Consider using Uri.TryCreate instead of a try-catch for better performance when parsing malformed URLs
  3. The cache has no size limit or expiration policy, which could potentially use excess memory over time
private string GetFaviconPath(string url)
{
    try
    {
+       // Add protocol if missing
+       if (!url.StartsWith("http://", StringComparison.OrdinalIgnoreCase) && 
+           !url.StartsWith("https://", StringComparison.OrdinalIgnoreCase))
+       {
+           url = "https://" + url;
+       }
+
+       // Use TryCreate instead of throwing exceptions
+       if (!Uri.TryCreate(url, UriKind.Absolute, out var uri))
+       {
+           return @"Images\bookmark.png";
+       }
-       var uri = new Uri(url);
        var domain = uri.Host;

        if (faviconCache.TryGetValue(domain, out var cachedFaviconUrl))
        {
            return cachedFaviconUrl;
        }
        else
        {
            var encodedDomain = Uri.EscapeDataString(domain);
            var faviconUrl = $"https://www.google.com/s2/favicons?domain={encodedDomain}&sz=64";

            // Store the favicon URL in the cache
            faviconCache[domain] = faviconUrl;
            return faviconUrl;
        }
    }
    catch (Exception ex)
    {
        Log.Exception("Main", "Failed to generate favicon URL", ex);

        // Fallback to default icon
        return @"Images\bookmark.png";
    }
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e28a69c and 2148f5a.

📒 Files selected for processing (1)
  • Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Report (PR)
🔇 Additional comments (3)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (3)

22-22: Good addition of a favicon cache for performance optimization.

Adding a cache for favicon URLs will prevent repeated generation of the same URLs for frequently accessed domains, improving performance.


73-73: Excellent enhancement replacing hardcoded icons with dynamic favicons.

Using the domain's favicon instead of a generic bookmark icon provides a better visual experience for users, making it easier to identify different bookmarks at a glance.

Also applies to: 95-95


122-123:

❓ Verification inconclusive

Consider alternatives to Google's favicon service for better reliability.

While Google's favicon service works well, it's an external dependency that could change or be unavailable. You might want to consider:

  1. Adding a local caching mechanism for actual favicon images
  2. Having multiple fallback services if one fails
  3. Implementing your own favicon retrieval directly from the websites

🌐 Web query:

What are the alternatives to Google's favicon service?

💡 Result:

Here are the top alternatives to Google's Favicon API for retrieving website icons in 2025:

Leading API-Based Solutions

1. FaviconExtractor

  • Supports sizes from 16x16 to 512x512, including SVG generation
  • Uses DuckDuckGo as a fallback source for reliability
  • Simple integration with HTML snippets or API calls:
    <img src="https://faviconextractor.com/api/favicon?url=example.com&size=64">
    [1][3][10]

2. Grabicon

  • Optimized for speed and customization
  • Retrieves high-quality icons via GET requests:
    curl https://grabicon.com/icon?url=example.com&size=128
    [1][3]

3. Statvoo's Favicon API

  • Offers multiple size parameters (up to 128x128)
  • Robust fallback mechanisms and detailed documentation [1][10]

DuckDuckGo/Yandex Direct Links
For quick implementation without API keys:

<!-- DuckDuckGo -->
<img src="https://icons.duckduckgo.com/ip3/example.com.ico">

<!-- Yandex (supports multiple domains) -->
<img src="https://favicon.yandex.net/favicon/google.com/example.com">

[3][7][12]

Specialized Tools

Icon Horse

  • Combines automatic retrieval with premium features like:
    • Fallback icon generation
    • Anti-hotlinking protection
    • Usage analytics
      [3][4]

Favicone

  • JSON metadata support and smart icon detection
  • Checks HTML, manifest files, and traditional .ico locations:
    curl https://favicone.com/example.com?json
    [6]

Self-Hosted Options

favicon-proxy (GitHub)

  • Solves CORS issues for browser-based apps
  • Deployable via Heroku or Docker:
    fetch('https://your-proxy-domain/example.com')
    [12]
Feature FaviconExtractor Grabicon Icon Horse DuckDuckGo
Max Resolution 512px 256px 512px 32px
Custom Size Support Yes Yes Yes No
Fallback System Multi-source Basic Advanced None
SVG Generation Yes No Premium No
Cost Free/Open-Source Freemium Premium Free

For most projects needing reliability and customization, FaviconExtractor and Icon Horse provide the best balance of features. Developers requiring simple integration often prefer DuckDuckGo's direct URLs, while enterprise applications benefit from Statvoo's API capabilities. Self-hosted solutions like favicon-proxy excel in environments with strict CORS policies[1][3][6][12].

Citations:


Action: Evaluate and implement a more robust favicon retrieval strategy

While your current implementation using Google’s favicon service works, relying solely on it introduces an external dependency that may become unreliable over time. Consider the following enhancements:

  • Local Caching: Implement a caching mechanism to store fetched favicons locally, reducing repeated external calls.
  • Fallback Services: Integrate additional sources such as:
    • FaviconExtractor – supports multiple sizes (up to 512px) and offers fallback via DuckDuckGo.
    • Grabicon – optimizes speed and customization.
    • DuckDuckGo Direct Endpoints – simple URL-based retrieval.
    • Self-Hosted Options: For example, using a favicon-proxy to handle CORS and service stability.
  • Direct Retrieval: Optionally, implement logic to extract favicons directly from websites, bypassing external APIs altogether.

These adjustments can improve reliability and reduce the risk of service disruption.

else
{
var encodedDomain = Uri.EscapeDataString(domain);
var faviconUrl = $"https://www.google.com/s2/favicons?domain={encodedDomain}&sz=64";
Copy link
Contributor

Choose a reason for hiding this comment

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

Since quite a lot of our users are in China, where Google is banned, do we have another option for favicon api? I did a bit research and can't find a reliable one. So what about making it customizable or a toggle?

Copy link
Contributor

Choose a reason for hiding this comment

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

@VictoriousRaptor I'm trying to find a way to retrieve bookmark data from local storage.
Even though I don’t know much about programming, I can tell that this approach shouldn’t be used.

Copy link
Author

Choose a reason for hiding this comment

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

@onesounds If your moving forward with PR #3361 would you like me to close this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ZainGill45 I'll close it. Thanks for the PR.

@onesounds onesounds closed this Mar 20, 2025
@onesounds
Copy link
Contributor

Continuing in #3361

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