Skip to content

fix: avoid unnecessary favicon.ico requests when unset#152

Merged
justlevine merged 6 commits intortCamp:developfrom
SH4LIN:fix/favicon-loading
Apr 14, 2025
Merged

fix: avoid unnecessary favicon.ico requests when unset#152
justlevine merged 6 commits intortCamp:developfrom
SH4LIN:fix/favicon-loading

Conversation

@SH4LIN
Copy link
Contributor

@SH4LIN SH4LIN commented Apr 10, 2025

What

  • This PR will fix the new GraphQL request sent for favicon.ico

Why

  • When icon tag is not available in head tag browser tries to fetch it from /favicon.ico path. This request is getting to the NextJS Server and it is considering this request to a page request and making a GQL request to fetch favicon.ico request.

How

  • Passing # as icon when Icon is not available

Checklist

  • I have read the Contribution Guidelines.
  • My code is tested to the best of my abilities.
  • My code passes all lints (ESLint, tsc, prettier etc.).
  • My code has detailed inline documentation.
  • I have added unit tests to verify the code works as intended.
  • I have updated the project documentation as needed.
  • I have added a changeset for this PR using npm run changeset.

@changeset-bot
Copy link

changeset-bot bot commented Apr 10, 2025

🦋 Changeset detected

Latest commit: dd51146

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@snapwp/next Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@SH4LIN SH4LIN requested a review from justlevine April 10, 2025 09:44
@SH4LIN SH4LIN requested a review from ayushnirwal April 10, 2025 09:46
@coveralls
Copy link

coveralls commented Apr 10, 2025

Pull Request Test Coverage Report for Build 14432541899

Details

  • 0 of 4 (0.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 53.621%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/next/src/middleware/proxies.ts 0 2 0.0%
packages/next/src/root-layout/icons-metadata.ts 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
packages/next/src/middleware/proxies.ts 1 3.45%
Totals Coverage Status
Change from base Build 14394242697: -0.1%
Covered Lines: 399
Relevant Lines: 660

💛 - Coveralls

@SH4LIN SH4LIN changed the title fix: pass default # as icon url fix: pass default icon url Apr 10, 2025
Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not a real approval, confirming repo permissions, will reset and review shortly)

@justlevine justlevine self-requested a review April 10, 2025 11:20
@justlevine justlevine self-requested a review April 11, 2025 02:30
@justlevine justlevine requested a review from Copilot April 13, 2025 19:20
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

packages/next/src/root-layout/icons-metadata.ts:49

  • Consider adding unit tests for the getIcons function to verify that it correctly returns the fallback icons when settings are undefined.
if ( ! settings ) { return fallbackIcons; }

packages/next/src/middleware/proxies.ts:25

  • Consider adding unit tests to ensure that requests to '/favicon.ico' are correctly intercepted and return a 404 response.
if ( '/favicon.ico' === nextPath ) {

Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing else to add. I defer to @ayushnirwal for review/approval.

@justlevine justlevine changed the title fix: pass default icon url fix: avoid unnecessary favicon.ico requests when unset. Apr 14, 2025
@justlevine justlevine changed the title fix: avoid unnecessary favicon.ico requests when unset. fix: avoid unnecessary favicon.ico requests when unset Apr 14, 2025
@justlevine justlevine merged commit 2595d68 into rtCamp:develop Apr 14, 2025
9 of 11 checks passed
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.

5 participants