Skip to content

Fix favicon fetching to check all icon links instead of just the first one#6880

Open
Shocker wants to merge 4 commits intodani-garcia:mainfrom
Shocker:fix-all-icons
Open

Fix favicon fetching to check all icon links instead of just the first one#6880
Shocker wants to merge 4 commits intodani-garcia:mainfrom
Shocker:fix-all-icons

Conversation

@Shocker
Copy link
Copy Markdown

@Shocker Shocker commented Feb 27, 2026

Fix logic in download_icon(), got broken (most likely accidentally) in 27dc67f#diff-c0f730ed83eb02623c5c9bde35b0366115b8961fb596c27f98613140cba6d2a7L714 which caused the downloader to stop after first error

Also don't limit max checked icons to 5, allow all icons in the list

@BlackDex
Copy link
Copy Markdown
Collaborator

The reason we only select the first 5 items is because we do not want to try and download every icon a site has listed.
It could be 1 or 20, and calling a site 20 times might also trigger a block/ban even.

The 5 items from the list are already sorted in a most preferred item is at the top of the list.

Are there specific sites on which you experience issues?
Since, i have run a local script which compares icons between Vaultwarden, Bitwarden, Google and DuckDuckGo.
And there are just a few which do not work currently, but also a lot which Vaultwarden is able to fetch an icon where Bitwarden, Google or DuckDuckGo (Either one or more) is not.

@Shocker
Copy link
Copy Markdown
Author

Shocker commented Feb 27, 2026

Regarding the 5 limit: I don't have any specific site with issues. To be honest it was just a weird seemingly undocumented line in the code which didn't make sense to have an arbitrary limit of 5.

What about the main issue being fixed with the PR?

@BlackDex
Copy link
Copy Markdown
Collaborator

Which main issue do you refer to?
Is it that if one item fails it stop and aborts?

@Shocker
Copy link
Copy Markdown
Author

Shocker commented Feb 27, 2026

Which main issue do you refer to? Is it that if one item fails it stop and aborts?

Yes.

Comment thread src/api/icons.rs Outdated
@Shocker Shocker requested a review from BlackDex April 7, 2026 16:20
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.

2 participants