search

stackernews / stacker.news

Internet communities that pay you Bitcoin
https://stacker.news
MIT License
428 stars 109 forks source link

Comments appear as replies to root comment if replying on another page #473

Closed ekzyis closed 1 year ago

ekzyis commented 1 year ago

I noticed the same but I thought I made a mistake. However, another stacker also noticed this now: https://stacker.news/items/245783

So maybe this is a real bug. Creating this ticket to investigate

ekzyis commented 1 year ago

Mhh, just tested with this comment on my local machine.

reply on another page correctly links to that comment such that the reply also is beneath that comment ...

so we need to find a way to reproduce this bug, else i think this is going to be really hard to fix, lol

Since this happened to three stackers now, I think we can rule out user error, lol

ekzyis commented 1 year ago

Maybe if two people use reply on another page, this happens.

See https://stacker.news/items/245872

ekzyis commented 1 year ago

We were able to reproduce it with and without reply on another page ...

also, nemo was able to trigger the bug replying to this without reply on another page: https://stacker.news/items/246286

huumn commented 1 year ago

If a comment has the wrong parent, that means the comment is upserted with the wrong parentId so this bug involves some deviation in the way we pass parentId

ekzyis commented 1 year ago

Mhhh, yes. For some reason, I wrote here that I think it's a backend thing even though I have looked at the code and did see that we don't change the incoming parentId in any way, lol

Not sure anymore what I was thinking

huumn commented 1 year ago

It's very my likely frontend if it's related to reply on another page because reply on another page is just a link to an item as far as the backend is concerned.

huumn commented 1 year ago

Can you described how this bug appears in more detail. Is it:

  1. click on reply on another page
  2. reply on that page
  3. the reply shows up on that page initially but then when navigate back to the thread it has the wrong parent, an earlier ancestor?
ekzyis commented 1 year ago

yes, but for some reason the bug isn't triggered always like that. as mentioned above, nemo was also able to trigger the bug without reply on another page

so maybe it has nothing to do with reply on another page, lol

but i think it has to do with the comment depth

huumn commented 1 year ago

Okay, forgive for double checking, but being clear on this is important to us solving it. It appears to be sometimes recreated as follows:

  1. reply to comment of great depth (one so deep it's hidden in 'view replies' or 'reply on another page' in the root thread)
  2. the reply appears on the wrong parent, an earlier ancestor
ekzyis commented 1 year ago

yes. additionally, i think it's always 10 parents above. so basically, if you click "reply on another page" and then reply, the comment may show up below the top root comment. so as a reply to someone replying to a post.

huumn commented 1 year ago

When this happens, it eventually shows up as a reply to the root (and not some other ancestor)?

Second question: When this happens, what does the person submitting the reply see? Does the comment initially appear in the right spot then later they see it on the root?

ekzyis commented 1 year ago

okay, sorry that I've been lazy to create a detailed description, lol

I think an example will help:

marked comment in screenshot is this one: https://stacker.news/items/245763 2023-09-07-035806_1920x1080_scrot

however, the comment didn't end up as a reply to the marked comment in the previous screenshot, but here: https://stacker.news/items/245771 2023-09-07-035842_1920x1080_scrot

it eventually shows up as a reply to the root (and not some other ancestor)?

i think it immediately shows up as a reply to the root. and i think we only reproduced that comments from the first reply on the next page show up as the root comment as described in this screenshot:

2023-09-07-040524_1920x1080_scrot

When this happens, what does the person submitting the reply see? Does the comment initially appear in the right spot then later they see it on the root?

Yes, my experience is that you first see it (because of optimistic responses) but when reload, it's gone and you find it at the root comment

huumn commented 1 year ago

I think I found it https://github.com/stackernews/stacker.news/blob/e6ffeb8f763303de9ac881507809c0fdc6cc2053/components/reply.js#L93C1-L97C32

ParentId is not listed as a dep so whatever has been cached is being used.

If we want to be fancy and cache. We. Must. Always. Specify. All. Deps. (Saying this for me as much as I am for you.)

ekzyis commented 1 year ago

Ohh, hehe, yes, I think that's it

Nice catch! Didn't look out for missing deps

We should probably also check all other callbacks deps that I changed for anon stuff now

SatsAllDay commented 1 year ago

If we want to be fancy and cache. We. Must. Always. Specify. All. Deps. (Saying this for me as much as I am for you.)

I don't know about the standard linter that we use, but I know there is an eslint plugin for react that can highlight issues like this one (missing dependencies in deps array) to help avoid it via static analysis.

ekzyis commented 1 year ago

Yes, good point. :) I am currently working on integrating eslint into our CI pipeline. I think there are some other, similar bugs. For example, I think scrolling collapsed comments into view no longer works consistently:

/home/ekzyis/programming/stacker.news/components/reply.js
   36:40  error  'onSuccess' is defined but never used
                                                                               no-unused-vars
   43:6   error  React Hook useEffect has missing dependencies: 'parentId' and 'replyOpen'. Either include them or remove the dependency array. If 'setReply' needs the current value of 'replyOpen', you can also s
witch to useReducer instead of useState and read 'replyOpen' in the reducer    react-hooks/exhaustive-deps
   93:41  error  'amount' is defined but never used
                                                                               no-unused-vars
   97:6   error  React Hook useCallback has missing dependencies: 'parentId' and 'replyOpen'. Either include them or remove the dependency array. If 'setReply' needs the current value of 'replyOpen', you can also
 switch to useReducer instead of useState and read 'replyOpen' in the reducer  react-hooks/exhaustive-deps
  102:6   error  React Hook useEffect has a missing dependency: 'replyOpen'. Either include it or remove the dependency array
                                                                               react-hooks/exhaustive-deps

The error at 97:6 is causing the bug of this ticket. And the ones at 43:6 and 102:6 may be related to scrolling not working.

edit: Okay, no, I was wrong. replyOpen has nothing do with scrolling, lol. It's only about opening the text input to reply or not.

I just saw something with .focus() and thought maybe that's related. But it's not