ReviewZoneWidget makes duplicate calls to show() + hide()

[hermannloose]

Does this issue occur when all extensions are disabled?: Yes

• VS Code Version: 1.76.0
• OS Version: macOS

Steps to Reproduce:

1. Click on a line gutter icon for an expanded comment thread.
2. ReviewZoneWidget.toggleExpand() sets the comment thread's collapsibleState to Collapsed:

   vscode/src/vs/workbench/contrib/comments/browser/commentThreadZoneWidget.ts

   Line 278 in e282742

   this._commentThread.collapsibleState = languages.CommentThreadCollapsibleState.Collapsed;

3. This causes the widget's listener on the thread's collapsible state to fire synchronously and call hide():

   vscode/src/vs/workbench/contrib/comments/browser/commentThreadZoneWidget.ts

   Line 377 in e282742

   this.hide();

4. toggleExpand() follows this up with an explicit, duplicate call to hide():

   vscode/src/vs/workbench/contrib/comments/browser/commentThreadZoneWidget.ts

   Line 279 in e282742

   this.hide();

For the other direction, the problem is the same and show() is called twice with identical parameters on every change.

I am not sure if these duplicate calls cause unnecessary rendering or similar elsewhere in the stack, but they seem redundant. I think relying on the listener for onDidChangeCollapsibleState is probably sufficient, but I am not that familiar enough with the code base to have a confident opinion here.

[VSCodeTriageBot]

Thanks for creating this issue! It looks like you may be using an old version of VS Code, the latest stable release is 1.75.1. Please try upgrading to the latest version and checking whether this issue remains.

Happy Coding!

[hermannloose]

Thanks triage bot, but I wish you handled "freshly built from HEAD" a bit better!

[hermannloose]

Looking a bit further, it seems to be the case that ZoneWidget.show() through ZoneWidget._showImp() does indeed discard and recreate the overlay widget unconditionally, causing ReviewZoneWidget._doLayout() to run, which calls CommentThreadWidget.layout(). However ReviewZoneWidget.show() also explicitly calls its own _refresh() method which makes another call to CommentThreadWidget.layout(), so in total for every click to expand a comment thread in the line gutter, the corresponding CommentThreadWidget runs through four layout() passes.

[hermannloose]

Hm, it actually seems a bit worse still regarding the number of calls to CommentThreadWidget.layout() (although a few of them return pretty instantly): ReviewZoneWidget.show() also sets collapsibleState, so that listener fires again; within ReviewZoneWidget it's a no-op, but it causes another layout() call, so for every call to ReviewZoneWidget.show(), CommentThreadWidget.layout() is called four times, and for every click on the line gutter icon to show the thread, it's called eight times; actually nine, but I haven't looked yet where that last call comes from.

This is not a performance problem right now but it seems like a spot with potential to multiply any future slowdown on the layout path.

[hermannloose]

For the slightly excessive firing of onDidChangeCollapsibleState I wonder if the right approach would be to suppress events currently fired for setting collapsibleState to the value it already has:

vscode/src/vs/workbench/api/browser/mainThreadComments.ts

Line 113 in e282742

this._onDidChangeCollapsibleState.fire(this._collapsibleState);