-
Notifications
You must be signed in to change notification settings - Fork 29.4k
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
support watch task reconnection #155120
support watch task reconnection #155120
Conversation
@@ -167,6 +167,8 @@ export interface IPtyHostAttachTarget { | |||
icon: TerminalIcon | undefined; | |||
fixedDimensions: IFixedTerminalDimensions | undefined; | |||
environmentVariableCollections: ISerializableEnvironmentVariableCollections | undefined; | |||
reconnectionOwner?: string; | |||
task?: { label: string; id: string; lastTask?: string; group?: string }; |
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.
Can this be a reconnection owner id or something and this data gets persisted by contrib/tasks? We don't want to mention tasks anywhere in terminal ideally
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.
This?
reconnectionOwner: {
ownerName: string;
ownerId: number;
}
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.
If that's not easily doable, a temporary fix is to give this a generic name with type unknown
and validate the type in tasks. I'd be fine for that to live in terminal as it basically moves the tech debt out of terminal and into tasks.
@@ -347,7 +364,7 @@ export abstract class AbstractTaskService extends Disposable implements ITaskSer | |||
return this.inTerminal(); | |||
} | |||
|
|||
private _registerCommands(): void { | |||
private async _registerCommands(): Promise<void> { |
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.
Can revert this?
This PR fixes #117408
To do/ might defer: