Conversation
Preview deployments |
backspace
changed the title
Do we even need this assertion?
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e73b7e7f1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
I’ve fixed this to properly inject |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a38c3851c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Fixes duplicate favicon/apple-touch-icon tags in “host mode” by removing hardcoded icon tags from the host app HTML and making the realm server inject default icons only when needed.
Changes:
- Remove the default icon
<link>tags frompackages/host/app/index.htmlto prevent duplication when head HTML injection adds theme icons. - Update
RealmServer.serveIndexto inject default icon links (and a title fallback) when head HTML is missing or contains no icon links, including for non-public realms. - Expand/adjust server endpoint tests to assert there is exactly one favicon and one apple-touch-icon, and add a published-realm scenario test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/realm-server/tests/server-endpoints/index-responses-test.ts | Updates assertions around icon injection/duplication and adds a published-realm test scenario. |
| packages/realm-server/server.ts | Injects default icon links (and title fallback) when head HTML doesn’t supply icons / isn’t available. |
| packages/host/app/index.html | Removes static default icon tags so server-side injection doesn’t duplicate them in host mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/realm-server/tests/server-endpoints/index-responses-test.ts
Outdated
Show resolved
Hide resolved
packages/realm-server/tests/server-endpoints/index-responses-test.ts
Outdated
Show resolved
Hide resolved
lukemelia
left a comment
lukemelia
left a comment
There was a problem hiding this comment.
Codex's feedback should be addressed. Otherwise, looksgood
# Conflicts: # packages/realm-server/tests/server-endpoints/index-responses-test.ts
There was a problem with #4047 when deployed:
…/host/app/index.htmlincludes hardcoded icons, so injected ones are duplicates. This removes the default icons from that file and only injects them when we’re not in host mode or they’re missing. With this branch deployed, you can see the custom icon in the tab:The failing
hosttest is unrelated, flaky.