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

feat: User pages SSR & open graph #1095

Merged
merged 14 commits into from
Apr 17, 2023

Conversation

Deadreyo
Copy link
Contributor

@Deadreyo Deadreyo commented Apr 13, 2023

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation Update
  • 🎨 Style
  • πŸ§‘β€πŸ’» Code Refactor
  • πŸ”₯ Performance Improvements
  • βœ… Test
  • πŸ€– Build
  • πŸ” CI
  • πŸ“¦ Chore (Release)
  • ⏩ Revert

Description

  • Turns user pages /user/[username] into server-side rendering, instead of default static site generation.
  • Enables the meta tags in user pages.
  • Integrates opengraph service into meta tags.
  • Fetches the data in parallel. Time till finish on server on local decreased from ~800ms to ~400ms.

Some Notes

  • User profile's data is fetched on the server because it is needed for the user's bio meta tag. However, user's contribution data is not needed in the meta tags so it is fetched client-side to decrease the load on the server. This means a generic loading state for the whole page is now unacceptable, and there should be loading states over the components that wait for the contribution data. (Needs a ticket). Not applicable anymore till the below point is dealt with.
  • The page's inner UI component was disabled from SSR as a quick fix for hydration issues. Results in a slower initial UI rendering for the profile. Needs a separate ticket to dive and investigate the cause of these issues.
  • Requesting User data & social card data are done sequentially. They are independent and so should be done in parallel for faster response. Done.

Insight/Reasoning behind the decision to move to SSR: #1091 (comment)

Related Tickets & Documents

fixes #1091
fixes #879

Mobile & Desktop Screenshots/Recordings

image
image

Added tests?

  • πŸ‘ yes
  • πŸ™… no, because they aren't needed
  • πŸ™‹ no, because I need help

Added to documentation?

  • πŸ“œ README.md
  • πŸ““ docs.opensauced.pizza
  • πŸ• dev.to/opensauced
  • πŸ“• storybook
  • πŸ™… no documentation needed

[optional] Are there any post-deployment tasks we need to perform?

Some tickets need opening, mentioned in the side-effects in the description.

[optional] What gif best describes this PR or how it makes you feel?

@netlify
Copy link

netlify bot commented Apr 13, 2023

βœ… Deploy Preview for oss-insights ready!

Name Link
πŸ”¨ Latest commit 081cf99
πŸ” Latest deploy log https://app.netlify.com/sites/oss-insights/deploys/643af8065e9f2800083bb4c2
😎 Deploy Preview https://deploy-preview-1095--oss-insights.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Apr 13, 2023

βœ… Deploy Preview for design-insights ready!

Name Link
πŸ”¨ Latest commit 081cf99
πŸ” Latest deploy log https://app.netlify.com/sites/design-insights/deploys/643af80667b1f600084b7672
😎 Deploy Preview https://deploy-preview-1095--design-insights.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Deadreyo Deadreyo marked this pull request as ready for review April 14, 2023 01:03
Copy link
Contributor

@0-vortex 0-vortex left a comment

Choose a reason for hiding this comment

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

It's not the correct image URL. Make a 2nd HEAD request for the actual image! πŸ˜…

pages/user/[username]/index.tsx Outdated Show resolved Hide resolved
pages/user/[username]/index.tsx Outdated Show resolved Hide resolved
pages/user/[username]/index.tsx Outdated Show resolved Hide resolved
pages/user/[username]/index.tsx Outdated Show resolved Hide resolved
pages/user/[username]/index.tsx Outdated Show resolved Hide resolved
@Deadreyo
Copy link
Contributor Author

Deadreyo commented Apr 14, 2023

@0-vortex @brandonroberts
Updated. The reason I was initially awaiting the generation request was to make sure the image finished generating before the the page is sent to the client, to avoid the case in which the page is sent and meta tags are loaded to the client but the image hasn't finished generating (so returning 404 in the social card). Isn't this case possible?

@Deadreyo
Copy link
Contributor Author

Updated the code to now run the data fetching in parallel.

@0-vortex
Copy link
Contributor

to avoid the case in which the page is sent and meta tags are loaded to the client but the image hasn't finished generating (so returning 404 in the social card). Isn't this case possible?

This is a "backfill" type (first time ever) generation issues, due to our S3 bucket being versioned: https://docs.aws.amazon.com/AmazonS3/latest/userguide/versioning-workflows.html

You are, however, correct that the fetch request can reject the promise under a network error case or the API returning 500 server error. In that case, we should catch and not let the meta generation fail or break the page, potentially returning a placeholder image.

@0-vortex
Copy link
Contributor

Some more information: image generation takes at best 7 seconds. When posting a user page to any social media, the 2nd server request will read the static re-generated.

You can test the caching needs update 304 status for users in the list with 3 days last generation, while, users not in this list will trigger 404:

Screenshot 2023-04-14 at 23 55 44

@Deadreyo
Copy link
Contributor Author

@0-vortex
Alright how about this last commit as a solution for the image status? If the image doesn't exist (404), wait for it to be generated. If it is just outdated (304), then regenerate but don't wait (can use the old version).

@0-vortex
Copy link
Contributor

@0-vortex
Alright how about this last commit as a solution for the image status? If the image doesn't exist (404), wait for it to be generated. If it is just outdated (304), then regenerate but don't wait (can use the old version).

previous version was ok, this is not fixing the potential error.

@Deadreyo
Copy link
Contributor Author

@0-vortex Reverted.

Copy link
Member

@bdougie bdougie left a comment

Choose a reason for hiding this comment

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

Let see if we can get this merged to move on to the next social card, Highlights. The approach and performance can be improved as we learn through more use cases.

@brandonroberts brandonroberts merged commit d1a043b into beta Apr 17, 2023
@brandonroberts brandonroberts deleted the insights-users-opengraph-metatags branch April 17, 2023 13:02
github-actions bot pushed a commit that referenced this pull request Apr 17, 2023
## [1.42.0-beta.1](v1.41.1...v1.42.0-beta.1) (2023-04-17)

### πŸ• Features

* redirect user to the dashboard with repos after onboarding ([#1096](#1096)) ([765de71](765de71))
* server render user pages and add open graph image ([#1095](#1095)) ([d1a043b](d1a043b))
@github-actions
Copy link

πŸŽ‰ This PR is included in version 1.42.0-beta.1 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

github-actions bot pushed a commit that referenced this pull request Apr 18, 2023
## [1.42.0](v1.41.1...v1.42.0) (2023-04-18)

### πŸ• Features

* redirect user to the dashboard with repos after onboarding ([#1096](#1096)) ([765de71](765de71))
* server render user pages and add open graph image ([#1095](#1095)) ([d1a043b](d1a043b))

### πŸ› Bug Fixes

* update user id check for editing an insight page ([166530d](166530d))
@github-actions
Copy link

πŸŽ‰ This PR is included in version 1.42.0 πŸŽ‰

The release is available on GitHub release

Your semantic-release bot πŸ“¦πŸš€

ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this pull request Aug 17, 2023
## [1.42.0-beta.1](open-sauced/app@v1.41.1...v1.42.0-beta.1) (2023-04-17)

### πŸ• Features

* redirect user to the dashboard with repos after onboarding ([#1096](open-sauced/app#1096)) ([765de71](open-sauced/app@765de71))
* server render user pages and add open graph image ([#1095](open-sauced/app#1095)) ([d1a043b](open-sauced/app@d1a043b))
ElpisHelle added a commit to ElpisHelle/next.js-tailwindcss that referenced this pull request Aug 17, 2023
## [1.42.0](open-sauced/app@v1.41.1...v1.42.0) (2023-04-18)

### πŸ• Features

* redirect user to the dashboard with repos after onboarding ([#1096](open-sauced/app#1096)) ([765de71](open-sauced/app@765de71))
* server render user pages and add open graph image ([#1095](open-sauced/app#1095)) ([d1a043b](open-sauced/app@d1a043b))

### πŸ› Bug Fixes

* update user id check for editing an insight page ([166530d](open-sauced/app@166530d))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants