Skip to content

Enhancements to live comments #2269

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

Merged
merged 95 commits into from
Jul 23, 2025

Conversation

Soxasora
Copy link
Member

@Soxasora Soxasora commented Jul 7, 2025

Description

Continues #2115 and #791
UI/UX and logic enhancements to the Live Comments system

  • Show all new comments of a thread
  • Scroll to new top-level comments
  • Outline newly injected comments
  • Blue live comments dot

Media

Blue live comments dot
image

Show all new comments of a thread

Screen.Recording.2025-07-23.at.12.40.32.mp4

Scroll to new top-level comments

Screen.Recording.2025-07-23.at.12.44.55.mp4

Outline newly injected comments

Screen.Recording.2025-07-23.at.12.46.35.mp4

Additional Context

This PR has been split from its proposed nice-to-have changes

fixes:

  • correct injection of comments at the deepest semi-visible level (lacking item.comments)
  • traverse comment structure without going further than the visible depth COMMENT_DEPTH_LIMIT

optimizations:

  • avoid closure persistence on payload creation via prepareComments
  • less array copies
  • avoid duplicate ncomments counting shared logic
  • don't mutate payloads for different comment structures, create dedicated ones

Checklist

Are your changes backward compatible? Please answer below:
For example, a change is not backward compatible if you removed a GraphQL field or dropped a database column.

Yes, these are completely new additions, no breakage or changes on the way comments and comment injection works.

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
7, successful iterative QA for showing all new comments of a thread, floating button visibility and ux/ui enhancements

For frontend changes: Tested on mobile, light and dark mode? Please answer below:
Yes, iterative QA

Did you introduce any new environment variables? If so, call them out explicitly here:
n/a

Soxasora and others added 30 commits April 18, 2025 05:34
…fix leak on useEffect because of missing sort

atomic apollo cache manipulations; manage top sort not being present in item query cache
queue nested comments without a parent, retry on the next poll
fix commit messages
… dropped comments;

ui: show amount of new comments

refactor: correct function positioning;

cleanup: useless logs
…Comments readFragment fallback to received comment
…ss dedupe on ShowNewComments, count nested ncomments from fresh new comments
@Soxasora Soxasora marked this pull request as ready for review July 23, 2025 10:48
cursor[bot]

This comment was marked as outdated.

…oads

- ncomments count logic shared with injection and counting
- don't re-create and persist closures for every injection, rather temporarily on injection
- access item hierarchy once, avoid creating new arrays
- don't create and mutate payloads, rather know what to return

fixes:
- fix wrong parameters on traverseNewComments recursion
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@Soxasora Soxasora requested a review from huumn July 23, 2025 14:06
cursor[bot]

This comment was marked as outdated.

@huumn
Copy link
Member

huumn commented Jul 23, 2025

Looks great for the most part. I'm seeing some weird behavior in corner cases though.

  1. I had two tabs, one per user, start commenting on an item without comments. For some reason, the tab that commented first did not get updates from the other tab until I refreshed the page. The console said there's a duplicate CommentFields fragment. I'm not sure if that's related but it might be.
  2. something strange happens when I edit a comment when there show new comments on it. It behaves like we've reached the depth limit.
Screenshot 2025-07-23 at 9 48 52 AM
  1. the language here is a bit awkward 'show all new 1 comment'
Screenshot 2025-07-23 at 9 48 52 AM Screenshot 2025-07-23 at 9 54 41 AM

@Soxasora
Copy link
Member Author

Soxasora commented Jul 23, 2025

  1. I had two tabs, one per user, start commenting on an item without comments. For some reason, the tab that commented first did not get updates from the other tab until I refreshed the page. The console said there's a duplicate CommentFields fragment. I'm not sure if that's related but it might be.

This is indeed weird, I'm trying to reproduce this but I can't, I'm now directly looking at how I use fragments.

  1. something strange happens when I edit a comment when there show new comments on it. It behaves like we've reached the depth limit.

I think I found the cause, for bottomed out status we check (item.comments?.comments.length === 0 && item.nDirectComments > 0). Applying an edit triggers the mutation COMMENT_UPDATE which gets new data from the server, thus nDirectComments would grow. But it's a mixed state, it should also get comments.comments. I'm working on a fix.

  1. the language here is a bit awkward 'show all new 1 comment'

I thought this too, let's see if I can find something actually better

…ated has pending newComments, fixes CommentEdit consequences
cursor[bot]

This comment was marked as outdated.

lib/apollo.js Outdated
Comment on lines 326 to 336
nDirectComments: {
merge (existing, incoming, { variables, readField }) {
// if the item has new comments, don't merge the nDirectComments field
// preventing new comments not yet injected from being counted, thus causing bottomedOut
if (variables?.id && readField('newComments')?.length > 0) {
return existing
}

return incoming
}
},
Copy link
Member Author

@Soxasora Soxasora Jul 23, 2025

Choose a reason for hiding this comment

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

on comment edits, upsertComment updates the cache with new values, which includes an updated nDirectComments but not comments, thus causing bottomedOut (item.comments?.comments.length === 0 && item.nDirectComments > 0) to trigger

So in the case of a comment with pending newComments, I think it's ok and harmless to ignore nDirectComments updates.

Screen.Recording.2025-07-23.at.18.36.17.mp4

edit: pushed a fix for incorrect usage of readField, it was missing 'from' where it should read this field... even though it was already reading it ... anyway, my tests shows that this works correctly.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: Function Fails When Comments Array Missing

The prepareComments function incorrectly assumes data.comments.comments is always an array when data.comments is present. If data.comments.comments is undefined, newComments.concat(data.comments.comments) will add undefined as an element to the array. The previous version safely handled this by falling back to an empty array.

components/show-new-comments.js#L55-L59

newComments: [],
comments: {
...data.comments,
comments: newComments.concat(data.comments.comments)
}

Fix in CursorFix in Web


Was this report helpful? Give feedback by reacting with 👍 or 👎

@huumn huumn merged commit 9092d90 into stackernews:master Jul 23, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improvements to existing features ui/ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants