feat: favicons from logo-icon.svg in all required sizes (Closes #471)#683
feat: favicons from logo-icon.svg in all required sizes (Closes #471)#683LaphoqueRC wants to merge 1 commit intoSolFoundry:mainfrom
Conversation
- 5 PNG favicons: 16x16, 32x32, 180x180, 192x192, 512x512 - site.webmanifest with icon refs and purpose field - index.html head updated with typed link tags and theme-color - Generation script (rsvg-convert) for reproducibility - Test suite: 27 assertions covering all acceptance criteria
📝 WalkthroughWalkthroughThe pull request adds PWA and favicon support to the project. A new favicon generation Bash script converts 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/test-favicons.sh`:
- Line 5: Update the top comment in the test-favicons.sh script to correctly
state the number of assertions: change the line that currently reads "Runs 29
assertions" to "Runs 27 assertions" so the comment matches the actual count of
assert* calls (the various assert* lines in Sections 1–5 of the script).
- Around line 31-69: The assert_no_grep helper uses sh -c with embedded single
quotes which can break if the pattern ($2) contains a single quote; update the
assert_no_grep function to pass the pattern and filename as positional arguments
to sh -c instead of interpolating them, e.g. call sh -c with a command string
like '! grep -q "$1" "$2"' and supply a dummy $0 followed by the pattern and
file as arguments so grep gets safe, properly-quoted parameters; edit the
assert_no_grep function accordingly to use this argument-passing form.
- Around line 66-69: Remove the dead helper function assert_no_grep (and its
inner call to assert with sh -c "! grep -q '$2' '$3'") since it's never used; if
you actually intended negative checks, instead add explicit calls to
assert_no_grep at the appropriate places (e.g., to verify no stale references)
or inline the negative assertion using assert sh -c "! grep -q 'PATTERN' 'FILE'"
where needed—either delete the unused assert_no_grep definition or add the
specific assert_no_grep(...) calls that perform the intended negative checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4ec7971-0185-4f04-ac36-b2d69a359476
⛔ Files ignored due to path filters (5)
assets/favicons/favicon-16x16.pngis excluded by!**/*.pngassets/favicons/favicon-180x180.pngis excluded by!**/*.pngassets/favicons/favicon-192x192.pngis excluded by!**/*.pngassets/favicons/favicon-32x32.pngis excluded by!**/*.pngassets/favicons/favicon-512x512.pngis excluded by!**/*.png
📒 Files selected for processing (4)
assets/favicons/site.webmanifestindex.htmlscripts/generate-favicons.shscripts/test-favicons.sh
| # | ||
| # test-favicons.sh — Validate favicon assets, HTML integration, and manifest. | ||
| # | ||
| # Runs 29 assertions that verify every acceptance criterion from issue #471: |
There was a problem hiding this comment.
Assertion count discrepancy: comment claims 29 but script contains 27.
Line 5 states "Runs 29 assertions" but counting the actual assert* calls:
- Section 1 (PNG files): 7 assertions (lines 74-80)
- Section 2 (Manifest): 7 assertions (lines 85-91)
- Section 3 (HTML head): 8 assertions (lines 96-103)
- Section 4 (Generation script): 4 assertions (lines 108-111)
- Section 5 (Consistency): 1 assertion (line 116)
Total: 27 assertions, not 29. Update the comment to reflect the actual count.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test-favicons.sh` at line 5, Update the top comment in the
test-favicons.sh script to correctly state the number of assertions: change the
line that currently reads "Runs 29 assertions" to "Runs 27 assertions" so the
comment matches the actual count of assert* calls (the various assert* lines in
Sections 1–5 of the script).
| assert() { | ||
| # Run a command; record pass/fail. | ||
| local desc="$1"; shift | ||
| if "$@" >/dev/null 2>&1; then | ||
| printf ' ✅ %s\n' "$desc" | ||
| PASS=$((PASS + 1)) | ||
| else | ||
| printf ' ❌ %s\n' "$desc" | ||
| FAIL=$((FAIL + 1)) | ||
| fi | ||
| } | ||
|
|
||
| assert_file() { | ||
| # Verify a file exists. | ||
| assert "Exists: $1" test -f "$1" | ||
| } | ||
|
|
||
| assert_min_bytes() { | ||
| # Verify file is at least N bytes (handles missing files gracefully). | ||
| local file="$1" min="$2" label="$3" | ||
| if [[ ! -f "$file" ]]; then | ||
| printf ' ❌ %s — file not found\n' "$label" | ||
| FAIL=$((FAIL + 1)) | ||
| return | ||
| fi | ||
| local actual | ||
| actual=$(wc -c < "$file") | ||
| assert "$label (${actual}B >= ${min}B)" test "$actual" -ge "$min" | ||
| } | ||
|
|
||
| assert_grep() { | ||
| # Verify a pattern appears in a file. | ||
| assert "$1" grep -q "$2" "$3" | ||
| } | ||
|
|
||
| assert_no_grep() { | ||
| # Verify a pattern does NOT appear in a file. | ||
| assert "$1" sh -c "! grep -q '$2' '$3'" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test helper functions are well-designed with minor shell quoting concern.
The assertion helpers properly track pass/fail counts and handle missing files gracefully in assert_min_bytes.
Minor note on line 68: The pattern sh -c "! grep -q '$2' '$3'" uses single quotes inside double quotes. If $2 ever contained a single quote, it would break the command. In this controlled script context where patterns are hardcoded, this is not exploitable, but a more robust approach would use:
assert_no_grep() {
assert "$1" sh -c '! grep -q "$1" "$2"' _ "$2" "$3"
}This is a defensive improvement, not a blocking issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test-favicons.sh` around lines 31 - 69, The assert_no_grep helper
uses sh -c with embedded single quotes which can break if the pattern ($2)
contains a single quote; update the assert_no_grep function to pass the pattern
and filename as positional arguments to sh -c instead of interpolating them,
e.g. call sh -c with a command string like '! grep -q "$1" "$2"' and supply a
dummy $0 followed by the pattern and file as arguments so grep gets safe,
properly-quoted parameters; edit the assert_no_grep function accordingly to use
this argument-passing form.
| assert_no_grep() { | ||
| # Verify a pattern does NOT appear in a file. | ||
| assert "$1" sh -c "! grep -q '$2' '$3'" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Dead code: assert_no_grep is defined but never used.
The assert_no_grep helper function is implemented but has no callers in the script. Either remove it to reduce maintenance burden, or add the intended negative assertions it was designed for (e.g., verifying no stale references as mentioned in the header comment on line 10).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test-favicons.sh` around lines 66 - 69, Remove the dead helper
function assert_no_grep (and its inner call to assert with sh -c "! grep -q '$2'
'$3'") since it's never used; if you actually intended negative checks, instead
add explicit calls to assert_no_grep at the appropriate places (e.g., to verify
no stale references) or inline the negative assertion using assert sh -c "! grep
-q 'PATTERN' 'FILE'" where needed—either delete the unused assert_no_grep
definition or add the specific assert_no_grep(...) calls that perform the
intended negative checks.
1 participant
Changes
Generate favicons from
assets/logo-icon.svgin all required sizes.Assets (5 PNGs)
favicon-16x16.pngfavicon-32x32.pngfavicon-180x180.pngfavicon-192x192.pngfavicon-512x512.pngIntegration
index.html<head>— all<link>tags with correct MIME types +theme-colormetaassets/favicons/site.webmanifest— 192 + 512 icons withpurposefieldscripts/generate-favicons.sh— reproducible generation from SVG sourcescripts/test-favicons.sh— 27 assertionsAcceptance criteria (#471)
Wallet:
HZV6YPdTeJPjPujWjzsFLLKja91K2Ze78XeY8MeFhfK8