Keyboard Shortcut 'c' to leave comment stops working

[Marquis Wong]

Hello everyone,

Another problem we've run into after upgrading to 3.6.1.

It seems like the "c" keyboard shortcut stops working if you navigate between files in the Gerrit UI. We are still able to highlight some text and hit 'c' to leave a comment, but just clicking on a single line and hitting 'c' does nothing.

Additionally, we've noticed that the new line highlighting on click stops working as well. 

If you refresh the page, both features start working again.

The highlighting is not important, but not being able to leave comments easily is a huge pain.

I did a clean Gerrit install of 3.6.1 and was able to reproduce pretty easily. I also disabled all of the plugins in case they were causing problems.

I did not notice anything interesting in the Javascript console. In safari, it seems to be reporting the shortcut, just not doing anything with it:

Has anyone else observed this? Is there anything I can do to help debug this?

Thanks,

Marquis

[Marquis Wong]

I'd like to try debugging this myself. Quick question: Is there a way to prevent the Javascript from being minified so that you can debug the frontend easily? Or are there more up-to-date debugging instructions than https://gerrit.googlesource.com/gerrit/+/master/polygerrit-ui/README.md?

[Frank Borden]

Hi, we have just converted the local dev server and the documentation is outdated. I am sending https://gerrit-review.googlesource.com/c/gerrit/+/343918 to update it.

In the mean time `yarn start` or `npm run start` should point to the new dev server instead of the previous go-based dev server.

[Marquis Wong]

Unfortunately, I think this is beyond my ability to debug.

I'm seeing very inconsistent behavior where:

* When I am running a local instance of Gerrit, the bug reproduces when I use http://myhostname:8080/ to access Gerrit.

* However, if I go via http://localhost:8080/ everything works fine.

The etc/gerrit.config has the former as the canonicalWebUrl.

Based on my Javascript console debugging, it seems like the event to create the draft comment is sent fine, but it just doesn't get rendered for some reason.

Any help would be appreciated. This bug is quite a problem for us, since it really interrupts the review workflow.

[Marquis Wong]

I made some progress!

The main problem is that GrDiffView#disconnectedCallback() calls this.cursor.dispose(), which unsubscribes the cursorManager, but connectedCallback() doesn't resubscribe, which means that the DiffViews lose track of what line is selected, so it doesn't know what line to leave a comment on.

I made a small change that adds a subscribe() method to GrDiffCursor, and made GrDiffView#connectedCallback() call it. 

subscribe() {
    this.targetSubscription = this.cursorManager.target$.subscribe(target => {
        this.diffRow = target || undefined;
   });
}

This seems to mostly do the trick. However, I am occasionally still able to cause it to lose track of what line is highlighted, so that when you hit 'c' nothing happens, but when I switch to a different file I discover that I've created 12 draft comments.

I've made https://bugs.chromium.org/p/gerrit/issues/detail?id=16216, and will update it with my findings.

[Ben Rohlfs]

Hi Marquis,

yeah, we had some issues around diff cursor and creating comments. Unfortunately, this issue is already fixed at head, and we had a fair bunch of changes affecting the diff directories:

https://gerrit-review.googlesource.com/q/project:gerrit+branch:master+directory:polygerrit-ui/app/embed/diff/+status:merged

Also, I was not able to reproduce your issue after checking out the stable-3.6 branch. Can you provide exact repro steps? Which browser are you using?

That said, these two changes could be candidates for cherrypicking:

338014: Fix diff row selectiong when diff view is loaded | https://gerrit-review.googlesource.com/c/gerrit/+/338014

338379: Rerender a diff if it's reconnected and avoid reusing GrDiffCursor. | https://gerrit-review.googlesource.com/c/gerrit/+/338379

But I am not sure how cleanly they would merge into 3.6. Anyway, need a repro before I can continue here.

-Ben

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/e08c27e6-1b3a-44ad-b27d-3b4f2b255bd7n%40googlegroups.com.

[Ben Rohlfs]

I think this should fix it: 344057: Rerender a diff if it's reconnected and avoid reusing GrDiffCursor | https://gerrit-review.googlesource.com/c/gerrit/+/344057

[Marquis Wong]

Thank you Ben!

I thought I was getting the same problem when running off of head, but I'm not sure. I will cherry-pick your fix and give it a try.

Do you still need a replication scenario? I can try to build a Docker image or similar that reproduces for me, and a video showing the reproduction in the UI.

I'm primarily working with Chrome and Safari on macOS Monterey 12.5.1. I've sanity checked that it is also broken on Windows with Chrome.

Marquis