feat: favicon from logo-icon.svg — all required sizes, ICO + PNG (Closes #471)#673
feat: favicon from logo-icon.svg — all required sizes, ICO + PNG (Closes #471)#673LaphoqueRC wants to merge 1 commit intoSolFoundry:mainfrom
Conversation
Generated directly from assets/logo-icon.svg using rsvg-convert + ImageMagick. 4 binary files committed (under the 5-file limit): - favicon.ico (multi-size: 16x16 + 32x32 embedded) - apple-touch-icon.png (180x180) - favicon-192x192.png (192x192) - favicon-512x512.png (512x512) Also includes: - site.webmanifest with 'purpose' metadata (any + maskable) on both icon sizes - index.html <head> updated with all favicon link tags + theme-color meta - scripts/generate-favicons.sh — regeneration script, supports ImageMagick 6 (convert) and 7 (magick) - scripts/test-favicons.sh — 28 assertions covering files, sizes, HTML refs, manifest content, script quality Wallet: HZV6YPdTeJPjPujWjzsFLLKja91K2Ze78XeY8MeFhfK8
📝 WalkthroughWalkthroughThis pull request adds favicon asset support and web app manifest configuration to the repository. A new Web App Manifest JSON file defines the application's name, description, start URL, display mode, colors, and icon references. The index.html head is updated with favicon link tags (ICO, multiple PNG sizes, Apple touch icon, and web manifest) pointing to the new favicon assets. Two new Bash scripts are added: one to generate the favicon PNG and ICO files from an existing SVG logo source using rsvg-convert and ImageMagick, and another to validate the presence and structure of all favicon assets and HTML/manifest references. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/favicons/site.webmanifest`:
- Around line 16-21: site.webmanifest currently reuses the same PNG for both
"purpose": "any" and "purpose": "maskable" which risks background clipping on
masked displays; regenerate or produce maskable-specific PNGs from
assets/logo-icon.svg that either remove or extend/flatten the background to
accommodate circular/masked crops, update the manifest entries for the maskable
sizes (the entries with "purpose": "maskable") to point to those new files, and
verify the result using a preview tool like Maskable.app to ensure no important
background gradients or edges are lost.
In `@index.html`:
- Around line 9-10: The two link elements that declare type="image/png" but
reference /assets/favicons/favicon.ico are a MIME mismatch; fix by either
changing the type attribute on those link tags to image/x-icon to match the .ico
file (update the link elements with href="/assets/favicons/favicon.ico"
accordingly) or (if you want separate PNG assets) update the favicon generation
script to preserve/output favicon-16x16.png and favicon-32x32.png and change the
hrefs to those PNG filenames while keeping type="image/png"; remove
duplicate/incorrect tags if you choose to rely solely on the ICO.
In `@scripts/generate-favicons.sh`:
- Around line 39-40: The script currently writes to predictable temp paths
(/tmp/_favicon-16.png, /tmp/_favicon-32.png and similarly at the later
rsvg-convert calls), which is vulnerable to symlink/race attacks; change the
logic so you create secure temporary files with mktemp (e.g. TMP_16=$(mktemp)
and TMP_32=$(mktemp)), use those variables in the rsvg-convert invocations
(references: the rsvg-convert commands and the /tmp/_favicon-*.png names), and
ensure you register a trap to remove the temp files on exit to avoid leaks.
In `@scripts/test-favicons.sh`:
- Around line 58-61: Remove the conditional guards and make the size assertions
unconditional: call assert_min_size for each favicon path (favicon.ico,
apple-touch-icon.png, favicon-192x192.png, favicon-512x512.png) without the
leading [[ -f ... ]] &&. Update the assert_min_size function to detect a missing
file (using the same existence logic as assert_file) and fail the test with a
clear message when the file does not exist, and keep its existing size check
behavior otherwise; reference assert_min_size and the FAVICONS variable to
locate the changes and remove the guarded calls currently surrounding those four
filename checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b0c6f12-ccfe-46a0-8656-73ecb79c3334
⛔ Files ignored due to path filters (4)
assets/favicons/apple-touch-icon.pngis excluded by!**/*.pngassets/favicons/favicon-192x192.pngis excluded by!**/*.pngassets/favicons/favicon-512x512.pngis excluded by!**/*.pngassets/favicons/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (4)
assets/favicons/site.webmanifestindex.htmlscripts/generate-favicons.shscripts/test-favicons.sh
| { | ||
| "src": "/assets/favicons/favicon-192x192.png", | ||
| "sizes": "192x192", | ||
| "type": "image/png", | ||
| "purpose": "maskable" | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if logo-icon.svg exists and inspect its viewBox to assess padding
if [[ -f "assets/logo-icon.svg" ]]; then
echo "=== SVG viewBox and dimensions ==="
grep -oP '(viewBox|width|height)="[^"]*"' assets/logo-icon.svg | head -5
echo ""
echo "=== Manual check needed: verify icon content has ~10% padding for maskable safe zone ==="
else
echo "logo-icon.svg not found at expected path"
fiRepository: SolFoundry/solfoundry
Length of output: 261
🏁 Script executed:
#!/bin/bash
# Examine the SVG content to assess padding and visual element placement
echo "=== SVG Content (first 50 lines) ==="
head -50 assets/logo-icon.svg
echo ""
echo "=== Checking favicon files referenced in manifest ==="
ls -la assets/favicons/ | grep -E "(png|svg)"
echo ""
echo "=== Checking webmanifest for icon paths ==="
grep -A 3 '"src"' assets/favicons/site.webmanifestRepository: SolFoundry/solfoundry
Length of output: 3092
Maskable icons may have suboptimal background clipping—verify PNG generation handles this properly.
The same PNG files are used for both purpose: "any" and purpose: "maskable" (lines 16-21 and 28-33). The source SVG (assets/logo-icon.svg) shows the background gradient extends to the full 512×512 canvas edges, which will be clipped when Android masks the icon to circular or other shapes.
The main anvil/hammer icon content is centered and has adequate padding, but the background treatment may result in visual issues on devices that apply masking. Verify that:
- The PNG generation process accounts for maskable clipping, or
- Create separate maskable-specific variants that either remove the background or ensure it handles masking gracefully
- Use a tool like Maskable.app to preview the actual display
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/favicons/site.webmanifest` around lines 16 - 21, site.webmanifest
currently reuses the same PNG for both "purpose": "any" and "purpose":
"maskable" which risks background clipping on masked displays; regenerate or
produce maskable-specific PNGs from assets/logo-icon.svg that either remove or
extend/flatten the background to accommodate circular/masked crops, update the
manifest entries for the maskable sizes (the entries with "purpose": "maskable")
to point to those new files, and verify the result using a preview tool like
Maskable.app to ensure no important background gradients or edges are lost.
| <link rel="icon" type="image/png" sizes="32x32" href="/assets/favicons/favicon.ico"> | ||
| <link rel="icon" type="image/png" sizes="16x16" href="/assets/favicons/favicon.ico"> |
There was a problem hiding this comment.
MIME type mismatch: type="image/png" but href points to .ico file.
Lines 9-10 declare type="image/png" but reference favicon.ico, which is an ICO file (image/x-icon). This semantic mismatch may cause browsers to incorrectly interpret the resource or fail to use it.
The generation script (generate-favicons.sh) creates temporary 16×16 and 32×32 PNGs only to embed them into the ICO, then deletes them. If you want separate PNG link tags for these sizes, the script should preserve those files, or these lines should be removed/corrected to avoid the type mismatch.
Options:
- Remove lines 9-10 entirely (rely on the ICO which already embeds both sizes)
- Change
typetoimage/x-iconto match the actual file type - Update the generation script to output separate
favicon-16x16.pngandfavicon-32x32.pngfiles and reference those here
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 9 - 10, The two link elements that declare
type="image/png" but reference /assets/favicons/favicon.ico are a MIME mismatch;
fix by either changing the type attribute on those link tags to image/x-icon to
match the .ico file (update the link elements with
href="/assets/favicons/favicon.ico" accordingly) or (if you want separate PNG
assets) update the favicon generation script to preserve/output
favicon-16x16.png and favicon-32x32.png and change the hrefs to those PNG
filenames while keeping type="image/png"; remove duplicate/incorrect tags if you
choose to rely solely on the ICO.
| rsvg-convert -w 16 -h 16 "$SOURCE_SVG" -o /tmp/_favicon-16.png | ||
| rsvg-convert -w 32 -h 32 "$SOURCE_SVG" -o /tmp/_favicon-32.png |
There was a problem hiding this comment.
Insecure use of predictable temp file paths in shared /tmp directory.
Using hardcoded paths like /tmp/_favicon-16.png and /tmp/_favicon-32.png in a shared directory creates a symlink race condition vulnerability. In multi-user environments, an attacker could pre-create a symlink at these paths pointing to a sensitive file, causing the script to overwrite it when run by another user.
Use mktemp to create secure temporary files:
🔒 Proposed fix using mktemp
+# Create secure temp directory
+TMPDIR=$(mktemp -d)
+trap 'rm -rf "$TMPDIR"' EXIT
+
# ── PNG sizes ────────────────────────────────────────────────────────────────
-rsvg-convert -w 16 -h 16 "$SOURCE_SVG" -o /tmp/_favicon-16.png
-rsvg-convert -w 32 -h 32 "$SOURCE_SVG" -o /tmp/_favicon-32.png
+rsvg-convert -w 16 -h 16 "$SOURCE_SVG" -o "$TMPDIR/favicon-16.png"
+rsvg-convert -w 32 -h 32 "$SOURCE_SVG" -o "$TMPDIR/favicon-32.png"
rsvg-convert -w 180 -h 180 "$SOURCE_SVG" -o "$OUT_DIR/apple-touch-icon.png"
rsvg-convert -w 192 -h 192 "$SOURCE_SVG" -o "$OUT_DIR/favicon-192x192.png"
rsvg-convert -w 512 -h 512 "$SOURCE_SVG" -o "$OUT_DIR/favicon-512x512.png" # ── Multi-size ICO (embeds 16x16 + 32x32) ────────────────────────────────────
-"$IMAGEMAGICK_CMD" /tmp/_favicon-16.png /tmp/_favicon-32.png "$OUT_DIR/favicon.ico"
-rm -f /tmp/_favicon-16.png /tmp/_favicon-32.png
+"$IMAGEMAGICK_CMD" "$TMPDIR/favicon-16.png" "$TMPDIR/favicon-32.png" "$OUT_DIR/favicon.ico"
+# Cleanup handled by trapAlso applies to: 51-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate-favicons.sh` around lines 39 - 40, The script currently
writes to predictable temp paths (/tmp/_favicon-16.png, /tmp/_favicon-32.png and
similarly at the later rsvg-convert calls), which is vulnerable to symlink/race
attacks; change the logic so you create secure temporary files with mktemp (e.g.
TMP_16=$(mktemp) and TMP_32=$(mktemp)), use those variables in the rsvg-convert
invocations (references: the rsvg-convert commands and the /tmp/_favicon-*.png
names), and ensure you register a trap to remove the temp files on exit to avoid
leaks.
| [[ -f "$FAVICONS/favicon.ico" ]] && assert_min_size "$FAVICONS/favicon.ico" 1024 "favicon.ico is non-trivial (≥1KB)" | ||
| [[ -f "$FAVICONS/apple-touch-icon.png" ]] && assert_min_size "$FAVICONS/apple-touch-icon.png" 5000 "apple-touch-icon.png is non-trivial (≥5KB)" | ||
| [[ -f "$FAVICONS/favicon-192x192.png" ]] && assert_min_size "$FAVICONS/favicon-192x192.png" 5000 "favicon-192x192.png is non-trivial (≥5KB)" | ||
| [[ -f "$FAVICONS/favicon-512x512.png" ]] && assert_min_size "$FAVICONS/favicon-512x512.png" 20000 "favicon-512x512.png is non-trivial (≥20KB)" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider unconditional size assertions for clearer test failure reporting.
The pattern [[ -f "$file" ]] && assert_min_size ... silently skips size checks if files are missing. While assert_file above catches missing files, if a file is deleted between checks (unlikely but possible), or if someone reorders tests, the size check would be silently skipped without incrementing FAIL.
For more explicit test behavior, consider always calling assert_min_size and having it handle the missing file case internally:
♻️ Optional: Handle missing files in assert_min_size
assert_min_size() {
local size
+ if [[ ! -f "$1" ]]; then
+ echo " ✗ $3 — file not found: $1"
+ ((FAIL++)) || true
+ return
+ fi
size=$(wc -c < "$1" 2>/dev/null || echo 0)Then remove the [[ -f ... ]] && guards on lines 58-61.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test-favicons.sh` around lines 58 - 61, Remove the conditional guards
and make the size assertions unconditional: call assert_min_size for each
favicon path (favicon.ico, apple-touch-icon.png, favicon-192x192.png,
favicon-512x512.png) without the leading [[ -f ... ]] &&. Update the
assert_min_size function to detect a missing file (using the same existence
logic as assert_file) and fail the test with a clear message when the file does
not exist, and keep its existing size check behavior otherwise; reference
assert_min_size and the FAVICONS variable to locate the changes and remove the
guarded calls currently surrounding those four filename checks.
1 participant
Changes
Generates proper favicons from
assets/logo-icon.svgin all required sizes and formats, with all assets committed for immediate browser use.What's included
assets/favicons/favicon.icoassets/favicons/apple-touch-icon.pngassets/favicons/favicon-192x192.pngassets/favicons/favicon-512x512.pngassets/favicons/site.webmanifestpurpose: any + maskableon both icon entriesindex.html<head>updated with all favicon link tags +theme-colormetascripts/generate-favicons.shscripts/test-favicons.shAcceptance Criteria
.icoand.pngformatssite.webmanifestupdated with icon references +purposemetadata<head>updated with all favicon<link>tags +theme-colorBinary Count
4 binary files committed (favicon.ico + 3 PNGs). Well under the 5-file auto-rejection threshold.
Wallet:
HZV6YPdTeJPjPujWjzsFLLKja91K2Ze78XeY8MeFhfK8