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.

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

fix: add a nice sharing 404 #28065

Merged
merged 4 commits into from
Jan 31, 2025
Merged

fix: add a nice sharing 404 #28065

merged 4 commits into from
Jan 31, 2025

Conversation

pauldambra
Copy link
Member

if someone visits a sharing URL that doesn't exist or doesn't exist any more

they get a DRF API style error page, yuck!

before after
Screenshot 2025-01-29 at 23 16 24 Screenshot 2025-01-29 at 23 14 22

@pauldambra pauldambra requested a review from veryayskiy January 29, 2025 23:17
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR enhances the user experience by replacing the default DRF API error page with a custom 404 page for invalid or expired sharing URLs.

  • Added new shared_resource_404.html template with a space-themed design and PostHog branding
  • Modified SharingViewerPageViewSet in posthog/api/sharing.py to handle NotFound exceptions with custom 404 response
  • Improved error handling for shared resources that don't exist or are no longer accessible

2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +209 to +211
def custom_404_response(request):
"""Returns a custom 404 page."""
return render(request, "shared_resource_404.html", status=404)
Copy link
Contributor

Choose a reason for hiding this comment

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

style: consider adding request type annotation and docstring with more details about the template context

Copy link
Member

@rafaeelaudibert rafaeelaudibert left a comment

Choose a reason for hiding this comment

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

LGTM, but I think we could load a font to make it slightly nicer

{% include "head.html" %}
</head>
<style>
html, body, iframe {
Copy link
Member

Choose a reason for hiding this comment

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

It would be cool if you could load our font using @font, will look more professional compared to the Times New Roman thing we have going on

Copy link
Contributor

@veryayskiy veryayskiy left a comment

Choose a reason for hiding this comment

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

I'm kinda desgner myself

@pauldambra pauldambra enabled auto-merge (squash) January 30, 2025 08:49
@pauldambra pauldambra merged commit 83e0dc0 into master Jan 31, 2025
92 checks passed
@pauldambra pauldambra deleted the feat/nice-sharing-404 branch January 31, 2025 10:23
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.

3 participants