Skip to content
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.

Sign up for GitHub

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

Adds indicator in console when extension host is disconnected #6089

Merged
merged 8 commits into from
Jan 29, 2025
Merged

Adds indicator in console when extension host is disconnected #6089

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8b355f8
Adds indicator in console when extension host is disconnected
samclark2015 Jan 22, 2025
975e426
Merge remote-tracking branch 'origin/main' into feature/console-exten…
samclark2015 Jan 22, 2025
f6bcae0
Adds logic to save prompt input during extension host restarts
samclark2015 Jan 23, 2025
1489360
Merge remote-tracking branch 'origin/main' into feature/console-exten…
samclark2015 Jan 23, 2025
3b79f24
Merge remote-tracking branch 'origin/main' into feature/console-exten…
samclark2015 Jan 24, 2025
88c47ba
Fixes inefficient calculation of condition for console extension host…
samclark2015 Jan 27, 2025
cf1067a
Adds localization for console extension host indicator
samclark2015 Jan 27, 2025
3dd0b6d
Adds back appropriate message when user initiates runtime shutdown
samclark2015 Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/vs/workbench/contrib/positronConsole/browser/components/consoleInput.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,9 @@
padding-bottom: 10px;
position: relative;
}


.console-input.hidden {
display: none;
background-color: red;
}
3 changes: 2 additions & 1 deletion src/vs/workbench/contrib/positronConsole/browser/components/consoleInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ type ILineNumbersOptions = Pick<IEditorOptions, 'lineNumbers' | 'lineNumbersMinC
// ConsoleInputProps interface.
interface ConsoleInputProps {
readonly width: number;
readonly hidden: boolean;
readonly positronConsoleInstance: IPositronConsoleInstance;
readonly onSelectAll: () => void;
readonly onCodeExecuted: () => void;
Expand Down Expand Up @@ -960,7 +961,7 @@ export const ConsoleInput = (props: ConsoleInputProps) => {

// Render.
return (
<div className='console-input' tabIndex={0} onFocus={focusHandler}>
<div className={props.hidden ? 'console-input hidden' : 'console-input'} tabIndex={0} onFocus={focusHandler}>
<div ref={codeEditorWidgetContainerRef} />
{historyBrowserActive &&
<HistoryBrowserPopup
Expand Down
7 changes: 7 additions & 0 deletions src/vs/workbench/contrib/positronConsole/browser/components/consoleInstance.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,10 @@
.console-core .console-instance:focus {
outline: none !important;
}

.console-core .console-instance .console-instance-container .console-item-reconnecting {
display: flex;
flex-direction: row;
justify-items: center;
gap: 0.25rem;
}
47 changes: 33 additions & 14 deletions src/vs/workbench/contrib/positronConsole/browser/components/consoleInstanceItems.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import React, { Component } from 'react';
import { FontInfo } from '../../../../../editor/common/config/fontInfo.js';
import { ConsoleInput } from './consoleInput.js';
import { RuntimeTrace } from './runtimeTrace.js';
import { RuntimeExited } from './runtimeExited.js';
import { RuntimeStartup } from './runtimeStartup.js';
import { RuntimeStarted } from './runtimeStarted.js';
import { RuntimeOffline } from './runtimeOffline.js';
Expand All @@ -25,7 +24,6 @@ import { RuntimeItemExited } from '../../../../services/positronConsole/browser/
import { RuntimeItemStartup } from '../../../../services/positronConsole/browser/classes/runtimeItemStartup.js';
import { RuntimeItemStarted } from '../../../../services/positronConsole/browser/classes/runtimeItemStarted.js';
import { RuntimeItemOffline } from '../../../../services/positronConsole/browser/classes/runtimeItemOffline.js';
import { RuntimeReconnected } from './runtimeReconnected.js';
import { RuntimeItemStarting } from '../../../../services/positronConsole/browser/classes/runtimeItemStarting.js';
import { RuntimeItemActivity } from '../../../../services/positronConsole/browser/classes/runtimeItemActivity.js';
import { RuntimePendingInput } from './runtimePendingInput.js';
Expand Down Expand Up @@ -65,6 +63,20 @@ export class ConsoleInstanceItems extends Component<ConsoleInstanceItemsProps> {
* @returns The rendered component.
*/
override render() {

let extensionHostDisconnected = false;

const reversedItems = this.props.positronConsoleInstance.runtimeItems.slice().reverse();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can be a lot of runtime items, so the copy made by slice() could be expensive (which matters since this runs on every render). I would recommend just adding a flag we can check. If we do need a loop we could just index backwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified the check to set this flag, and moved it within the existing map() call; same effect, but without a copy or secondary iteration.

for (const item of reversedItems) {
if (item instanceof RuntimeItemStartup) {
break;
} else if (item instanceof RuntimeItemExited && item.reason == "extensionHost") {
extensionHostDisconnected = true;
break;
}

}

return (
<>
<div className='top-spacer' />
Expand All @@ -76,15 +88,15 @@ export class ConsoleInstanceItems extends Component<ConsoleInstanceItemsProps> {
} else if (runtimeItem instanceof RuntimeItemStartup) {
return <RuntimeStartup key={runtimeItem.id} runtimeItemStartup={runtimeItem} />;
} else if (runtimeItem instanceof RuntimeItemReconnected) {
return <RuntimeReconnected key={runtimeItem.id} runtimeItemReconnected={runtimeItem} />;
return null;
} else if (runtimeItem instanceof RuntimeItemStarting) {
return <RuntimeStarting key={runtimeItem.id} runtimeItemStarting={runtimeItem} />;
} else if (runtimeItem instanceof RuntimeItemStarted) {
return <RuntimeStarted key={runtimeItem.id} runtimeItemStarted={runtimeItem} />;
} else if (runtimeItem instanceof RuntimeItemOffline) {
return <RuntimeOffline key={runtimeItem.id} runtimeItemOffline={runtimeItem} />;
} else if (runtimeItem instanceof RuntimeItemExited) {
return <RuntimeExited key={runtimeItem.id} runtimeItemExited={runtimeItem} />;
return null;
} else if (runtimeItem instanceof RuntimeItemRestartButton) {
return <RuntimeRestartButton key={runtimeItem.id} runtimeItemRestartButton={runtimeItem} positronConsoleInstance={this.props.positronConsoleInstance} />;
} else if (runtimeItem instanceof RuntimeItemStartupFailure) {
Expand All @@ -96,17 +108,24 @@ export class ConsoleInstanceItems extends Component<ConsoleInstanceItemsProps> {
return null;
}
})}
{!this.props.positronConsoleInstance.promptActive && this.props.runtimeAttached &&
<ConsoleInput
width={this.props.consoleInputWidth}
positronConsoleInstance={this.props.positronConsoleInstance}
onSelectAll={this.props.onSelectAll}
onCodeExecuted={() =>
// Update the component to eliminate flickering.
flushSync(() => this.forceUpdate()
)}
/>
{extensionHostDisconnected ?
(<div className='console-item-reconnecting'>
<span className='codicon codicon-loading codicon-modifier-spin'></span>
<span>Extensions restarting...</span>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This string needs to be localized.

Copy link
Contributor Author

@samclark2015 samclark2015 Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the localization in the React component, but want to confirm no l10n properties need to be defined elsewhere (e.g., map the key to a string outside the source code). I searched the codebase for several other l10n keys and didn't find references outside the components.

</div>) :
null
}

<ConsoleInput
hidden={this.props.positronConsoleInstance.promptActive || !this.props.runtimeAttached}
width={this.props.consoleInputWidth}
positronConsoleInstance={this.props.positronConsoleInstance}
onSelectAll={this.props.onSelectAll}
onCodeExecuted={() =>
// Update the component to eliminate flickering.
flushSync(() => this.forceUpdate()
)}
/>
</>
);
}
Expand Down
Loading