Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR continues the cleanup of the chat status tooltip UI, primarily by restructuring quota indicator markup/styling and removing the “Agent Sessions” section from the dashboard.
Changes:
- Refactors quota indicator UI (new title/details layout, updated bar sizing, warning/error emphasis).
- Adjusts quota reset formatting (separate date/time formatters; “Resets …” label integrated into quota header).
- Removes agent sessions rendering and related service dependencies from the dashboard.
Show a summary per file
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/browser/chatStatus/media/chatStatus.css | Updates quota indicator styling/layout and adds warning/error visual treatments. |
| src/vs/workbench/contrib/chat/browser/chatStatus/chatStatusDashboard.ts | Simplifies dashboard dependencies, restructures quota indicator DOM, and updates reset/usage labeling logic. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 6
| const resetValue = $('span.quota-reset'); | ||
|
|
||
| if (resetLabel) { | ||
| resetValue.textContent = resetLabel; | ||
| } | ||
|
|
||
| const quotaIndicator = container.appendChild($('div.quota-indicator', undefined, | ||
| $('div.quota-label', undefined, | ||
| $('span', undefined, label), | ||
| quotaValue | ||
| $('div.quota-title', undefined, label), | ||
| $('div.quota-details', undefined, | ||
| $('div.quota-percentage', undefined, | ||
| quotaValue, | ||
| quotaValueSuffix | ||
| ), | ||
| resetValue | ||
| ), |
There was a problem hiding this comment.
resetValueis always inserted into the flex row, even whenresetLabelis undefined. In cases like the “Limited” string quota path (noresetLabelpassed), this extra empty flex item can affect spacing/justify-content behavior. Prefer to only create/append the reset element whenresetLabelis present, or hide it when empty (e.g., conditional DOM or a CSS:empty { display: none; }rule for.quota-reset`).
| container.appendChild($('div.description', undefined, overageLabel)); | ||
|
|
||
| if (supportsOverage && (this.chatEntitlementService.entitlement === ChatEntitlement.EDU || this.chatEntitlementService.entitlement === ChatEntitlement.Pro || this.chatEntitlementService.entitlement === ChatEntitlement.ProPlus)) { | ||
| const manageOverageButton = disposables.add(new Button(quotaIndicator, { ...defaultButtonStyles, secondary: true, hoverDelegate: nativeHoverDelegate })); | ||
| const manageOverageButton = disposables.add(new Button(container, { ...defaultButtonStyles, secondary: true, hoverDelegate: nativeHoverDelegate })); |
There was a problem hiding this comment.
The overage description row is appended unconditionally, even when supportsOverage is false (or when no overage messaging will be set), leaving an always-present wrapper around an often-empty overageLabel. This can introduce blank vertical space in the tooltip. Create/append that description row only when supportsOverage is true (or when you actually set content), or remove/hide the row when the label ends up empty."
| quotaValue.textContent = `+${this.quotaOverageFormatter.value.format(quota.overageCount)}`; | ||
| quotaValueSuffix.textContent = ` ${localize('quotaOverageRequests', "requests")}`; | ||
| } else { | ||
| quotaValue.textContent = localize('quotaDisplay', "{0}%", this.quotaPercentageFormatter.value.format(usedPercentage)); | ||
| quotaValueSuffix.textContent = ` ${localize('quotaUsed', "used")}`; | ||
| } |
There was a problem hiding this comment.
The UI string is being split across quotaValue and quotaValueSuffix using manual string concatenation and leading spaces. This is fragile for localization (ordering/spacing can differ, and some locales may require different grammar), and it makes it harder to translate correctly. Prefer a single localized message with placeholders that covers the full rendered text (even if you still style parts differently, you can keep the full localized string in an attribute for a11y, or use a single localize value and split more safely without hardcoded spaces)."
| const chatQuotaIndicator = chatQuota && (chatQuota.total > 0 || chatQuota.unlimited) ? this.createQuotaIndicator(container, this._store, chatQuota, localize('chatsLabel', "Chat messages"), false) : undefined; | ||
| const premiumChatLabel = premiumChatQuota?.overageEnabled && !premiumChatQuota?.unlimited ? localize('includedPremiumChatsLabel', "Included premium requests") : localize('premiumChatsLabel', "Premium requests"); | ||
| const premiumChatQuotaIndicator = premiumChatQuota && (premiumChatQuota.total > 0 || premiumChatQuota.unlimited) ? this.createQuotaIndicator(container, this._store, premiumChatQuota, premiumChatLabel, true) : undefined; | ||
| const resetLabel = resetDate ? (resetDateHasTime ? localize('quotaResetsAt', "Resets {0} at {1}", this.dateFormatter.value.format(new Date(resetDate)), this.timeFormatter.value.format(new Date(resetDate))) : localize('quotaResets', "Resets {0}", this.dateFormatter.value.format(new Date(resetDate)))) : undefined; |
There was a problem hiding this comment.
The resetLabel construction is duplicated in multiple places with the same localization keys and formatting. To keep future changes consistent (e.g., wording, date/time formatting, punctuation), factor this into a small helper method (e.g., getQuotaResetLabel() returning string | undefined) and reuse it."
See below for a potential fix:
private getQuotaResetLabel(resetDate: number | string | Date | undefined, resetDateHasTime: boolean): string | undefined {
if (!resetDate) {
return undefined;
}
const date = new Date(resetDate);
return resetDateHasTime
? localize('quotaResetsAt', "Resets {0} at {1}", this.dateFormatter.value.format(date), this.timeFormatter.value.format(date))
: localize('quotaResets', "Resets {0}", this.dateFormatter.value.format(date));
}
private renderUsageContent(container: HTMLElement, token: CancellationToken, updatePromise?: Promise<void>): void {
const { chat: chatQuota, completions: completionsQuota, premiumChat: premiumChatQuota, resetDate, resetDateHasTime } = this.chatEntitlementService.quotas;
if (chatQuota || premiumChatQuota) {
const resetLabel = this.getQuotaResetLabel(resetDate, resetDateHasTime);
| const { completions: completionsQuota, resetDate, resetDateHasTime } = this.chatEntitlementService.quotas; | ||
| const resetLabel = resetDate ? (resetDateHasTime ? localize('quotaResetsAt', "Resets {0} at {1}", this.dateFormatter.value.format(new Date(resetDate)), this.timeFormatter.value.format(new Date(resetDate))) : localize('quotaResets', "Resets {0}", this.dateFormatter.value.format(new Date(resetDate)))) : undefined; |
There was a problem hiding this comment.
The resetLabel construction is duplicated in multiple places with the same localization keys and formatting. To keep future changes consistent (e.g., wording, date/time formatting, punctuation), factor this into a small helper method (e.g., getQuotaResetLabel() returning string | undefined) and reuse it."
See below for a potential fix:
private getQuotaResetLabel(resetDate: number | undefined, resetDateHasTime: boolean | undefined): string | undefined {
if (!resetDate) {
return undefined;
}
const date = new Date(resetDate);
return resetDateHasTime
? localize('quotaResetsAt', "Resets {0} at {1}", this.dateFormatter.value.format(date), this.timeFormatter.value.format(date))
: localize('quotaResets', "Resets {0}", this.dateFormatter.value.format(date));
}
private renderInlineSuggestionsContent(container: HTMLElement, token: CancellationToken, updatePromise?: Promise<void>): void {
// Completions quota
{
const { completions: completionsQuota, resetDate, resetDateHasTime } = this.chatEntitlementService.quotas;
const resetLabel = this.getQuotaResetLabel(resetDate, resetDateHasTime);
| if (supportsOverage && (this.chatEntitlementService.entitlement === ChatEntitlement.EDU || this.chatEntitlementService.entitlement === ChatEntitlement.Pro || this.chatEntitlementService.entitlement === ChatEntitlement.ProPlus)) { | ||
| const manageOverageButton = disposables.add(new Button(quotaIndicator, { ...defaultButtonStyles, secondary: true, hoverDelegate: nativeHoverDelegate })); | ||
| const manageOverageButton = disposables.add(new Button(container, { ...defaultButtonStyles, secondary: true, hoverDelegate: nativeHoverDelegate })); | ||
| manageOverageButton.label = localize('enableAdditionalUsage', "Manage paid premium requests"); |
There was a problem hiding this comment.
The “Manage paid premium requests” button is now attached to container rather than the quota indicator block (quotaIndicator). This can make the layout/order harder to reason about when multiple quota indicators are rendered (button may visually group with the wrong indicator depending on surrounding DOM/CSS). Consider appending the button to the specific quota indicator (or to a dedicated actions container inside it) so the DOM structure matches the visual grouping."
No description provided.