🐛 Fixed missing favicon and apple-touch-icon in React admin#27528
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
leafwind
changed the title
leafwind
force-pushed
the
fix/react-admin-missing-apple-touch-icon
branch
3 times, most recently
from
d232149 to
f30b7bd
Compare
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
leafwind
force-pushed
the
fix/react-admin-missing-apple-touch-icon
branch
from
f30b7bd to
0a0dd9f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/admin/src/index-html.test.ts (2)
6-8: Regex is sensitive to attribute order/quoting.The pattern only matches when
rel="apple-touch-icon"appears beforehref, both use double quotes, and there's no intervening whitespace variation. A benign edit toindex.html(e.g., reordering attributes tohref="..." rel="apple-touch-icon", using single quotes, or addingsizes/typebeforehref) would silently null outappleTouchIconHref, causing tests 2 and 3 to fail for reasons unrelated to the actual bug this suite is guarding against. Consider parsing with a lightweight HTML parser (e.g.,node-html-parser) or splitting into two regex passes: locate the<link ... rel="apple-touch-icon" ...>tag first, then extracthreffrom it independently of order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/index-html.test.ts` around lines 6 - 8, The current extraction for appleTouchIconHref (headContent and appleTouchIconHref) is brittle because the regex assumes rel appears before href and uses double quotes; update the logic to be order- and quote-agnostic by either using a lightweight HTML parser (e.g., node-html-parser) to parse headContent and query for the <link rel="apple-touch-icon"> element then read its href, or perform two robust regex passes: first match the full <link ...> tag containing rel=('|")apple-touch-icon\1 (searching within headContent) and then extract the href attribute from that matched tag regardless of attribute order/quoting; replace the current single regex that sets appleTouchIconHref accordingly.
15-20: Also guard against protocol-absolute URLs.The "relative path" assertion only rejects paths starting with
/. Anhreflikehttps://cdn.example.com/apple-touch-icon.pngor//cdn.example.com/...would still pass, yet would equally break subdirectory installs (and thefs.existsSynccheck in the next test would then fail confusingly). A small tightening keeps the intent explicit.🧪 Proposed tightening
- expect(filePath).not.toMatch(/^\//); + expect(filePath).not.toMatch(/^\/|^[a-z]+:\/\//i); expect(filePath).toContain('apple-touch-icon.png');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/admin/src/index-html.test.ts` around lines 15 - 20, The test "href is a relative path (absolute paths break subdirectory installs)" currently only rejects leading slashes; update the assertions around appleTouchIconHref in that test to also reject protocol-absolute and absolute URLs (e.g., starting with '//' or a scheme like 'http:'/'https:'). Locate the test block containing the variable appleTouchIconHref and replace or add the regex check so filePath (or appleTouchIconHref) does not match patterns like /^\/|^\/\/|^[a-zA-Z][a-zA-Z0-9+.-]*:/. This will ensure only true relative paths are allowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/admin/src/index-html.test.ts`:
- Around line 6-8: The current extraction for appleTouchIconHref (headContent
and appleTouchIconHref) is brittle because the regex assumes rel appears before
href and uses double quotes; update the logic to be order- and quote-agnostic by
either using a lightweight HTML parser (e.g., node-html-parser) to parse
headContent and query for the <link rel="apple-touch-icon"> element then read
its href, or perform two robust regex passes: first match the full <link ...>
tag containing rel=('|")apple-touch-icon\1 (searching within headContent) and
then extract the href attribute from that matched tag regardless of attribute
order/quoting; replace the current single regex that sets appleTouchIconHref
accordingly.
- Around line 15-20: The test "href is a relative path (absolute paths break
subdirectory installs)" currently only rejects leading slashes; update the
assertions around appleTouchIconHref in that test to also reject
protocol-absolute and absolute URLs (e.g., starting with '//' or a scheme like
'http:'/'https:'). Locate the test block containing the variable
appleTouchIconHref and replace or add the regex check so filePath (or
appleTouchIconHref) does not match patterns like
/^\/|^\/\/|^[a-zA-Z][a-zA-Z0-9+.-]*:/. This will ensure only true relative paths
are allowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6813bed-9c3f-4e8c-bdb0-abf0ec05ae83
⛔ Files ignored due to path filters (1)
apps/admin/public/assets/img/apple-touch-icon.pngis excluded by!**/*.png
📒 Files selected for processing (2)
apps/admin/index.htmlapps/admin/src/index-html.test.ts
✅ Files skipped from review due to trivial changes (1)
- apps/admin/index.html
|
jonatansberg
force-pushed
the
fix/react-admin-missing-apple-touch-icon
branch
from
d48ba85 to
beb5168
Compare
iOS Safari falls back to the page title's first letter ("G") instead of
the Ghost logo when saving the admin as a home screen shortcut. The
apple-touch-icon link was lost during the Ember→React admin migration —
emberAssetsPlugin only injects CSS, JS, and config meta tags, not icon
link tags.
Moved the remaining admin favicon asset into the React admin public assets so the React entrypoint owns the icon metadata it serves.
|
@leafwind thank you for identifying this and creating a PR! I went ahead and moved the old favicon as well. |
jonatansberg
force-pushed
the
fix/react-admin-missing-apple-touch-icon
branch
from
beb5168 to
598e44a
Compare
@jonatansberg I appreciate your help (because I am not familiar with the folder structure), thanks! |
3 participants
🐛 Fixed missing favicon and apple-touch-icon in React admin
iOS Safari falls back to showing the first letter of the page title ("G") instead of the Ghost logo when adding the admin to an iOS home screen. The apple-touch-icon link was lost during the Ember→React admin migration — the emberAssetsPlugin only injects CSS, JS, and config meta tags, not icon link tags.
Note
Low Risk
Small, static HTML change plus a unit test; no runtime logic or security-sensitive code paths affected.
Overview
Restores iOS home-screen icon support for the React admin by adding an
apple-touch-iconlink toapps/admin/index.html.Adds a Vitest check (
src/index-html.test.ts) to ensure the tag exists and that itshrefis relative (not/...) so admin installs served from subdirectories don’t break.Reviewed by Cursor Bugbot for commit d232149. Bugbot is set up for automated code reviews on this repo. Configure here.