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

comment pagination with limit/offset #1824

Merged
merged 13 commits into from
Jan 30, 2025
Merged

comment pagination with limit/offset #1824

merged 13 commits into from
Jan 30, 2025

Conversation

huumn
Copy link
Member

@huumn huumn commented Jan 18, 2025

fix #1470

  • For large comment threads we go back to the recursive comment query of yore so we can limit/order the children of each parent independently. We continue to store the results in a temporary table so it can aggregate the results into a tree through the recursive function call
  • For large comment threads, clicking on a comment in notifications takes you directly to the comment rather than to the thread then scrolling to it
  • This adds a new denormalized field nDirectComments so that we can determine whether there are more comments to be loaded
  • The current comment query performs well enough with 200 or so comments, so we're keeping it for threads with <200 comments. Then for anything larger, we limit top level to 50 then aggressively limit children of children.

The main trickiness when balancing performance with UX on comments is that we don't know how dense the tree is by looking at the parent. e.g. an item can have 300 comments, with 75 top level with 3 children each, or 3 top level with 99 children each. So without computing or denormalizing better density info, we also need to limit children of children aggressively - and we do.

TODO

  • cursor
    • need to use time in cursor to avoid duplicates
  • view more buttons
    • style more button on children of children
    • need a better way to communicate to client if results were actually truncated
      • went with denormalizing direct children counts
  • need to figure out rules/constraints for reply links in notifications, or make exceptions in query when commentId is set
  • need to make sure pinned comments are always returned first
    • children are borked
  • for threads < 200 comments, it might be more efficient to use the old logic
  • replies to top level items are borked given the cache/query shape changes
  • top level items without replied have unexpected footer bookend
  • squash migrations
  • rendering is still very slow (but this isn't the pr to fix it0
    • if you make it so that replies don't render (comment them out), the page loads fast even with lots of comments ... most of the slowdown in rendering

QA

Easiest way to create megathreads is by inserting generated rows:

WITH insert_children AS (
    INSERT INTO "Item" ("parentId", "userId", "text", created_at, ncomments, "nDirectComments")
    SELECT 458362, 616, substr(md5(random()::text), 1, 25), now() - interval '1 day', 15+15*15, 15
    FROM generate_series(1, 150) AS g (id)
    RETURNING id
),
update_children_ncomments AS (
    UPDATE "Item" SET ncomments = ncomments + 150+150*15*15, "nDirectComments" = 150 WHERE id = 458362
),
insert_children_of_children AS (
    INSERT INTO "Item" ("parentId", "userId", "text", created_at, ncomments, "nDirectComments")
    SELECT insert_children.id, 616, substr(md5(random()::text), 1, 25), now() - interval '1 day', 15, 15
    FROM insert_children, generate_series(1, 15) AS g (id)
    RETURNING id
),
insert_children_of_children_of_children AS (
    INSERT INTO "Item" ("parentId", "userId", "text", created_at)
    SELECT insert_children_of_children.id, 616, substr(md5(random()::text), 1, 25), now() - interval '1 day'
    FROM insert_children_of_children, generate_series(1, 15) AS g (id)
    RETURNING id
)
SELECT 1;

You can create additional branches by just changing the parentId.

@huumn huumn changed the title basic query with limit/offset comment pagination with limit/offset Jan 18, 2025
@huumn huumn marked this pull request as draft January 18, 2025 16:49
@huumn huumn marked this pull request as ready for review January 29, 2025 19:51
@huumn huumn merged commit 01b021a into master Jan 30, 2025
6 checks passed
@huumn huumn deleted the comment-pagination branch January 30, 2025 01:00
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.

pagination for comments
1 participant