feat: Show favicons as Bookmarks icons #3079
Conversation
We replaced the default bookmarks plugin icon with Google's favicon API to fetch and display favicons for each bookmark from their corresponding websites.
z1nc0r3
added
enhancement
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs(2 hunks)
🔇 Additional comments (2)
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Main.cs (2)
93-93: Same favicon handling concerns as above
This instance has the same issues as the previous occurrence.
Line range hint 71-93: Verify performance impact with large bookmark collections
The current implementation makes an external API call for each bookmark displayed. This could impact performance, especially when displaying many bookmarks.
Let's analyze the potential performance impact:
Consider implementing:
- Batch loading of favicons during bookmark initialization
- Rate limiting for favicon requests
- Local caching with periodic updates
Would you like assistance with implementing any of these improvements?
| @@ -68,7 +68,7 @@ public List<Result> Query(Query query) | |||
| { | |||
| Title = c.Name, | |||
| SubTitle = c.Url, | |||
| IcoPath = @"Images\bookmark.png", | |||
| IcoPath = $"https://www.google.com/s2/favicons?domain={c.Url}&sz=64", | |||
There was a problem hiding this comment.
Consider adding error handling and caching for favicon URLs
The direct use of bookmark URLs in the favicon API call has several potential issues:
- No fallback mechanism when the favicon API fails
- Potential security concerns with unencoded URLs
- Performance impact from making external API calls for each bookmark
Consider implementing these improvements:
-IcoPath = $"https://www.google.com/s2/favicons?domain={c.Url}&sz=64",
+IcoPath = GetFaviconPath(c.Url),
+private string GetFaviconPath(string url)
+{
+ try
+ {
+ var uri = new Uri(url);
+ var encodedDomain = Uri.EscapeDataString(uri.Host);
+ return $"https://www.google.com/s2/favicons?domain={encodedDomain}&sz=64";
+ }
+ catch (Exception ex)
+ {
+ Log.Exception("Main", "Failed to generate favicon URL", ex);
+ return @"Images\bookmark.png"; // Fallback to default icon
+ }
+}Consider implementing a caching mechanism for favicons to:
- Reduce external API calls
- Handle offline scenarios
- Improve performance
Would you like me to provide a caching implementation example?
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@z1nc0r3 I think this is worth addressing. Use the existing image as a fallback.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
And I also think these calls to grab icons might impact performance, if there are a lot of bookmarks.
@coderabbitai provide a caching implementation example please.
@z1nc0r3 please test performance.
There was a problem hiding this comment.
@jjw24, certainly! Here's an example of how you can implement caching for the favicons to improve performance:
First, add a dictionary to act as an in-memory cache:
private static Dictionary<string, string> faviconCache = new Dictionary<string, string>();Next, update the GetFaviconPath method to utilize the cache:
private string GetFaviconPath(string url)
{
try
{
var uri = new Uri(url);
var domain = uri.Host;
if (faviconCache.TryGetValue(domain, out var cachedFaviconUrl))
{
// Return the cached favicon URL if it exists
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";
}
}Then, update the IcoPath assignment in your Result objects to use this method:
IcoPath = GetFaviconPath(c.Url),This implementation:
- Caches favicon URLs based on the domain to avoid redundant API calls.
- Reduces external requests, which improves performance, especially with a large number of bookmarks.
- Handles exceptions and provides a fallback to the default icon if needed.
Please remember to test the performance with your set of bookmarks to ensure the caching mechanism effectively improves load times.
Let me know if you have any questions or need further assistance!
There was a problem hiding this comment.
Yeah sure @jjw24. I'll try these changes
|
🥷 Code experts: Yusyuriv, jjw24 Yusyuriv, jjw24 have most 🧠 knowledge in the files. See details
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
What's in the PR:
There was an issue asking for this change but couldn't find it :\
I replaced the default bookmarks plugin icon with Google's favicon API to fetch and display favicons for each bookmark from their corresponding websites.
Known issue: