-
Notifications
You must be signed in to change notification settings - Fork 464
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.
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
Refactoring connection management #18684
base: main
Are you sure you want to change the base?
Conversation
Benjin
commented
Feb 14, 2025
- Lays the groundwork for connection grouping
- Fixes an issue where similar connections could trample each other in the Object Explorer
- cleans up a bunch of duplicate or poorly-named code in connection management
PR Changes
|
@@ -872,40 +869,30 @@ export class ObjectExplorerService { | |||
isDisconnect: boolean = false, | |||
): Promise<void> { | |||
await this.closeSession(node); | |||
const nodeUri = ObjectExplorerUtils.getNodeUri(node); | |||
const nodeUri = this.getNodeIdentifier(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update ObjectExplorerUtils.getNodeUri()
?
Do we have an issue for this? Would love to understand how "similar" two connections need to be to trigger this isse, maybe a before/after screenshot with the fix? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, I'd say we might need do more manual testings next week just to make sure this change won't break any existing OE experience.
Agreed. I really wanted to get this in on the earlier side of a release cycle, but too many things were competing with my time when I wanted to get this PR out. The right call could be to hold this PR after the release branch splits so we can decide whether to take it as a port. |