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.

Sign up for GitHub

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

Jump to bottom

Reviewing a lot of files resets the counter when loading more #20681

Closed
kolaente opened this issue Aug 5, 2022 · 2 comments · Fixed by #21230
Closed

Reviewing a lot of files resets the counter when loading more #20681

kolaente opened this issue Aug 5, 2022 · 2 comments · Fixed by #21230
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/bug
Milestone
1.17.3

Comments

@kolaente
Copy link
Member

kolaente commented Aug 5, 2022 •
edited
Loading

Description

Steps to reproduce:

  1. Create a repo
  2. Create a PR with a lot of changed files (in my case 193)
  3. Start reviewing the PR, marking each reviewed file as "viewed" with the checkbox
  4. Once the first 100 files are marked as viewed, click the "load more" button at the end of the page to load the rest of the files
  5. Mark the 101st file as viewed

Expected behavior:

101 files are marked as viewed

Actual behavior:

99 files are marked as viewed

The very first file is un-marked as viewed when marking the 101st file as viewed. When I mark it as viewed again, 100 files are marked as viewed again. When I then try to mark another of the newly loaded files as viewed, it unmarks the first file again, resulting in 99 files are marked as viewed.

Gitea Version

1.17.0

Can you reproduce the bug on the Gitea demo site?

Probably (on the go right now, will try to reproduce it later)

How are you running Gitea?

Official docker image.

Database

MySQL
@lunny lunny added this to the 1.17.1 milestone Aug 5, 2022
@delvh
Copy link
Member

delvh commented Aug 5, 2022 •
edited
Loading

Hmm.
I have absolutely no idea what could cause this.
I'm fairly certain I've tested the behavior on #19007, and there it worked correctly.
To be fair, I only tested by marking some and not all files as viewed before loading more files dynamically, but still?
I don't see anything in the code (I wrote) (web_src/js/features/pull-view-file.js) that would cause this, unless this.checked (where this is a checkbox) was inverted by default for some reason?

@delvh delvh added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Aug 5, 2022
@delvh
Copy link
Member

delvh commented Aug 5, 2022 •
edited
Loading

Yep, just confirmed it on try.gitea.io (https://try.gitea.io/delvh/kanban-test/pulls/25/files).
I had even stranger behavior: For me, the counter was reset to 0 (when loading more files), meaning that when I clicked on the 101st checkbox, the 1st checkbox was acted upon, and as that was already selected, it was de-selected, thus bringing the counter down to -1.

So, to me, the first issue appears to be an index issue where if x files are loaded initially, all subsequent ones use index - x (or index % x?) for the viewed checkbox JS. However, I still have no idea why this could be the case, and why I didn't notice it originally.
The second issue is that the counter was reset to 0, which it shouldn't.

@zeripath zeripath modified the milestones: 1.17.1, 1.17.2 Aug 17, 2022
@lunny lunny modified the milestones: 1.17.2, 1.17.3 Sep 5, 2022
@wxiaoguang wxiaoguang mentioned this issue Sep 21, 2022
Closed
@delvh delvh mentioned this issue Sep 21, 2022
Merged
wxiaoguang added a commit that referenced this issue Sep 21, 2022
@delvh @wxiaoguang
Prevent invalid behavior for file reviewing when loading more files (#…
acee32c 
…21230)

The problem was that many PR review components loaded by `Show more`
received the same ID as previous batches, which confuses browsers (when
clicked). All such occurrences should now be fixed.

Additionally improved the background of the `viewed` checkbox.

Lastly, the `go-licenses.json` was automatically updated.

Fixes #21228.
Fixes #20681.

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
delvh added a commit to delvh/gitea that referenced this issue Sep 21, 2022
@delvh @wxiaoguang
Prevent invalid behavior for file reviewing when loading more files (g…
188b260 
…o-gitea#21230)

The problem was that many PR review components loaded by `Show more`
received the same ID as previous batches, which confuses browsers (when
clicked). All such occurrences should now be fixed.

Additionally improved the background of the `viewed` checkbox.

Lastly, the `go-licenses.json` was automatically updated.

Fixes go-gitea#21228.
Fixes go-gitea#20681.

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@delvh delvh mentioned this issue Sep 21, 2022
Merged
techknowlogick pushed a commit that referenced this issue Sep 23, 2022
@delvh @wxiaoguang
Prevent invalid behavior for file reviewing when loading more files (#…
e50473e 
…21230) (#21234)

Backport of #21230

The problem was that many PR review components loaded by `Show more`
received the same ID as previous batches, which confuses browsers (when
clicked). All such occurrences should now be fixed.

Additionally improved the background of the `viewed` checkbox.

Fixes #21228.
Fixes #20681.

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/bug
Projects
None yet
Milestone
1.17.3
Development

Successfully merging a pull request may close this issue.

Prevent invalid behavior for file reviewing when loading more files delvh/gitea
4 participants
@lunny @zeripath @kolaente @delvh