Skip to content

fix: untrack new comments only on actual unmount or interaction#2930

Merged
huumn merged 4 commits intostackernews:masterfrom
Soxasora:fix/live-comments-navigator-tracking-patch
Apr 27, 2026
Merged

fix: untrack new comments only on actual unmount or interaction#2930
huumn merged 4 commits intostackernews:masterfrom
Soxasora:fix/live-comments-navigator-tracking-patch

Conversation

@Soxasora
Copy link
Copy Markdown
Member

@Soxasora Soxasora commented Apr 21, 2026

Description

Fixes #2927
New comment outlining and tracking happens within a useEffect that depends on lastCommentAt and meCommentsViewedAt of the root item.

Live comments would inject a new comment updating the very same fields the effect depends on, triggering the effect cleanup that untracks new comments. The same class of changes were likely happening with any other cache mod to root, like a zap.

Screenshots

dedicated-navigator-cleanup-before-after.mov

Additional Context

This didn't happen in React 18/Apollo Client 3. A theory can be that React 19 has different render scheduling/transitions.

Zap -> navigator clear cannot be reproduced in dev for some reason. Tracing untrackNewComment in Chrome DevTools showed that the useEffect cleanup, that untracks new comments, runs also after a zap or live comment.

Checklist

Are your changes backward compatible? Please answer below:

yes

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:

5, could only verify the live comments bug, but it's in the same class/behavior of the zap->untrack bug.

Did you use AI for this? If so, how much did it assist you?
no


Note

Medium Risk
Adjusts React effect timing around new-comment tracking/untracking; mistakes could cause missed outlines, incorrect counts, or sticky navigator state on item pages.

Overview
Fixes premature removal of “new comment” tracking/outline by tracking each comment at most once and only untracking on real unmount.

Comment now keeps a didTrackRef flag to avoid re-registering on re-renders/cache updates and moves the untrackNewComment call into an unmount-only cleanup. use-comments-navigator makes trackNewComment synchronous and returns whether insertion occurred, enabling immediate, reliable untracking on user interaction.

Reviewed by Cursor Bugbot for commit 429d08b. Bugbot is set up for automated code reviews on this repo. Configure here.

@Soxasora Soxasora marked this pull request as ready for review April 22, 2026 11:17
@Soxasora Soxasora requested a review from huumn April 22, 2026 15:50
Copy link
Copy Markdown

@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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit ee781e6. Configure here.

Comment thread components/comment.js Outdated
@huumn
Copy link
Copy Markdown
Member

huumn commented Apr 26, 2026

bot review claims to have found a regression:

This looks like a regression from the new didTrackRef tracking lifetime. The fix avoids dependency cleanup untracking live comments too early, but now marks the comment as tracked before trackNewComment actually inserts it on the next animation frame.

If the user dismisses the outline before that RAF runs, the pending insert can still add the comment back after it was untracked, and unmount cleanup may skip it because didTrackRef was reset to false.

Suggested patch: make trackNewComment register synchronously and return whether the ref was actually tracked, then set didTrackRef.current from that return value.

@Soxasora
Copy link
Copy Markdown
Member Author

We don't even need the RAF for tracking new comments, I remember implementing it just to micro-optimize and follow the same pattern of throttleCountUpdate but it's pretty much useless, it's useful only to update the count without causing too many re-renders due to the context.

Fixed! Thank you!!

@huumn huumn merged commit cb77e56 into stackernews:master Apr 27, 2026
7 checks passed
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.

live comments indicator/navigator regressions

2 participants