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

remove resolution from post thread #4297

Merged
merged 7 commits into from
Jun 3, 2024

Conversation

haileyok
Copy link
Contributor

Why

In the continuous effort to remove waterfalls, we can now remove these requests from getPostThread. To do so however, we need to do a little bit of reworking on how we pull already-loaded posts from the cache.

Currently we load the posts from the cache just based off of their URI. However, since we now are not going to have the resolved URI, we instead need to check both the handle and the rkey instead in our various findAllPostsInQueryData functions. This is a pretty straight forward change, though it adds a little extra logic.

Test Plan

Note in the debugger that there are no more requests for DID resolutions when opening a post on a fresh load. Also note that whenever you open a post from these areas, there is no spinner:

  • Feed
  • Profile
  • Search page
  • Notifications

nit

completely remove did cache lookup

move cache check for did to `usePostThreadQuery`

remove resolution from post thread
Copy link

render bot commented May 31, 2024

Copy link

github-actions bot commented May 31, 2024

Old size New size Diff
7.38 MB 7.38 MB 22 B (0.00%)

Copy link
Member

@mozzius mozzius left a comment

Choose a reason for hiding this comment

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

good stuff

@gaearon
Copy link
Collaborator

gaearon commented May 31, 2024

Placeholders regressed.

plc.mov

} else if (
!isDid &&
quotedPost?.author.handle === atUri.host &&
quotedPost.uri.endsWith(atUri.rkey)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels complicated. Ok but wondering if there are easier ways. I do think we need to pull this into a utility though cause the copy pasta is not obvious.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's at least extract isSamePostUri or something

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

  • let's reduce duplication of the URI check (it's non-trivial)
  • placeholders are broken

@haileyok
Copy link
Contributor Author

@gaearon can you check now if placeholders work? I can't repro the spinner even off the first commit and still can't, so not sure what happened there? Left a video on Slack.

@haileyok haileyok requested a review from gaearon May 31, 2024 21:05
@gaearon gaearon merged commit 8d83234 into main Jun 3, 2024
6 checks passed
@gaearon gaearon deleted the hailey/remove-did-resolution-post-thread branch June 3, 2024 22:58
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