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

Include own replies to own post roots via new filter posts_and_author_threads #1776

Merged
merged 11 commits into from
Dec 8, 2023

Conversation

estrattonbailey
Copy link
Member

@estrattonbailey estrattonbailey commented Oct 25, 2023

Update here: to avoid breaking any other clients, we've opted to keep posts_no_replies as-is, and add a new posts_and_author_threads filter that includes the logic needed by the frontend. I'll make another corresponding frontend PR to update the filter param.

Original post below.


Since we moved posts_no_replies logic to the backend, we've lacked self-threads on the default "Posts" view of profiles.

This PR expands that query to include replies, but ONLY if those replies are (1) by the author you're viewing and (2) their reply root is a post by the author you're viewing.

As noted in bluesky-social/social-app#1166 (comment), this results in returning replies that are "detached" from the root by a post by another user. That part will be handled on the frontend for now, at least until we move to our new backend architecture.

@mary-ext
Copy link
Contributor

mary-ext commented Oct 26, 2023

could there be an additional condition where the replyParent also starts with at://${actorDid}/ if it matches the replyRoot startswith?

I'm a little wary that as it is now, it could be catastrophic for profiles whose content is primarily dominated by A <- B <- A sort of replies (e.g. certain clusters like the doll cluster is pretty used to making daily greetings with each other)

so by adding this condition, apps could just focus on filtering out the very edge case scenario as noted in bluesky-social/social-app#1166 (comment)

@estrattonbailey
Copy link
Member Author

Hey @mary-ext 👋

could there be an additional condition

If I understand what you're asking, yeah, a replyParent could have been written by the same actor and could match too. But that replyParent would also have a replyRoot that would match, so this check should capture both.

could be catastrophic for profiles

Could you elaborate on what would happen in this case? For A -> B -> A reply trees, this PR should flatten them to only return A -> A. The logic here is (intended to be, anyway): get all posts and reposts for an actor, PLUS any replies for an actor that are written by the actor in response to a "root" post by the same actor.

The frontend will then filter out any "detached" threads, as mentioned in bluesky-social/social-app#1166 (comment). The result should be that a user should see only posts, reposts, and their own unbroken threads.

I'll double check on this stuff! Thanks for the comment, lmk if I'm off-base or not making sense 😄

@mary-ext
Copy link
Contributor

mary-ext commented Oct 27, 2023

ah, let me visualize it this way then

Alice -> makes a good morning post
  Bob
    Alice
  Natalie
    Alice
  Devon
    Alice
  Naomi
    Alice
  ... so on

from the query that I'm seeing in this PR, Alice's replies to everyone else would make it onto post_no_replies as it's still under the same thread root, and therefore have to be filtered away by the client, or is there something I'm not getting at here?

edit: ohh, but then I suppose filtering it then would make it impossible to work around the edge case in bluesky-social/social-app#1166 (comment) wouldn't it?

@estrattonbailey
Copy link
Member Author

@mary-ext yep exactly, you're correct. This PR will include all replies by Alice and send them down to the frontend.

The frontend then does the rest of the work. For now, we're going filter out any posts that are not part of an un-broken chain back to a root post by the user. So for the edge case here bluesky-social/social-app#1166 (comment), the reply by Bob and the two replies to Bob from Alice will not appear on Alice's posts_no_replies tab.

…tab-should-show-own-threads

* origin/main: (59 commits)
  Config to start notifications daemon from a specific did (#1922)
  Feature branch: PDS v2 (#1789)
  Cleanup outdated notifications in appview, add daemon for similar tasks (#1893)
  Add flag for running db migrations on appview (#1913)
  Do not generate notifs when post violates threadgate (#1901)
  Version packages (#1909)
  Additional @atproto/api 0.6.24 changeset (#1912)
  Fix snapshots for list items (#1911)
  Attach record URI to listItemView (#1758)
  helpers for rkey and tid syntax; validate rkey at record creation time (#1738)
  harden datetime verification (#1702)
  fix(debug): properly type debugCatch wrapper result (#1817)
  style(xrpc-server): avoid un-neccessary "if" statement (#1826)
  perf(bsky): avoid re-creating auth functions on every request (#1822)
  Don't create unnecessary error objects (#1908)
  fix(pds): include aspectRatio on read-sticky posts (#1824)
  Handle missing creator on lists and feed generators (#1906)
  ✨ Expose labels attached with legacy actions when events are queried and fix email event builder (#1905)
  Evented architecture for moderation system (#1617)
  Put canReply state on post viewer state instead of thread viewer state (#1882)
  ...
…hould-show-own-threads

* origin:
  Version packages (#1945)
  Strip trailing colons from link detection, add test (#1944)
  Add labels to actor search typeahead results (#1940)
  Version packages (#1938)
  Bump api package for no-unauthenticated PR (#1937)
  Add !no-unauthenticated imperative label (#1926)
  Fail open on did cache (#1934)
  Appview rate limits parse cidr block (#1932)
  Tweak rate limit cfg (#1931)
  Set trust proxy on appview for xff headers (#1930)
  Transfer with-friends & best-of-follows to feed generator (#1919)
  Add basic application rate limits to appview (#1902)
  Cache tweaks (#1929)
  Fix typo in service entry
  Import redis in service entry
  Cache labels in Redis (#1897)
  ♻️ Cleanup linter warnings (#1907)
  fix(bsky): cf image invalidator not taken into account when bunny used (#1823)
  pre-compile handlebar templates at build time (#1833)
  feat: infer definition type from "types" argument in getDefOrThrow (#1812)
@estrattonbailey estrattonbailey changed the title Include own replies to own post roots in post_no_replies Include own replies to own post roots via new filter posts_and_author_threads Dec 8, 2023
@@ -99,23 +99,23 @@ export const LABELS: LabelDefinitionMap = {
strings: {
settings: {
en: {
name: 'Requested Hidden to Logged-out Users',
name: 'Sign-in Required',
Copy link
Collaborator

Choose a reason for hiding this comment

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

guess this codegen just didn't quite make it into a previous PR?

wanna make sure this PR contains the correct content & not main

Copy link
Collaborator

@dholms dholms left a comment

Choose a reason for hiding this comment

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

sweet this looks great 👌

@dholms dholms merged commit ffe39aa into main Dec 8, 2023
@dholms dholms deleted the eric/app-891-replies-tab-should-show-own-threads branch December 8, 2023 21:32
@github-actions github-actions bot mentioned this pull request Dec 8, 2023
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.

4 participants