Hi @mattbierner,

Thanks for the review!

I'm currently wondering about the failing unit test. It seems there was this code in remoteIndicator.ts which caused this issue #142204 :

quickPick.onDidHide(itemUpdater.dispose);

Now this code doesn't exist anymore and it also seems a bit odd to me that someone would call dispose with undefined as this (except as a mistake maybe?).

Do you think the failing test can be removed? Thanks again.

Thanks, yes I think we should remove that test. Our other disposable types (Disposable, DisposableStore) would not work with this pattern. I don't think it makes sense to support it for toDisposable only

Unsure about why the CI checks are failing. Re-running after merging to see if they are transient failures or need more investigation

Hm. Looks like it's always failing due to chat errors. I will try to see if I can reproduce the errors locally and maybe solve them.

I checked and it's indeed the same error that also was in the unit test with the unbound this.

Typescript doesn't catch these errors, but I checked with the typescript eslint rule @typescript-eslint/unbound-method, which showed ~480 unbound method errors.

Still needeing to track down further which of these 480 errors are relevant and which are not.

Ugh sorry this ended up being annoying and thanks for all the great follow up

I found one very obvious case that could have caused the failure here: #268202. Will see if I spot any others. Hopefully we can avoid having to update all 480 locations as many are false positives

Thanks very much! The tests are passing now. It might just have been that one occurrence. I'd say this is ready to merge now - but with a small chance of potentially breaking something. :)