Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PR preview pages don't handle visit to non deployed pages well #12786

Open
OnkarRuikar opened this issue Jan 23, 2025 · 6 comments
Open

PR preview pages don't handle visit to non deployed pages well #12786

OnkarRuikar opened this issue Jan 23, 2025 · 6 comments
Labels
needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened.

Comments

@OnkarRuikar
Copy link
Contributor

Summary

Previews generated by the PR review companion include only the modified files. On preview pages, links to other unmodified pages give a 404 error. Also, the error is wrong: Key: pr37670/en-us/_spas/404.html. Following is the content of the 404 response:

404 Not Found
Code: NoSuchKey
Message: The specified key does not exist.
Key: pr37670/en-us/_spas/404.html
RequestId: W9ZG8PFR8V5N0MK5
HostId: EoaAHM8g7wgSv7rGa1dRb6/F9o5Pg93I6h0iZVYuA8A5x1RrlpNKgHZVADEZDyl/ZnC2KuEdIr8=

Reviewers find it annoying to see the 404 page instead of the requested page, even though it's not modified in the PR. It would be better if, on the 404 page, there is a link to the page on the production site.

Steps to reproduce

  1. Open any latest PR in the mdn/content repo.
  2. Open any preview page.
  3. On the page, click on any link to an MDN page that is not modified in the PR.
  4. You'll get pr37670/en-us/_spas/404.html not found error.

Solution

The solution is based on the TamporMonkey script created by Will. The script adds a link to the same doc on the production site. Instead of TamporMonkey, we can do it in the deployer code itself.\

During dev deployments for PRs, populate the /en-us/_spas/404.html file with the following content:

<!DOCTYPE html>
<HTML>
  <head>
      <title>404 Page Not Found</title>
  </head>
  <body>
    <h1>404 Page Not Found</h1>
    <script>
      if (document.location.hostname.endsWith("content.dev.mdn.mozit.cloud")) {
        const link = document.createElement("a");
        link.href = `https://developer.mozilla.org${document.location.pathname}`;
        link.textContent = "View on MDN production server.";
        document.body.appendChild(link);
      }
    </script>
  </body>
</html>

Note: Ignore the fact that PR build doesn't populate the original _spas/404.html. The production style 404 page is not useful in PR previews anyway.
@github-actions github-actions bot added the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label Jan 23, 2025
@OnkarRuikar OnkarRuikar mentioned this issue Jan 23, 2025
Closed
@caugner
Copy link
Contributor

caugner commented Jan 23, 2025

The root cause for the missing 404 page is that it was moved from /{locale}/_spas/404.html to /{locale}/404/index.html in #12248, while CloudFront was still configured to serve it from /en-US/_spas/404.html.

I have now updated the CloudFront config to serve the correct error page.

@Josh-Cena Josh-Cena removed the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label Jan 23, 2025
@OnkarRuikar
Copy link
Contributor Author

I think this is still in progress. Convesation from the last meeting:

(onkar) mdn/content#37769 (comment) - instead of direct redirect would be better if we show 404 page in pre preview

  • We should bring this back to engineering so that it should be more apparent.
  • (Estelle) Maybe open in a new tab for these?
  • (Chris) Actually this is really cool, we should tweak it a little
  • (Ruth) Maybe not do this for media? Seems like broken URLs to images etc could be bad. Looks like tweaks could make it seamless

@Josh-Cena Josh-Cena reopened this Mar 15, 2025
@github-actions github-actions bot added the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label Mar 15, 2025
@Josh-Cena
Copy link
Member

I thought this issue is only about the bad not found page. If you think we should solve the whole 404 UX in this PR, that's fine to me. I'm not sure what the preferences are from the notes though. It seems people are neither happy with showing a 404 (current behavior) nor with directly redirecting to prod (previous behavior, iirc). Maybe a custom 404? But anyway this seems entirely out of control for the content repo and should go to engineering instead.

@Josh-Cena Josh-Cena removed the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label Mar 15, 2025
@OnkarRuikar
Copy link
Contributor Author

Move this issue to the rari repo?
I think they simply need to add a View on MDN live server link to the current 404 page when it's not a prod environment.

@pepelsbey pepelsbey transferred this issue from mdn/content Mar 17, 2025
@pepelsbey
Copy link
Member

@caugner moved it to rari as it seemed like a proper place

@caugner caugner transferred this issue from mdn/rari Mar 17, 2025
@github-actions github-actions bot added the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label Mar 17, 2025
@caugner
Copy link
Contributor

caugner commented Mar 17, 2025

@pepelsbey Both the 404 page and the Cloud Function serving the 404 page are in yari.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(ci/preview): add custom 404 page for PR preview pages OnkarRuikar/content
4 participants
@pepelsbey @caugner @Josh-Cena @OnkarRuikar