refactor: address announcement banner review notes

Centralize status bar item names and priorities in util/statusBar while
preserving each item's pre-registry id and placement. Read the
deprecated service_banner only when announcement_banners is absent so
modern deployments that mirror the first announcement into it show no
duplicates, and derive banner keys from the message alone. Union seen
keys instead of overwriting, mark banners seen only when the user could
see them, and guard overlapping fetches with a generation counter so
stale responses never apply. Open the picker from the popup without
refetching, show the empty-state message only after a successful
refresh, avoid awaiting non-modal notifications, truncate popup text by
code points, and align tests with the setup() factory pattern.

Author: EhabY

src/announcements/banners.ts

@@ -7,51 +7,31 @@ import type {
 
 const POPUP_MESSAGE_MAX_LENGTH = 120;
 
-export type AnnouncementSource = "announcement" | "service";
-
 export interface Announcement {
-	readonly source: AnnouncementSource;
 	readonly message: string;
-	readonly backgroundColor?: string;
+	/** Stable fingerprint used to track which banners were already seen. */
 	readonly key: string;
 }
 
 export function normalizeBanners(
 	appearance: AppearanceConfig,
 ): readonly Announcement[] {
-	return [
-		toAnnouncement("service", appearance.service_banner),
-		// Nullish guards tolerate older deployments that omit banner fields.
-		...(appearance.announcement_banners ?? []).map((banner) =>
-			toAnnouncement("announcement", banner),
-		),
-	].filter((banner): banner is Announcement => banner !== undefined);
-}
-
-export function bannerKey(
-	banner: Pick<Announcement, "source" | "message" | "backgroundColor">,
-): string {
-	return createHash("sha256")
-		.update(
-			JSON.stringify({
-				source: banner.source,
-				message: banner.message,
-				backgroundColor: banner.backgroundColor ?? "",
-			}),
-		)
-		.digest("hex")
-		.slice(0, 16);
+	// Modern servers mirror announcements[0] into the deprecated service_banner.
+	const banners = appearance.announcement_banners ?? [
+		appearance.service_banner,
+	];
+	return banners
+		.map((banner) => toAnnouncement(banner))
+		.filter((banner): banner is Announcement => banner !== undefined);
 }
 
 export function statusText(count: number): string {
-	return count === 1 ? "$(megaphone) Coder" : `$(megaphone) Coder ${count}`;
+	return `$(megaphone) Coder${count === 1 ? "" : ` ${count}`}`;
 }
 
 export function statusTooltip(banners: readonly Announcement[]): string {
 	return [
-		banners.length === 1
-			? "Coder deployment announcement"
-			: "Coder deployment announcements",
+		`Coder deployment announcement${banners.length === 1 ? "" : "s"}`,
 		"",
 		...banners.map((banner, index) => `${index + 1}. ${banner.message}`),
 	].join("\n");
@@ -64,24 +44,23 @@ export function popupMessage(banners: readonly Announcement[]): string {
 }
 
 function toAnnouncement(
-	source: AnnouncementSource,
 	banner: BannerConfig | undefined,
 ): Announcement | undefined {
 	const message = banner?.message?.trim();
 	if (!banner?.enabled || !message) {
 		return undefined;
 	}
-	const backgroundColor = banner.background_color?.trim() || undefined;
-	return {
-		source,
-		message,
-		backgroundColor,
-		key: bannerKey({ source, message, backgroundColor }),
-	};
+	return { message, key: bannerKey(message) };
+}
+
+function bannerKey(message: string): string {
+	return createHash("sha256").update(message).digest("hex").slice(0, 16);
 }
 
 function truncate(message: string): string {
-	return message.length <= POPUP_MESSAGE_MAX_LENGTH
+	// Slice code points so emoji are never cut in half.
+	const chars = [...message];
+	return chars.length <= POPUP_MESSAGE_MAX_LENGTH
 		? message
-		: `${message.slice(0, POPUP_MESSAGE_MAX_LENGTH - 1)}…`;
+		: `${chars.slice(0, POPUP_MESSAGE_MAX_LENGTH - 1).join("")}…`;
 }

src/announcements/manager.ts

@@ -1,11 +1,8 @@
 import * as vscode from "vscode";
 
 import { errToStr } from "../api/api-helper";
-import { type CoderApi } from "../api/coderApi";
-import { type SecretsManager } from "../core/secretsManager";
-import { type SessionState } from "../deployment/sessionStore";
-import { type Logger } from "../logging/logger";
 import { areNotificationsDisabled } from "../settings/notifications";
+import { createStatusBarItem } from "../util/statusBar";
 
 import {
 	type Announcement,
@@ -15,7 +12,12 @@ import {
 	statusTooltip,
 } from "./banners";
 
-const REFRESH_INTERVAL_MS = 30 * 60 * 1000;
+import type { CoderApi } from "../api/coderApi";
+import type { SecretsManager } from "../core/secretsManager";
+import type { SessionState } from "../deployment/sessionStore";
+import type { Logger } from "../logging/logger";
+
+export const REFRESH_INTERVAL_MS = 30 * 60 * 1000;
 const VIEW_ACTION = "View";
 
 interface RefreshOptions {
@@ -24,9 +26,10 @@ interface RefreshOptions {
 }
 
 export class AnnouncementManager implements vscode.Disposable {
-	private readonly statusBarItem: vscode.StatusBarItem;
+	private readonly statusBarItem = createStatusBarItem("announcements");
 	private readonly sessionChangeDisposable: vscode.Disposable;
-	private banners: readonly Announcement[] = [];
+	#banners: readonly Announcement[] = [];
+	private fetchGeneration = 0;
 	private refreshTimeout: NodeJS.Timeout | undefined;
 	private disposed = false;
 
@@ -36,119 +39,112 @@ export class AnnouncementManager implements vscode.Disposable {
 		private readonly secretsManager: SecretsManager,
 		private readonly logger: Logger,
 	) {
-		this.statusBarItem = vscode.window.createStatusBarItem(
-			vscode.StatusBarAlignment.Left,
-			998,
-		);
-		this.statusBarItem.name = "Coder Announcements";
 		this.statusBarItem.command = "coder.viewAnnouncements";
 		this.sessionChangeDisposable = this.sessionState.onDidChange(() => {
 			this.onSessionChange();
 		});
 		this.onSessionChange();
 	}
 
-	public dispose(): void {
-		this.disposed = true;
-		this.cancelRefresh();
-		this.sessionChangeDisposable.dispose();
-		this.statusBarItem.dispose();
-	}
-
-	public async refresh(
-		options: RefreshOptions = {},
-	): Promise<readonly Announcement[] | undefined> {
+	/** Refreshes the banners; resolves false when the refresh failed. */
+	public async refresh(options: RefreshOptions = {}): Promise<boolean> {
 		if (this.disposed) {
-			return undefined;
+			return false;
 		}
 		this.cancelRefresh();
 		try {
-			return await this.fetch(options);
+			await this.fetch(options);
+			return true;
 		} catch (error) {
 			this.logger.warn("Failed to refresh Coder announcements", error);
 			if (options.showErrors) {
 				void vscode.window.showErrorMessage(
 					`Failed to refresh Coder announcements: ${errToStr(error)}`,
 				);
 			}
-			return undefined;
+			return false;
 		} finally {
 			this.scheduleRefresh();
 		}
 	}
 
 	public async showAnnouncements(): Promise<void> {
-		const banners =
-			(await this.refresh({ notify: false, showErrors: true })) ?? this.banners;
-		if (banners.length === 0) {
+		const refreshed = await this.refresh({ notify: false, showErrors: true });
+		if (this.banners.length > 0) {
+			await this.pickAnnouncement();
+		} else if (refreshed) {
+			// On failure the error popup is the whole story.
 			void vscode.window.showInformationMessage(
 				"No active Coder announcements.",
 			);
-			return;
 		}
+	}
 
-		const selected = await vscode.window.showQuickPick(
-			banners.map((banner, index) => ({
-				label: `${banner.source === "service" ? "$(info) Service banner" : "$(megaphone) Announcement"} ${index + 1}`,
-				detail: banner.message,
-				banner,
-			})),
-			{
-				title: "Coder Announcements",
-				placeHolder: "Select an announcement to view the full message",
-			},
-		);
-		if (selected) {
-			void vscode.window.showInformationMessage(selected.banner.message);
-		}
+	public dispose(): void {
+		this.disposed = true;
+		this.cancelRefresh();
+		this.sessionChangeDisposable.dispose();
+		this.banners = [];
+		this.statusBarItem.dispose();
 	}
 
 	private onSessionChange(): void {
 		this.cancelRefresh();
-		this.setBanners([]);
+		this.banners = [];
 		if (this.sessionState.current.kind === "signedIn") {
 			void this.refresh({ notify: true });
 		}
 	}
 
-	private async fetch(
-		options: RefreshOptions,
-	): Promise<readonly Announcement[] | undefined> {
+	private async fetch(options: RefreshOptions): Promise<void> {
 		const session = this.sessionState.current;
 		if (session.kind !== "signedIn") {
-			this.setBanners([]);
-			return [];
+			this.banners = [];
+			return;
 		}
 
-		const banners = normalizeBanners(await this.client.getAppearance());
-		if (this.disposed || this.sessionState.current !== session) {
-			return undefined;
+		const generation = ++this.fetchGeneration;
+		const appearance = await this.client.getAppearance();
+		// A newer fetch or a session change supersedes this response.
+		if (
+			this.disposed ||
+			generation !== this.fetchGeneration ||
+			this.sessionState.current !== session
+		) {
+			return;
 		}
-		this.setBanners(banners);
+		const banners = normalizeBanners(appearance);
+		this.banners = banners;
 
 		const seen = new Set(
 			this.secretsManager.getSeenBanners(session.deployment.safeHostname),
 		);
-		const keys = banners.map((banner) => banner.key);
 		const unseen = banners.filter((banner) => !seen.has(banner.key));
-		if (
-			options.notify &&
-			unseen.length > 0 &&
-			!areNotificationsDisabled(vscode.workspace.getConfiguration())
-		) {
-			void this.showPopup(unseen);
+		if (unseen.length === 0) {
+			return;
 		}
-		if (unseen.length > 0 || seen.size !== new Set(keys).size) {
+		const notifiable = !areNotificationsDisabled(
+			vscode.workspace.getConfiguration(),
+		);
+		if (options.notify && notifiable) {
+			this.showPopup(unseen);
+		}
+		// Mark seen only what the user could see; suppressed banners notify later.
+		if (!options.notify || notifiable) {
 			await this.secretsManager.setSeenBanners(
 				session.deployment.safeHostname,
-				keys,
+				[...seen, ...unseen.map((banner) => banner.key)],
 			);
 		}
-		return banners;
 	}
 
-	private setBanners(banners: readonly Announcement[]): void {
-		this.banners = banners;
+	/** The setter syncs the status bar so the two cannot drift apart. */
+	private get banners(): readonly Announcement[] {
+		return this.#banners;
+	}
+
+	private set banners(banners: readonly Announcement[]) {
+		this.#banners = banners;
 		if (banners.length === 0) {
 			this.statusBarItem.hide();
 			return;
@@ -158,17 +154,34 @@ export class AnnouncementManager implements vscode.Disposable {
 		this.statusBarItem.show();
 	}
 
-	private async showPopup(banners: readonly Announcement[]): Promise<void> {
-		try {
-			const action = await vscode.window.showInformationMessage(
-				popupMessage(banners),
-				VIEW_ACTION,
-			);
-			if (action === VIEW_ACTION) {
-				void this.showAnnouncements();
-			}
-		} catch (error) {
-			this.logger.warn("Failed to show Coder announcement popup", error);
+	/** Non-modal messages may never settle, so chain instead of awaiting. */
+	private showPopup(banners: readonly Announcement[]): void {
+		Promise.resolve(
+			vscode.window.showInformationMessage(popupMessage(banners), VIEW_ACTION),
+		)
+			.then((action) =>
+				// Banners are seconds old; skip the refetch.
+				action === VIEW_ACTION ? this.pickAnnouncement() : undefined,
+			)
+			.catch((error) => {
+				this.logger.warn("Failed to show Coder announcement popup", error);
+			});
+	}
+
+	private async pickAnnouncement(): Promise<void> {
+		const selected = await vscode.window.showQuickPick(
+			this.banners.map((banner, index) => ({
+				label: `$(megaphone) Announcement ${index + 1}`,
+				detail: banner.message,
+				banner,
+			})),
+			{
+				title: "Coder Announcements",
+				placeHolder: "Select an announcement to view the full message",
+			},
+		);
+		if (selected) {
+			void vscode.window.showInformationMessage(selected.banner.message);
 		}
 	}
 

src/remote/remote.ts

@@ -43,6 +43,7 @@ import {
 	type AuthorityParts,
 	parseRemoteAuthority,
 } from "../util/authority";
+import { createStatusBarItem } from "../util/statusBar";
 import { vscodeProposed } from "../vscodeProposed";
 import { WorkspaceMonitor } from "../workspace/workspaceMonitor";
 
@@ -1080,10 +1081,7 @@ export class Remote {
 		agent: WorkspaceAgent,
 		client: CoderApi,
 	): Promise<vscode.Disposable[]> {
-		const statusBarItem = vscode.window.createStatusBarItem(
-			"agentMetadata",
-			vscode.StatusBarAlignment.Left,
-		);
+		const statusBarItem = createStatusBarItem("agentMetadata");
 
 		const agentWatcher = await createAgentMetadataWatcher(agent.id, client);
 

src/remote/sshProcess.ts

@@ -6,6 +6,7 @@ import * as vscode from "vscode";
 import { SshTelemetry, type ProcessLossCause } from "../instrumentation/ssh";
 import { findPort } from "../util";
 import { cleanupFiles } from "../util/fileCleanup";
+import { createStatusBarItem } from "../util/statusBar";
 
 import { NetworkStatusReporter } from "./networkStatus";
 import {
@@ -140,10 +141,7 @@ export class SshProcessMonitor implements vscode.Disposable {
 			networkPollInterval: options.networkPollInterval ?? 3000,
 		};
 		this.telemetry = new SshTelemetry(options.telemetry);
-		this.statusBarItem = vscode.window.createStatusBarItem(
-			vscode.StatusBarAlignment.Left,
-			1000,
-		);
+		this.statusBarItem = createStatusBarItem("networkStatus");
 		this.reporter = new NetworkStatusReporter(this.statusBarItem);
 	}