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 orphaned feed slices, handle blocks #4944

Merged
merged 5 commits into from
Aug 19, 2024
Merged

Conversation

estrattonbailey
Copy link
Member

@estrattonbailey estrattonbailey commented Aug 15, 2024

Relies on bluesky-social/atproto#2721

Fixes a regression in #4871 where A -> B -> C and A blocks B, we'd see the entire A -> B -> C thread, whereas the intention was to filter out those "orphaned" threads. If we want to show orphaned threads, it's a one-line change in feed-tuners.

Testing

Test the scenarios below using the backend PR. Otherwise test prod, nothing should look different using prod data. Only the backend PR sends down data needed to compute the correct view.

Blocks

A blocks B, or B blocks A. Viewing as C.

See in prod, thread is orphaned (and technically "Reply to test.esb.lol" should say "Reply to blocked post")
CleanShot 2024-08-15 at 17 19 01@2x

In main, you can see through the 3p blocks:
CleanShot 2024-08-15 at 17 20 28@2x

In this PR, C only sees A because the thread view would otherwise be "orphaned"
CleanShot 2024-08-16 at 16 09 08@2x

C blocks A. Viewing as C

In all cases the entire feed slice is filtered out 👍

C blocks B. Viewing as C.

In all cases, only A is visible 👍

Not found posts

In case of B deleting their post, again only A is visible in feeds, and when viewing C author feed, this PR now notes that the post was removed with the following:

CleanShot 2024-08-16 at 16 28 45@2x

I chose "unknown post" instead of "deleted post" because there are multiple reasons a post might be "not found".

Copy link

render bot commented Aug 15, 2024

Copy link

github-actions bot commented Aug 15, 2024

Old size New size Diff
7.09 MB 7.09 MB 835 B (0.01%)

src/state/preferences/feed-tuners.tsx Outdated Show resolved Hide resolved
src/lib/api/feed-manip.ts Outdated Show resolved Hide resolved
src/lib/api/feed-manip.ts Outdated Show resolved Hide resolved
src/lib/api/feed-manip.ts Outdated Show resolved Hide resolved
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.

i think this looks good, see nits above. i don't understand what rootIsView is yet so it would be nice to confirm it works as intended (what's special about "post or blocked"?) before merging.

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.

lg!

@estrattonbailey
Copy link
Member Author

Actually should have no effect on main for now, so should be clear to merge this ahead of app view changes

@estrattonbailey estrattonbailey merged commit 3976d67 into main Aug 19, 2024
6 checks passed
estrattonbailey added a commit that referenced this pull request Aug 21, 2024
* origin/main: (50 commits)
  Add `list hidden` screen (#4958)
  Expose more methods, support disabled items (#4954)
  Expose more props from button (#4953)
  Fix orphaned feed slices, handle blocks (#4944)
  Tweak `expo-modules-core` hack patch (#4955)
  [Experiment] Always show bottom bar (#4946)
  Revert "[Video] Download videos" (#4945)
  Move global "Sign out" out of the current account row (#4941)
  Hack patch for testing OTA update crash behavior (#4942)
  [Video] Download videos (#4886)
  swap control files (#4936)
  [Embed] Starter pack embed embed (#4935)
  [Video] set audio category to ambient every time a new player is made (#4934)
  Add `/live/` to supported YouTube embed URLs (#4932)
  [Video] Try/catch video play/pause (#4930)
  Don't kick to login screen on network error (#4911)
  Remove .withProxy() calls (#4929)
  [Video] Audio duck off main thread (#4926)
  subclass agent to add setPersistSessionHandler (#4928)
  [Video] Fix crash when switching tabs (#4925)
  ...
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.

2 participants