DomWidget / ChatStatus cleanup #278860
DomWidget / ChatStatus cleanup #278860
Conversation
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the ChatStatusDashboard class into a separate file and introduces a new DomWidget base class to provide a standard pattern for reusable UI components with hot module replacement support.
Key Changes:
- Extracted
ChatStatusDashboardfromchatStatus.tsinto a new dedicated filechatStatusDashboard.ts - Introduced new
DomWidgetabstract class insrc/vs/platform/ui/browser/domWidget.tsas a base for reusable UI components - Updated
chatStatus.tsto use the newDomWidgetpattern withinstantiateInContents()instead of the lazyLazy<>pattern - Modified Vite build configuration to automatically inject hot reload support for
DomWidgetclasses
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/browser/chatStatusDashboard.ts | New file containing the extracted ChatStatusDashboard class that now extends DomWidget and renders immediately in constructor |
| src/vs/workbench/contrib/chat/browser/chatStatus.ts | Refactored to remove the inlined ChatStatusDashboard class, export utility functions, and use DomWidget.instantiateInContents() for tooltip creation |
| src/vs/platform/ui/browser/domWidget.ts | New base class providing standard patterns for disposable UI widgets with static helpers for instantiation and hot reload support |
| build/vite/vite.config.ts | Enhanced hot reload plugin to detect and inject hot replacement code for classes extending DomWidget |
|
|
||
| if (hasDomWidget) { | ||
| const matches = code.matchAll(/class\s+([a-zA-Z0-9_]+)\s+extends\s+DomWidget/g); | ||
| /// @ts-ignore |
There was a problem hiding this comment.
The /// @ts-ignore comment should be replaced with // @ts-expect-error for better type checking. When using @ts-expect-error, TypeScript will error if the suppression becomes unnecessary, helping maintain code quality over time.
| /// @ts-ignore | |
| // @ts-expect-error |
| timerDisposables.add(disposableWindowInterval( | ||
| getWindow(container), | ||
| () => update(enabled), | ||
| 1000 |
There was a problem hiding this comment.
The interval value should use a numeric separator for consistency with other parts of the codebase: 1_000 instead of 1000. This improves readability for millisecond values.
| 1000 | |
| 1_000 |
| } | ||
|
|
||
| if (hasDomWidget) { | ||
| const matches = code.matchAll(/class\s+([a-zA-Z0-9_]+)\s+extends\s+DomWidget/g); |
There was a problem hiding this comment.
The regex pattern /class\s+([a-zA-Z0-9_]+)\s+extends\s+DomWidget/g may not correctly handle all cases. It won't match classes that have TypeScript generic parameters (e.g., class MyWidget<T> extends DomWidget) or those with multiple spaces/line breaks. Consider using a more robust pattern or AST-based parsing for reliability.
| const matches = code.matchAll(/class\s+([a-zA-Z0-9_]+)\s+extends\s+DomWidget/g); | |
| const matches = code.matchAll(/class\s+([a-zA-Z0-9_]+)\s*(<[^>]*>)?\s*extends\s*DomWidget/smg); |
| const elem = ChatStatusDashboard.instantiateInContents(this.instantiationService, store); | ||
|
|
||
| // todo@connor4312/@benibenj: workaround for #257923 | ||
| store.add(disposableWindowInterval(mainWindow, () => { |
There was a problem hiding this comment.
Can we please avoid a hack like this, why is it now needed when it was not before?
| }; | ||
| tooltip: { | ||
| element: (token: CancellationToken) => { | ||
| const store = new DisposableStore(); |
There was a problem hiding this comment.
This block would probably read better if it was extracted into a method?
| import { isHotReloadEnabled } from '../../../base/common/hotReload.js'; | ||
| import { Disposable, DisposableStore, toDisposable } from '../../../base/common/lifecycle.js'; | ||
| import { ISettableObservable, IObservable, autorun, constObservable, derived, observableValue } from '../../../base/common/observable.js'; | ||
| import { IInstantiationService, GetLeadingNonServiceArgs } from '../../instantiation/common/instantiation.js'; |
There was a problem hiding this comment.
I find platform/ui not very specific, lets try to find folder names in platform that are explanatory of what is going on inside. The original intent was to make it easy to understand what kind of service is provided.
| import { defaultChat, canUseChat, isNewUser, isCompletionsEnabled } from './chatStatus.js'; | ||
| import { IChatStatusItemService, ChatStatusEntry } from './chatStatusItemService.js'; | ||
|
|
||
| interface ISettingsAccessor { |
There was a problem hiding this comment.
If you are splitting up chat status, then why not introduce a new folder status in contrib/chat to make this intent really clear?
No description provided.