From 709f04ee451fd77ed222b29b5d7f5b1a8ab08c3e Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Mon, 23 Feb 2026 14:51:26 -0500 Subject: [PATCH 01/15] set up ResourceContainer abstract base class --- src/models/containers/resourceContainer.ts | 125 +++++++++++++++++++++ 1 file changed, 125 insertions(+) create mode 100644 src/models/containers/resourceContainer.ts diff --git a/src/models/containers/resourceContainer.ts b/src/models/containers/resourceContainer.ts new file mode 100644 index 000000000..b64ffd3d5 --- /dev/null +++ b/src/models/containers/resourceContainer.ts @@ -0,0 +1,125 @@ +import { ThemeIcon, TreeItem, TreeItemCollapsibleState } from "vscode"; +import { ERROR_ICON, IconNames } from "../../icons"; +import { Logger } from "../../logging"; +import type { ISearchable } from "../resource"; + +/** Poll interval to use when waiting for a container to finish loading. */ +export const LOADING_POLL_INTERVAL_MS = 100; + +/** + * Abstract base class for container {@link TreeItem tree items} that manage an array of resources + * with shared loading, error, and children state. + */ +export abstract class ResourceContainer + extends TreeItem + implements ISearchable +{ + // enforce string so subclasses set this after super() + declare id: string; + + abstract loggerName: string; + + private _children: T[]; + + private _isLoading: boolean = false; + private _hasError: boolean = false; + protected readonly _defaultContextValue: string | undefined; + protected readonly _defaultIcon: ThemeIcon | undefined; + + protected constructor(label: string, children: T[], contextValue?: string, icon?: ThemeIcon) { + super(label, TreeItemCollapsibleState.Collapsed); + + this._children = children; + + this._defaultContextValue = contextValue; + if (contextValue) { + this.contextValue = contextValue; + } + this._defaultIcon = icon; + this.iconPath = this._defaultIcon; + } + + // lazy because loggerName is abstract and not available during super() / constructor time + private _logger?: Logger; + private get logger(): Logger { + if (!this._logger) { + this._logger = new Logger(this.loggerName); + } + return this._logger; + } + + /** + * Child resources belonging to this container. + * Setting this will clear the internal {@linkcode isLoading} state. + * If the children array has items, this will also set {@linkcode hasError} to `false`. + */ + get children(): T[] { + return this._children; + } + + set children(children: T[]) { + this._children = children; + this.isLoading = false; + this.description = `(${children.length})`; + + if (children.length > 0) { + this.hasError = false; + } + } + + get isLoading(): boolean { + return this._isLoading; + } + + set isLoading(loading: boolean) { + this._isLoading = loading; + this.iconPath = loading ? new ThemeIcon(IconNames.LOADING) : this._defaultIcon; + } + + get hasError(): boolean { + return this._hasError; + } + + /** Set or clear the error state for this container. */ + set hasError(error: boolean) { + this._hasError = error; + this.iconPath = error ? ERROR_ICON : this._defaultIcon; + + if (this._defaultContextValue) { + // append or remove "-error" suffix to context value based on error state to toggle enablement + // of resource-specific commands + this.contextValue = error ? `${this._defaultContextValue}-error` : this._defaultContextValue; + } + } + + searchableText(): string { + // label is required to be a string in the constructor, so we don't support the TreeItem + // label being undefined or a TreeItemLabel object here + return this.label as string; + } + + /** Wait until the container is no longer in a loading state, or timeout after `timeoutMs`. */ + async ensureDoneLoading(timeoutMs: number = 10000): Promise { + const startTime = Date.now(); + + while (this.isLoading) { + if (Date.now() - startTime >= timeoutMs) { + throw new Error("Timeout waiting for container to finish loading"); + } + await new Promise((resolve) => setTimeout(resolve, LOADING_POLL_INTERVAL_MS)); + } + } + + /** Get the container's {@link children resources}, waiting for loading to complete if necessary. */ + async gatherResources(timeoutMs: number = 10000): Promise { + let resources: T[] = []; + try { + await this.ensureDoneLoading(timeoutMs); + resources = this.children; + } catch (error) { + // should only be a timeout error: + this.logger.error(`Error getting resources: ${error}`); + } + return resources; + } +} From c717a753edea848249b0b94d7b472f6e2eff6654 Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Mon, 23 Feb 2026 15:46:11 -0500 Subject: [PATCH 02/15] move and simplify FlinkDatabaseResourceContainer class by subclassing from ResourceContainer; update tests and createTestResource params --- .../flinkDatabaseResourceContainer.test.ts | 33 ++ .../flinkDatabaseResourceContainer.ts | 36 ++ .../resourceContainer.test.ts} | 315 +++++++++--------- src/models/flinkDatabaseResourceContainer.ts | 135 -------- tests/unit/testResources/base.ts | 10 +- 5 files changed, 230 insertions(+), 299 deletions(-) create mode 100644 src/models/containers/flinkDatabaseResourceContainer.test.ts create mode 100644 src/models/containers/flinkDatabaseResourceContainer.ts rename src/models/{flinkDatabaseResourceContainer.test.ts => containers/resourceContainer.test.ts} (52%) delete mode 100644 src/models/flinkDatabaseResourceContainer.ts diff --git a/src/models/containers/flinkDatabaseResourceContainer.test.ts b/src/models/containers/flinkDatabaseResourceContainer.test.ts new file mode 100644 index 000000000..2199d9b74 --- /dev/null +++ b/src/models/containers/flinkDatabaseResourceContainer.test.ts @@ -0,0 +1,33 @@ +import * as assert from "assert"; +import { createFakeFlinkDatabaseResource } from "../../../tests/unit/testResources/flinkDatabaseResource"; +import { ConnectionType } from "../../clients/sidecar"; +import { CCLOUD_CONNECTION_ID } from "../../constants"; +import type { FlinkDatabaseResource } from "../flinkDatabaseResource"; +import { FlinkDatabaseResourceContainer } from "./flinkDatabaseResourceContainer"; + +describe("models/flinkDatabaseResourceContainer", () => { + describe("FlinkDatabaseResourceContainer", () => { + describe("constructor", () => { + it("should set connectionId to CCLOUD_CONNECTION_ID", () => { + const container = new FlinkDatabaseResourceContainer("Test", []); + + assert.strictEqual(container.connectionId, CCLOUD_CONNECTION_ID); + }); + + it("should set connectionType to Ccloud", () => { + const container = new FlinkDatabaseResourceContainer("Test", []); + + assert.strictEqual(container.connectionType, ConnectionType.Ccloud); + }); + + it("should set id to connectionId-label", () => { + const label = "Test Database"; + const container = new FlinkDatabaseResourceContainer(label, [ + createFakeFlinkDatabaseResource(), + ]); + + assert.strictEqual(container.id, `${CCLOUD_CONNECTION_ID}-${label}`); + }); + }); + }); +}); diff --git a/src/models/containers/flinkDatabaseResourceContainer.ts b/src/models/containers/flinkDatabaseResourceContainer.ts new file mode 100644 index 000000000..24686e5f3 --- /dev/null +++ b/src/models/containers/flinkDatabaseResourceContainer.ts @@ -0,0 +1,36 @@ +import type { ThemeIcon } from "vscode"; +import { ConnectionType } from "../../clients/sidecar"; +import { CCLOUD_CONNECTION_ID } from "../../constants"; +import type { FlinkArtifact } from "../flinkArtifact"; +import type { FlinkDatabaseResource } from "../flinkDatabaseResource"; +import type { ConnectionId } from "../resource"; +import { ResourceContainer } from "./resourceContainer"; + +/** Labels for the top-level containers in the Flink Database view. */ +export enum FlinkDatabaseContainerLabel { + RELATIONS = "Tables and Views", + ARTIFACTS = "Artifacts", + UDFS = "UDFs", + AI_CONNECTIONS = "Connections", + AI_TOOLS = "AI Tools", + AI_MODELS = "AI Models", + AI_AGENTS = "AI Agents", +} + +/** A container {@link TreeItem} for resources to display in the Flink Database view. */ +export class FlinkDatabaseResourceContainer< + T extends FlinkDatabaseResource | FlinkArtifact, +> extends ResourceContainer { + readonly connectionId: ConnectionId = CCLOUD_CONNECTION_ID; + readonly connectionType: ConnectionType = ConnectionType.Ccloud; + + get loggerName() { + return `models.FlinkDatabaseResourceContainer(${this.label})`; + } + + constructor(label: string, children: T[], contextValue?: string, icon?: ThemeIcon) { + super(label, children, contextValue, icon); + + this.id = `${CCLOUD_CONNECTION_ID}-${label}`; + } +} diff --git a/src/models/flinkDatabaseResourceContainer.test.ts b/src/models/containers/resourceContainer.test.ts similarity index 52% rename from src/models/flinkDatabaseResourceContainer.test.ts rename to src/models/containers/resourceContainer.test.ts index 6bbb450d0..a530a412e 100644 --- a/src/models/flinkDatabaseResourceContainer.test.ts +++ b/src/models/containers/resourceContainer.test.ts @@ -1,17 +1,27 @@ import * as assert from "assert"; import * as sinon from "sinon"; import { ThemeIcon, TreeItemCollapsibleState } from "vscode"; -import { createFakeFlinkDatabaseResource } from "../../tests/unit/testResources/flinkDatabaseResource"; -import { ConnectionType } from "../clients/sidecar"; -import { CCLOUD_CONNECTION_ID } from "../constants"; -import { ERROR_ICON, IconNames } from "../icons"; -import type { FlinkDatabaseResource } from "./flinkDatabaseResource"; -import { - FlinkDatabaseResourceContainer, - LOADING_POLL_INTERVAL_MS, -} from "./flinkDatabaseResourceContainer"; - -describe("models/flinkDatabaseResourceContainer", () => { +import { createTestResource } from "../../../tests/unit/testResources/base"; +import { ERROR_ICON, IconNames } from "../../icons"; +import type { BaseViewProviderData } from "../../viewProviders/baseModels/base"; +import { LOADING_POLL_INTERVAL_MS, ResourceContainer } from "./resourceContainer"; + +/** Minimal concrete subclass to test abstract base. */ +class TestContainer extends ResourceContainer { + readonly loggerName = "test.TestContainer"; + + constructor( + label: string, + children: BaseViewProviderData[], + contextValue?: string, + icon?: ThemeIcon, + ) { + super(label, children, contextValue, icon); + this.id = `test-${label}`; + } +} + +describe("models/containers/resourceContainer.ts", () => { let sandbox: sinon.SinonSandbox; beforeEach(() => { @@ -22,39 +32,66 @@ describe("models/flinkDatabaseResourceContainer", () => { sandbox.restore(); }); - describe("FlinkDatabaseResourceContainer", () => { - describe("constructor", () => { - const testResources: FlinkDatabaseResource[] = [ - createFakeFlinkDatabaseResource(), - createFakeFlinkDatabaseResource({ id: "Resource2" }), - ]; - - it("should create an instance with correct properties", () => { - const label = "Test Database"; + describe("ResourceContainer", () => { + const testLabel = "Test Resource Group"; + const testContextValue = "test.context"; + const testResource = createTestResource("test-resource"); + const testResources = [ + createTestResource("test-resource-1"), + createTestResource("test-resource-2"), + ]; - const item = new FlinkDatabaseResourceContainer(label, testResources); + describe("constructor", () => { + it("should set label and id from arguments", () => { + const container = new TestContainer(testLabel, testResources); - assert.strictEqual(item.label, label); - assert.deepStrictEqual(item.children, testResources); - assert.strictEqual(item.connectionId, CCLOUD_CONNECTION_ID); - assert.strictEqual(item.connectionType, ConnectionType.Ccloud); - assert.strictEqual(item.id, `${item.connectionId}-${label}`); + assert.strictEqual(container.label, testLabel); + assert.strictEqual(container.id, `test-${testLabel}`); }); it("should always set the collapsible state to Collapsed", () => { - const label = "Test Database"; - - const withChildren = new FlinkDatabaseResourceContainer(label, testResources); + const withChildren = new TestContainer(testLabel, testResources); assert.strictEqual(withChildren.collapsibleState, TreeItemCollapsibleState.Collapsed); - const withoutChildren = new FlinkDatabaseResourceContainer(label, []); + const withoutChildren = new TestContainer(testLabel, []); assert.strictEqual(withoutChildren.collapsibleState, TreeItemCollapsibleState.Collapsed); }); - it("should update description when `children` is set", () => { - const label = "Test Database"; + it("should set iconPath from the icon argument", () => { + const icon = new ThemeIcon("symbol-folder"); + const container = new TestContainer(testLabel, [], undefined, icon); + + assert.strictEqual(container.iconPath, icon); + }); + + it("should leave iconPath undefined when no icon is provided", () => { + const container = new TestContainer(testLabel, []); + + assert.strictEqual(container.iconPath, undefined); + }); + + it("should set contextValue when provided", () => { + const container = new TestContainer(testLabel, [], testContextValue); + + assert.strictEqual(container.contextValue, testContextValue); + }); - const container = new FlinkDatabaseResourceContainer(label, []); + it("should leave contextValue undefined when omitted", () => { + const container = new TestContainer(testLabel, []); + + assert.strictEqual(container.contextValue, undefined); + }); + + it("should leave description undefined before children setter is called", () => { + const container = new TestContainer(testLabel, []); + + assert.strictEqual(container.description, undefined); + }); + }); + + describe("children setter", () => { + it("should update description when `children` is set", () => { + const container = new TestContainer(testLabel, []); // description is not set in constructor (before loading) assert.strictEqual(container.description, undefined); @@ -64,164 +101,155 @@ describe("models/flinkDatabaseResourceContainer", () => { // and updates when children change container.children = []; - assert.strictEqual(container.description, `(0)`); + assert.strictEqual(container.description, "(0)"); + }); + + it("should clear isLoading state when children are set", () => { + const container = new TestContainer(testLabel, []); + container.isLoading = true; + + container.children = [testResource]; + + assert.strictEqual(container.isLoading, false); + }); + + it("should clear hasError when non-empty children are set", () => { + const container = new TestContainer(testLabel, []); + container.hasError = true; + + container.children = [testResource]; + + assert.strictEqual(container.hasError, false); + }); + + it("should preserve hasError when empty children are set", () => { + const container = new TestContainer(testLabel, []); + container.hasError = true; + + container.children = []; + + assert.strictEqual(container.hasError, true); }); }); describe("searchableText", () => { it("should return the label as searchable text", () => { - const label = "Searchable Database Resource"; - const resources: FlinkDatabaseResource[] = []; - - const item = new FlinkDatabaseResourceContainer(label, resources); + const container = new TestContainer(testLabel, []); - const searchableText = item.searchableText(); - assert.strictEqual(searchableText, label); + assert.strictEqual(container.searchableText(), testLabel); }); }); describe("isLoading", () => { it("should start with isLoading set to false", () => { - const container = new FlinkDatabaseResourceContainer("Test", []); + const container = new TestContainer(testLabel, []); assert.strictEqual(container.isLoading, false); }); it("should use the loading icon when isLoading is set to true", () => { - const container = new FlinkDatabaseResourceContainer("Test", []); + const container = new TestContainer(testLabel, []); container.isLoading = true; + assert.ok(container.iconPath); assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.LOADING); }); - it("should clear .iconPath when isLoading is set to false and no default icon is provided", () => { - const container = new FlinkDatabaseResourceContainer( - "Test", - [], - // no default icon (nor contextValue) set - ); + it("should use the default icon when isLoading is set to false", () => { + const icon = new ThemeIcon("symbol-folder"); + const container = new TestContainer(testLabel, [], undefined, icon); - // set initial loading state container.isLoading = true; - assert.ok(container.iconPath); assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.LOADING); container.isLoading = false; - assert.strictEqual(container.iconPath, undefined); + assert.strictEqual(container.iconPath, icon); }); - it("should use the default icon when isLoading is set to false", () => { - const customIcon = new ThemeIcon("symbol-folder"); - const container = new FlinkDatabaseResourceContainer( - "Test", + it("should clear .iconPath when isLoading is set to false and no default icon is provided", () => { + const container = new TestContainer( + "A", [], - undefined, - customIcon, + // no default icon (nor contextValue) set ); - assert.strictEqual(container.iconPath, customIcon); // set initial loading state container.isLoading = true; + assert.ok(container.iconPath); assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.LOADING); container.isLoading = false; - assert.strictEqual(container.iconPath, customIcon); - }); - - it("should clear isLoading state when children are set", () => { - const container = new FlinkDatabaseResourceContainer("Test", []); - container.isLoading = true; - - const testResources = [createFakeFlinkDatabaseResource()]; - container.children = testResources; - - assert.strictEqual(container.isLoading, false); + assert.strictEqual(container.iconPath, undefined); }); }); describe("hasError", () => { it("should start with hasError set to false", () => { - const container = new FlinkDatabaseResourceContainer("Test", []); - + const container = new TestContainer(testLabel, []); assert.strictEqual(container.hasError, false); }); it("should use the error icon when hasError is set to true", () => { - const container = new FlinkDatabaseResourceContainer("Test", []); + const container = new TestContainer(testLabel, []); container.hasError = true; - assert.ok(container.iconPath); + assert.deepStrictEqual(container.iconPath, ERROR_ICON); }); - it("should clear .iconPath when hasError is set to false and no default icon is provided", () => { - const container = new FlinkDatabaseResourceContainer("Test", []); + it("should use the default icon when hasError is set to false", () => { + const icon = new ThemeIcon("symbol-folder"); + const container = new TestContainer(testLabel, [], undefined, icon); - // set initial error state container.hasError = true; - assert.ok(container.iconPath); assert.deepStrictEqual(container.iconPath, ERROR_ICON); container.hasError = false; - assert.strictEqual(container.hasError, false); - assert.strictEqual(container.iconPath, undefined); + assert.strictEqual(container.iconPath, icon); }); - it("should use the default icon when hasError is set to false", () => { - const customIcon = new ThemeIcon("symbol-folder"); - const container = new FlinkDatabaseResourceContainer( - "Test", - [], - undefined, - customIcon, - ); - assert.strictEqual(container.iconPath, customIcon); + it("should clear .iconPath when hasError is set to false and no default icon is provided", () => { + const container = new TestContainer(testLabel, []); - // set initial error state container.hasError = true; assert.deepStrictEqual(container.iconPath, ERROR_ICON); container.hasError = false; - assert.strictEqual(container.iconPath, customIcon); + assert.strictEqual(container.iconPath, undefined); }); - it("should not modify .contextValue when no original contextValue was provided in the constructor", () => { - const container = new FlinkDatabaseResourceContainer("Test", []); + it("should toggle the contextValue between error and non-error states without suffix duplication", () => { + const container = new TestContainer(testLabel, [], testContextValue); container.hasError = true; - assert.strictEqual(container.contextValue, undefined); + assert.strictEqual(container.contextValue, `${testContextValue}-error`); container.hasError = false; - assert.strictEqual(container.contextValue, undefined); + assert.strictEqual(container.contextValue, testContextValue); + + // verify no suffix duplication on repeated toggles + container.hasError = true; + assert.strictEqual(container.contextValue, `${testContextValue}-error`); }); - it("should toggle the contextValue between error and non-error states without suffix duplication", () => { - const contextValue = "flinkDatabase.container"; - const container = new FlinkDatabaseResourceContainer( - "Test", - [], - contextValue, - ); + it("should not modify .contextValue when no original contextValue was provided in the constructor", () => { + const container = new TestContainer(testLabel, []); container.hasError = true; - assert.strictEqual(container.contextValue, `${contextValue}-error`); + assert.strictEqual(container.contextValue, undefined); container.hasError = false; - assert.strictEqual(container.contextValue, contextValue); - - container.hasError = true; - assert.strictEqual(container.contextValue, `${contextValue}-error`); + assert.strictEqual(container.contextValue, undefined); }); }); describe("state interactions", () => { it("should settle loading state and description when setting children", () => { - const container = new FlinkDatabaseResourceContainer("Test", []); + const container = new TestContainer(testLabel, []); container.isLoading = true; - const testResources = [createFakeFlinkDatabaseResource()]; - container.children = testResources; + container.children = [testResource]; assert.strictEqual(container.isLoading, false); assert.strictEqual(container.description, "(1)"); @@ -229,93 +257,66 @@ describe("models/flinkDatabaseResourceContainer", () => { }); it("should clear hasError when setting non-empty children", () => { - const contextValue = "flinkDatabase.container"; - const container = new FlinkDatabaseResourceContainer( - "Test", - [], - contextValue, - ); + const container = new TestContainer(testLabel, [], testContextValue); container.hasError = true; - const testResources = [createFakeFlinkDatabaseResource()]; - container.children = testResources; + container.children = [testResource]; - // hasError is cleared when we have successful results assert.strictEqual(container.hasError, false); assert.strictEqual(container.description, "(1)"); assert.strictEqual(container.iconPath, undefined); - assert.strictEqual(container.contextValue, contextValue); + assert.strictEqual(container.contextValue, testContextValue); }); it("should not clear hasError when setting empty children array", () => { - const contextValue = "flinkDatabase.container"; - const container = new FlinkDatabaseResourceContainer( - "Test", - [], - contextValue, - ); + const container = new TestContainer(testLabel, [], testContextValue); container.hasError = true; container.children = []; - // hasError persists when children array is empty (no successful results) assert.strictEqual(container.hasError, true); assert.strictEqual(container.description, "(0)"); - assert.strictEqual(container.contextValue, `${contextValue}-error`); + assert.strictEqual(container.contextValue, `${testContextValue}-error`); }); it("should handle multiple state transitions", () => { - const contextValue = "flinkDatabase.container"; - const container = new FlinkDatabaseResourceContainer( - "Test", - [], - contextValue, - ); - // simulate initial loading + const container = new TestContainer(testLabel, [], testContextValue); + container.isLoading = true; assert.ok(container.iconPath); - // set error container.hasError = true; - assert.strictEqual(container.contextValue, `${contextValue}-error`); + assert.strictEqual(container.contextValue, `${testContextValue}-error`); assert.ok(container.iconPath); - // set children with items (clears loading, error, and iconPath) - container.children = [createFakeFlinkDatabaseResource()]; + container.children = [testResource]; assert.strictEqual(container.isLoading, false); assert.strictEqual(container.hasError, false); - assert.strictEqual(container.contextValue, contextValue); + assert.strictEqual(container.contextValue, testContextValue); assert.strictEqual(container.iconPath, undefined); }); it("should handle error recovery with empty then non-empty children", () => { - const contextValue = "flinkDatabase.container"; - const container = new FlinkDatabaseResourceContainer( - "Test", - [], - contextValue, - ); + const container = new TestContainer(testLabel, [], testContextValue); - // set error and empty children (error persists) container.hasError = true; container.children = []; assert.strictEqual(container.hasError, true); - assert.strictEqual(container.contextValue, `${contextValue}-error`); + assert.strictEqual(container.contextValue, `${testContextValue}-error`); - // then set non-empty children (error clears) - container.children = [createFakeFlinkDatabaseResource()]; + container.children = [testResource]; assert.strictEqual(container.hasError, false); - assert.strictEqual(container.contextValue, contextValue); + assert.strictEqual(container.contextValue, testContextValue); }); }); describe("ensureDoneLoading", () => { let clock: sinon.SinonFakeTimers; - let container: FlinkDatabaseResourceContainer; + let container: TestContainer; beforeEach(() => { clock = sandbox.useFakeTimers(); - container = new FlinkDatabaseResourceContainer("Test", []); + container = new TestContainer(testLabel, []); }); it("should resolve immediately when not loading", async () => { @@ -331,10 +332,8 @@ describe("models/flinkDatabaseResourceContainer", () => { const waitPromise = container.ensureDoneLoading(); - // simulate loading completing after 200ms await clock.tickAsync(200); container.isLoading = false; - // one more iteration to let the polling check run await clock.tickAsync(LOADING_POLL_INTERVAL_MS + 1); await waitPromise; @@ -346,7 +345,6 @@ describe("models/flinkDatabaseResourceContainer", () => { const timeoutMs = 500; const waitPromise = container.ensureDoneLoading(timeoutMs); - // fast forward past the timeout await clock.tickAsync(timeoutMs + 10); await assert.rejects(waitPromise, /Timeout waiting for container to finish loading/); @@ -355,16 +353,15 @@ describe("models/flinkDatabaseResourceContainer", () => { describe("gatherResources", () => { let ensureDoneLoadingStub: sinon.SinonStub; - let container: FlinkDatabaseResourceContainer; + let container: TestContainer; beforeEach(() => { - container = new FlinkDatabaseResourceContainer("Test", []); - // no need to handle FakeTimers here since we've tested ensureDoneLoading above + container = new TestContainer(testLabel, []); ensureDoneLoadingStub = sandbox.stub(container, "ensureDoneLoading"); }); it("should return children after calling ensureDoneLoading", async () => { - const resources = [createFakeFlinkDatabaseResource()]; + const resources = [testResource]; container.children = resources; const result = await container.gatherResources(); diff --git a/src/models/flinkDatabaseResourceContainer.ts b/src/models/flinkDatabaseResourceContainer.ts deleted file mode 100644 index 686d55039..000000000 --- a/src/models/flinkDatabaseResourceContainer.ts +++ /dev/null @@ -1,135 +0,0 @@ -import { ThemeIcon, TreeItem, TreeItemCollapsibleState } from "vscode"; -import { ConnectionType } from "../clients/sidecar"; -import { CCLOUD_CONNECTION_ID } from "../constants"; -import { ERROR_ICON, IconNames } from "../icons"; -import { Logger } from "../logging"; -import type { FlinkArtifact } from "./flinkArtifact"; -import type { FlinkDatabaseResource } from "./flinkDatabaseResource"; -import type { ConnectionId, ISearchable } from "./resource"; - -/** Labels for the top-level containers in the Flink Database view. */ -export enum FlinkDatabaseContainerLabel { - RELATIONS = "Tables and Views", - ARTIFACTS = "Artifacts", - UDFS = "UDFs", - AI_CONNECTIONS = "Connections", - AI_TOOLS = "AI Tools", - AI_MODELS = "AI Models", - AI_AGENTS = "AI Agents", -} - -/** Poll interval to use when waiting for a container to finish loading. */ -export const LOADING_POLL_INTERVAL_MS = 100; - -/** A container {@link TreeItem} for resources to display in the Flink Database view. */ -export class FlinkDatabaseResourceContainer - extends TreeItem - implements ISearchable -{ - readonly connectionId: ConnectionId = CCLOUD_CONNECTION_ID; - readonly connectionType: ConnectionType = ConnectionType.Ccloud; - - // `id` is string|undefined in TreeItem, but only string in IdItem so we need to specify it here - id: string; - - private _children: T[]; - - private _isLoading: boolean = false; - private _hasError: boolean = false; - private readonly _defaultContextValue: string | undefined; - private readonly _defaultIcon: ThemeIcon | undefined; - - private logger: Logger; - - constructor(label: string, children: T[], contextValue?: string, icon?: ThemeIcon) { - const collapsibleState = TreeItemCollapsibleState.Collapsed; - super(label, collapsibleState); - - this._children = children; - this.id = `${this.connectionId}-${label}`; - - this._defaultContextValue = contextValue; - if (contextValue) { - this.contextValue = contextValue; - } - this._defaultIcon = icon; - this.iconPath = this._defaultIcon; - - this.logger = new Logger(`models.FlinkDatabaseResourceContainer(${this.label})`); - } - - /** - * Flink Database resources belonging to this container. - * Setting this will clear the internal {@linkcode isLoading} state. - * If the children array has items, this will also set {@linkcode hasError} to `false`. - */ - get children(): T[] { - return this._children; - } - - set children(children: T[]) { - this._children = children; - this.isLoading = false; - this.description = `(${children.length})`; - - if (children.length > 0) { - this.hasError = false; - } - } - - get isLoading(): boolean { - return this._isLoading; - } - - set isLoading(loading: boolean) { - this._isLoading = loading; - this.iconPath = loading ? new ThemeIcon(IconNames.LOADING) : this._defaultIcon; - } - - get hasError(): boolean { - return this._hasError; - } - - /** Set or clear the error state for this container. */ - set hasError(error: boolean) { - this._hasError = error; - this.iconPath = error ? ERROR_ICON : this._defaultIcon; - - if (this._defaultContextValue) { - // append or remove "-error" suffix to context value based on error state to toggle enablement - // of resource-specific commands - this.contextValue = error ? `${this._defaultContextValue}-error` : this._defaultContextValue; - } - } - - searchableText(): string { - // label is required to be a string in the constructor, so we don't support the TreeItem - // label being undefined or a TreeItemLabel object here - return this.label as string; - } - - /** Wait until the container is no longer in a loading state, or timeout after `timeoutMs`. */ - async ensureDoneLoading(timeoutMs: number = 10000): Promise { - const startTime = Date.now(); - - while (this.isLoading) { - if (Date.now() - startTime >= timeoutMs) { - throw new Error("Timeout waiting for container to finish loading"); - } - await new Promise((resolve) => setTimeout(resolve, LOADING_POLL_INTERVAL_MS)); - } - } - - /** Get the container's {@link children resources}, waiting for loading to complete if necessary. */ - async gatherResources(timeoutMs: number = 10000): Promise { - let resources: T[] = []; - try { - await this.ensureDoneLoading(timeoutMs); - resources = this.children; - } catch (error) { - // should only be a timeout error: - this.logger.error(`Error getting resources: ${error}`); - } - return resources; - } -} diff --git a/tests/unit/testResources/base.ts b/tests/unit/testResources/base.ts index efd0787b2..ac097319c 100644 --- a/tests/unit/testResources/base.ts +++ b/tests/unit/testResources/base.ts @@ -48,7 +48,7 @@ export function getTestEnvironmentIdForConnectionType( */ export function createTestResource( id: string, - name: string, + name?: string, connectionType: ConnectionType = ConnectionType.Ccloud, children?: BaseViewProviderData[], ): BaseViewProviderData { @@ -56,7 +56,7 @@ export function createTestResource( id, connectionId: getTestConnectionIdForType(connectionType), connectionType, - searchableText: () => name, + searchableText: () => name ?? id, children, }; } @@ -73,16 +73,16 @@ export interface TestParentedResource extends EnvironmentedBaseViewProviderData */ export function createParentedTestResource( id: string, - name: string, + name?: string, connectionType: ConnectionType = ConnectionType.Ccloud, ): TestParentedResource { const base: TestParentedResource = { id, - name, + name: name ?? id, connectionId: getTestConnectionIdForType(connectionType), connectionType, environmentId: getTestEnvironmentIdForConnectionType(connectionType), - searchableText: () => name, + searchableText: () => name ?? id, }; if (connectionType === ConnectionType.Ccloud) { From 1158649caaa517697b4528d1fa1247d79bad4e6f Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Mon, 23 Feb 2026 15:46:45 -0500 Subject: [PATCH 03/15] update flinkDatabaseResourceContainer imports --- src/commands/flinkArtifacts.test.ts | 2 +- src/commands/flinkDatabaseView.test.ts | 2 +- src/commands/flinkDatabaseView.ts | 4 ++-- src/commands/utils/udfRegistration.test.ts | 5 +++-- src/viewProviders/flinkDatabase.test.ts | 2 +- src/viewProviders/flinkDatabase.ts | 2 +- 6 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/commands/flinkArtifacts.test.ts b/src/commands/flinkArtifacts.test.ts index fbc1a43d0..51d1851ba 100644 --- a/src/commands/flinkArtifacts.test.ts +++ b/src/commands/flinkArtifacts.test.ts @@ -12,7 +12,7 @@ import type { PresignedUploadUrlArtifactV1PresignedUrl200ResponseKindEnum, } from "../clients/flinkArtifacts/models/PresignedUploadUrlArtifactV1PresignedUrl200Response"; import type { FlinkArtifact } from "../models/flinkArtifact"; -import { FlinkDatabaseResourceContainer } from "../models/flinkDatabaseResourceContainer"; +import { FlinkDatabaseResourceContainer } from "../models/containers/flinkDatabaseResourceContainer"; import { type EnvironmentId } from "../models/resource"; import * as notifications from "../notifications"; import { FlinkDatabaseViewProvider } from "../viewProviders/flinkDatabase"; diff --git a/src/commands/flinkDatabaseView.test.ts b/src/commands/flinkDatabaseView.test.ts index 28fc94068..6ac32683a 100644 --- a/src/commands/flinkDatabaseView.test.ts +++ b/src/commands/flinkDatabaseView.test.ts @@ -21,7 +21,7 @@ import type { CCloudResourceLoader } from "../loaders"; import { FlinkDatabaseContainerLabel, FlinkDatabaseResourceContainer, -} from "../models/flinkDatabaseResourceContainer"; +} from "../models/containers/flinkDatabaseResourceContainer"; import { FlinkDatabaseViewProvider } from "../viewProviders/flinkDatabase"; describe("commands/flinkDatabaseView.ts", () => { diff --git a/src/commands/flinkDatabaseView.ts b/src/commands/flinkDatabaseView.ts index 341ce8043..9b263ef0b 100644 --- a/src/commands/flinkDatabaseView.ts +++ b/src/commands/flinkDatabaseView.ts @@ -4,8 +4,8 @@ import { setFlinkDocumentMetadata } from "../flinkSql/statementUtils"; import { CCloudResourceLoader } from "../loaders"; import { Logger } from "../logging"; import type { CCloudEnvironment } from "../models/environment"; -import type { FlinkDatabaseResourceContainer } from "../models/flinkDatabaseResourceContainer"; -import { FlinkDatabaseContainerLabel } from "../models/flinkDatabaseResourceContainer"; +import type { FlinkDatabaseResourceContainer } from "../models/containers/flinkDatabaseResourceContainer"; +import { FlinkDatabaseContainerLabel } from "../models/containers/flinkDatabaseResourceContainer"; import { FlinkDatabaseViewProvider } from "../viewProviders/flinkDatabase"; const logger = new Logger("FlinkDatabaseViewCommands"); diff --git a/src/commands/utils/udfRegistration.test.ts b/src/commands/utils/udfRegistration.test.ts index 705f42281..3bdfb164a 100644 --- a/src/commands/utils/udfRegistration.test.ts +++ b/src/commands/utils/udfRegistration.test.ts @@ -10,9 +10,10 @@ import { createFlinkUDF } from "../../../tests/unit/testResources/flinkUDF"; import { TEST_CCLOUD_FLINK_DB_KAFKA_CLUSTER } from "../../../tests/unit/testResources/kafkaCluster"; import * as emitters from "../../emitters"; import { type CCloudResourceLoader } from "../../loaders"; -import { FlinkDatabaseResourceContainer } from "../../models/flinkDatabaseResourceContainer"; +import { FlinkDatabaseResourceContainer } from "../../models/containers/flinkDatabaseResourceContainer"; import type { FlinkUdf } from "../../models/flinkUDF"; -import { CCloudFlinkDbKafkaCluster, CCloudKafkaCluster } from "../../models/kafkaCluster"; +import type { CCloudFlinkDbKafkaCluster } from "../../models/kafkaCluster"; +import { CCloudKafkaCluster } from "../../models/kafkaCluster"; import * as notifications from "../../notifications"; import * as kafkaClusterQuickpicks from "../../quickpicks/kafkaClusters"; import * as jarInspector from "../../utils/jarInspector"; diff --git a/src/viewProviders/flinkDatabase.test.ts b/src/viewProviders/flinkDatabase.test.ts index df173ffa1..8aeb182d5 100644 --- a/src/viewProviders/flinkDatabase.test.ts +++ b/src/viewProviders/flinkDatabase.test.ts @@ -29,7 +29,7 @@ import type { FlinkDatabaseResource } from "../models/flinkDatabaseResource"; import { FlinkDatabaseContainerLabel, FlinkDatabaseResourceContainer, -} from "../models/flinkDatabaseResourceContainer"; +} from "../models/containers/flinkDatabaseResourceContainer"; import { FlinkUdfTreeItem } from "../models/flinkUDF"; import type { CCloudFlinkDbKafkaCluster } from "../models/kafkaCluster"; import { CCloudKafkaCluster } from "../models/kafkaCluster"; diff --git a/src/viewProviders/flinkDatabase.ts b/src/viewProviders/flinkDatabase.ts index 65ac65d53..c3f73a234 100644 --- a/src/viewProviders/flinkDatabase.ts +++ b/src/viewProviders/flinkDatabase.ts @@ -22,7 +22,7 @@ import type { FlinkAIResource, FlinkDatabaseResource } from "../models/flinkData import { FlinkDatabaseContainerLabel, FlinkDatabaseResourceContainer, -} from "../models/flinkDatabaseResourceContainer"; +} from "../models/containers/flinkDatabaseResourceContainer"; import { FlinkRelation, FlinkRelationColumn } from "../models/flinkRelation"; import { FlinkUdf, FlinkUdfTreeItem } from "../models/flinkUDF"; import type { CCloudFlinkDbKafkaCluster } from "../models/kafkaCluster"; From a01fcdd7cee21beb8f2b8b20a162b40d62c17eb4 Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Mon, 23 Feb 2026 15:47:27 -0500 Subject: [PATCH 04/15] subclass ConsumerGroupContainer from ResourceContainer base and move to models/containers --- .../containers/consumerGroupContainer.ts | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 src/models/containers/consumerGroupContainer.ts diff --git a/src/models/containers/consumerGroupContainer.ts b/src/models/containers/consumerGroupContainer.ts new file mode 100644 index 000000000..36adbbcba --- /dev/null +++ b/src/models/containers/consumerGroupContainer.ts @@ -0,0 +1,37 @@ +import { ThemeIcon } from "vscode"; +import type { ConnectionType } from "../../clients/sidecar"; +import { IconNames } from "../../icons"; +import type { ConsumerGroup } from "../consumerGroup"; +import type { ConnectionId, EnvironmentId } from "../resource"; +import { ResourceContainer } from "./resourceContainer"; + +/** A container {@link TreeItem} for consumer groups in the Topics view. */ +export class ConsumerGroupContainer extends ResourceContainer { + readonly loggerName = "models.ConsumerGroupContainer"; + + readonly connectionId: ConnectionId; + readonly connectionType: ConnectionType; + readonly clusterId: string; + readonly environmentId: EnvironmentId; + + constructor( + connectionId: ConnectionId, + connectionType: ConnectionType, + clusterId: string, + environmentId: EnvironmentId, + children: ConsumerGroup[] = [], + ) { + super( + "Consumer Groups", + children, + "consumerGroups-container", + new ThemeIcon(IconNames.CONSUMER_GROUP), + ); + this.id = `${connectionId}-${clusterId}-consumer-groups`; + + this.connectionId = connectionId; + this.connectionType = connectionType; + this.clusterId = clusterId; + this.environmentId = environmentId; + } +} From a695e22f07c269ff6324ba06b932b00c51f93b88 Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Mon, 23 Feb 2026 15:47:36 -0500 Subject: [PATCH 05/15] add ConsumerGroupContainer tests --- .../containers/consumerGroupContainer.test.ts | 164 ++++++++++++++++++ 1 file changed, 164 insertions(+) create mode 100644 src/models/containers/consumerGroupContainer.test.ts diff --git a/src/models/containers/consumerGroupContainer.test.ts b/src/models/containers/consumerGroupContainer.test.ts new file mode 100644 index 000000000..a478e62bc --- /dev/null +++ b/src/models/containers/consumerGroupContainer.test.ts @@ -0,0 +1,164 @@ +import * as assert from "assert"; +import type { ThemeIcon } from "vscode"; +import { TEST_DIRECT_CONNECTION_ID } from "../../../tests/unit/testResources/connection"; +import { TEST_CCLOUD_CONSUMER_GROUP } from "../../../tests/unit/testResources/consumerGroup"; +import { + TEST_CCLOUD_KAFKA_CLUSTER, + TEST_DIRECT_KAFKA_CLUSTER, + TEST_LOCAL_KAFKA_CLUSTER, +} from "../../../tests/unit/testResources/kafkaCluster"; +import { ConnectionType } from "../../clients/sidecar"; +import { CCLOUD_CONNECTION_ID, LOCAL_CONNECTION_ID } from "../../constants"; +import { IconNames } from "../../icons"; +import { ConsumerGroupContainer } from "./consumerGroupContainer"; + +describe("models/containers/consumerGroupContainer", () => { + describe("ConsumerGroupContainer", () => { + describe("constructor", () => { + it("should set connectionId from constructor argument", () => { + const container = new ConsumerGroupContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_CCLOUD_KAFKA_CLUSTER.id, + TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + ); + + assert.strictEqual(container.connectionId, CCLOUD_CONNECTION_ID); + }); + + it("should set connectionType from constructor argument", () => { + const container = new ConsumerGroupContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_CCLOUD_KAFKA_CLUSTER.id, + TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + ); + + assert.strictEqual(container.connectionType, ConnectionType.Ccloud); + }); + + it("should set clusterId from constructor argument", () => { + const container = new ConsumerGroupContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_CCLOUD_KAFKA_CLUSTER.id, + TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + ); + + assert.strictEqual(container.clusterId, TEST_CCLOUD_KAFKA_CLUSTER.id); + }); + + it("should set environmentId from constructor argument", () => { + const container = new ConsumerGroupContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_CCLOUD_KAFKA_CLUSTER.id, + TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + ); + + assert.strictEqual(container.environmentId, TEST_CCLOUD_KAFKA_CLUSTER.environmentId); + }); + + it("should set id to connectionId-clusterId-consumer-groups", () => { + const container = new ConsumerGroupContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_CCLOUD_KAFKA_CLUSTER.id, + TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + ); + + assert.strictEqual( + container.id, + `${CCLOUD_CONNECTION_ID}-${TEST_CCLOUD_KAFKA_CLUSTER.id}-consumer-groups`, + ); + }); + + it("should use 'Consumer Groups' as the label", () => { + const container = new ConsumerGroupContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_CCLOUD_KAFKA_CLUSTER.id, + TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + ); + + assert.strictEqual(container.label, "Consumer Groups"); + }); + + it("should use the consumer group icon", () => { + const container = new ConsumerGroupContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_CCLOUD_KAFKA_CLUSTER.id, + TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + ); + + assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.CONSUMER_GROUP); + }); + + it("should set contextValue to consumerGroups-container", () => { + const container = new ConsumerGroupContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_CCLOUD_KAFKA_CLUSTER.id, + TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + ); + + assert.strictEqual(container.contextValue, "consumerGroups-container"); + }); + + it("should default to empty children", () => { + const container = new ConsumerGroupContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_CCLOUD_KAFKA_CLUSTER.id, + TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + ); + + assert.deepStrictEqual(container.children, []); + }); + + it("should accept initial children", () => { + const groups = [TEST_CCLOUD_CONSUMER_GROUP]; + const container = new ConsumerGroupContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_CCLOUD_KAFKA_CLUSTER.id, + TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + groups, + ); + + assert.deepStrictEqual(container.children, groups); + }); + }); + + describe("connection types", () => { + it("should work with Local connection type", () => { + const container = new ConsumerGroupContainer( + LOCAL_CONNECTION_ID, + ConnectionType.Local, + TEST_LOCAL_KAFKA_CLUSTER.id, + TEST_LOCAL_KAFKA_CLUSTER.environmentId, + ); + + assert.strictEqual(container.connectionId, LOCAL_CONNECTION_ID); + assert.strictEqual(container.connectionType, ConnectionType.Local); + assert.strictEqual( + container.id, + `${LOCAL_CONNECTION_ID}-${TEST_LOCAL_KAFKA_CLUSTER.id}-consumer-groups`, + ); + }); + + it("should work with Direct connection type", () => { + const container = new ConsumerGroupContainer( + TEST_DIRECT_CONNECTION_ID, + ConnectionType.Direct, + TEST_DIRECT_KAFKA_CLUSTER.id, + TEST_DIRECT_KAFKA_CLUSTER.environmentId, + ); + + assert.strictEqual(container.connectionId, TEST_DIRECT_CONNECTION_ID); + assert.strictEqual(container.connectionType, ConnectionType.Direct); + }); + }); + }); +}); From 14ed1cbb5949d0753ca89a1f4653a0c446df97cb Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Mon, 23 Feb 2026 18:17:48 -0500 Subject: [PATCH 06/15] swap ConsumerGroupContainer for KafkaClusterResourcecontainer to align with FlinkDatabaseResourceContainer --- .../containers/consumerGroupContainer.ts | 37 ----------------- .../kafkaClusterResourceContainer.ts | 40 +++++++++++++++++++ 2 files changed, 40 insertions(+), 37 deletions(-) delete mode 100644 src/models/containers/consumerGroupContainer.ts create mode 100644 src/models/containers/kafkaClusterResourceContainer.ts diff --git a/src/models/containers/consumerGroupContainer.ts b/src/models/containers/consumerGroupContainer.ts deleted file mode 100644 index 36adbbcba..000000000 --- a/src/models/containers/consumerGroupContainer.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { ThemeIcon } from "vscode"; -import type { ConnectionType } from "../../clients/sidecar"; -import { IconNames } from "../../icons"; -import type { ConsumerGroup } from "../consumerGroup"; -import type { ConnectionId, EnvironmentId } from "../resource"; -import { ResourceContainer } from "./resourceContainer"; - -/** A container {@link TreeItem} for consumer groups in the Topics view. */ -export class ConsumerGroupContainer extends ResourceContainer { - readonly loggerName = "models.ConsumerGroupContainer"; - - readonly connectionId: ConnectionId; - readonly connectionType: ConnectionType; - readonly clusterId: string; - readonly environmentId: EnvironmentId; - - constructor( - connectionId: ConnectionId, - connectionType: ConnectionType, - clusterId: string, - environmentId: EnvironmentId, - children: ConsumerGroup[] = [], - ) { - super( - "Consumer Groups", - children, - "consumerGroups-container", - new ThemeIcon(IconNames.CONSUMER_GROUP), - ); - this.id = `${connectionId}-${clusterId}-consumer-groups`; - - this.connectionId = connectionId; - this.connectionType = connectionType; - this.clusterId = clusterId; - this.environmentId = environmentId; - } -} diff --git a/src/models/containers/kafkaClusterResourceContainer.ts b/src/models/containers/kafkaClusterResourceContainer.ts new file mode 100644 index 000000000..53ae49af5 --- /dev/null +++ b/src/models/containers/kafkaClusterResourceContainer.ts @@ -0,0 +1,40 @@ +import type { ThemeIcon } from "vscode"; +import type { ConnectionType } from "../../clients/sidecar"; +import type { ConnectionId, EnvironmentId, ISearchable } from "../resource"; +import { ResourceContainer } from "./resourceContainer"; + +/** A container {@link TreeItem} for resources to display in the Topics view. */ + +export class KafkaClusterResourceContainer extends ResourceContainer { + readonly connectionId: ConnectionId; + readonly connectionType: ConnectionType; + readonly clusterId: string; + readonly environmentId: EnvironmentId; + + get loggerName() { + return `models.KafkaClusterResourceContainer(${this.label})`; + } + + constructor( + connectionId: ConnectionId, + connectionType: ConnectionType, + clusterId: string, + environmentId: EnvironmentId, + label: string, + children: T[] = [], + contextValue?: string, + icon?: ThemeIcon, + ) { + super(label, children, contextValue, icon); + + // convert label to hyphenated id: + // "Consumer Groups" → "consumer-groups", "Topics" → "topics" + const suffix = label.toLowerCase().replace(/\s+/g, "-"); + this.id = `${connectionId}-${clusterId}-${suffix}`; + + this.connectionId = connectionId; + this.connectionType = connectionType; + this.clusterId = clusterId; + this.environmentId = environmentId; + } +} From 174810eb0dee1c5b62ab9052c588418c79dd8ad9 Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Tue, 24 Feb 2026 09:59:25 -0500 Subject: [PATCH 07/15] update tests for generalized KafkaClusterResourceContainer --- ... => kafkaClusterResourceContainer.test.ts} | 116 +++++++++++++----- 1 file changed, 84 insertions(+), 32 deletions(-) rename src/models/containers/{consumerGroupContainer.test.ts => kafkaClusterResourceContainer.test.ts} (59%) diff --git a/src/models/containers/consumerGroupContainer.test.ts b/src/models/containers/kafkaClusterResourceContainer.test.ts similarity index 59% rename from src/models/containers/consumerGroupContainer.test.ts rename to src/models/containers/kafkaClusterResourceContainer.test.ts index a478e62bc..a1db301d0 100644 --- a/src/models/containers/consumerGroupContainer.test.ts +++ b/src/models/containers/kafkaClusterResourceContainer.test.ts @@ -10,150 +10,202 @@ import { import { ConnectionType } from "../../clients/sidecar"; import { CCLOUD_CONNECTION_ID, LOCAL_CONNECTION_ID } from "../../constants"; import { IconNames } from "../../icons"; -import { ConsumerGroupContainer } from "./consumerGroupContainer"; +import { KafkaClusterResourceContainer } from "./kafkaClusterResourceContainer"; -describe("models/containers/consumerGroupContainer", () => { - describe("ConsumerGroupContainer", () => { +const TEST_LABEL = "Test"; +const TEST_CONTEXT_VALUE = "test-container"; + +describe("models/containers/kafkaClusterResourceContainer", () => { + describe("KafkaClusterResourceContainer", () => { describe("constructor", () => { it("should set connectionId from constructor argument", () => { - const container = new ConsumerGroupContainer( + const container = new KafkaClusterResourceContainer( CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, TEST_CCLOUD_KAFKA_CLUSTER.id, TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + TEST_LABEL, ); assert.strictEqual(container.connectionId, CCLOUD_CONNECTION_ID); }); it("should set connectionType from constructor argument", () => { - const container = new ConsumerGroupContainer( + const container = new KafkaClusterResourceContainer( CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, TEST_CCLOUD_KAFKA_CLUSTER.id, TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + TEST_LABEL, ); assert.strictEqual(container.connectionType, ConnectionType.Ccloud); }); it("should set clusterId from constructor argument", () => { - const container = new ConsumerGroupContainer( + const container = new KafkaClusterResourceContainer( CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, TEST_CCLOUD_KAFKA_CLUSTER.id, TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + TEST_LABEL, ); assert.strictEqual(container.clusterId, TEST_CCLOUD_KAFKA_CLUSTER.id); }); it("should set environmentId from constructor argument", () => { - const container = new ConsumerGroupContainer( + const container = new KafkaClusterResourceContainer( CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, TEST_CCLOUD_KAFKA_CLUSTER.id, TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + TEST_LABEL, ); assert.strictEqual(container.environmentId, TEST_CCLOUD_KAFKA_CLUSTER.environmentId); }); - it("should set id to connectionId-clusterId-consumer-groups", () => { - const container = new ConsumerGroupContainer( + it("should set the label from constructor argument", () => { + const container = new KafkaClusterResourceContainer( CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, TEST_CCLOUD_KAFKA_CLUSTER.id, TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + TEST_LABEL, ); - assert.strictEqual( - container.id, - `${CCLOUD_CONNECTION_ID}-${TEST_CCLOUD_KAFKA_CLUSTER.id}-consumer-groups`, - ); + assert.strictEqual(container.label, TEST_LABEL); }); - it("should use 'Consumer Groups' as the label", () => { - const container = new ConsumerGroupContainer( + it("should set contextValue when provided", () => { + const container = new KafkaClusterResourceContainer( CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, TEST_CCLOUD_KAFKA_CLUSTER.id, TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + TEST_LABEL, + [], + TEST_CONTEXT_VALUE, ); - assert.strictEqual(container.label, "Consumer Groups"); + assert.strictEqual(container.contextValue, TEST_CONTEXT_VALUE); }); - it("should use the consumer group icon", () => { - const container = new ConsumerGroupContainer( + it("should set icon when provided", () => { + const container = new KafkaClusterResourceContainer( CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, TEST_CCLOUD_KAFKA_CLUSTER.id, TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + TEST_LABEL, + [], + undefined, + { id: IconNames.CONSUMER_GROUP } as ThemeIcon, ); assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.CONSUMER_GROUP); }); - it("should set contextValue to consumerGroups-container", () => { - const container = new ConsumerGroupContainer( + it("should default to empty children", () => { + const container = new KafkaClusterResourceContainer( CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, TEST_CCLOUD_KAFKA_CLUSTER.id, TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + TEST_LABEL, ); - assert.strictEqual(container.contextValue, "consumerGroups-container"); + assert.deepStrictEqual(container.children, []); }); - it("should default to empty children", () => { - const container = new ConsumerGroupContainer( + it("should accept initial children", () => { + const children = [TEST_CCLOUD_CONSUMER_GROUP]; + const container = new KafkaClusterResourceContainer( CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, TEST_CCLOUD_KAFKA_CLUSTER.id, TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + TEST_LABEL, + children, ); - assert.deepStrictEqual(container.children, []); + assert.deepStrictEqual(container.children, children); }); + }); - it("should accept initial children", () => { - const groups = [TEST_CCLOUD_CONSUMER_GROUP]; - const container = new ConsumerGroupContainer( + describe("id derivation", () => { + it("should derive id suffix from single-word label", () => { + const container = new KafkaClusterResourceContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_CCLOUD_KAFKA_CLUSTER.id, + TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + "Topics", + ); + + assert.strictEqual( + container.id, + `${CCLOUD_CONNECTION_ID}-${TEST_CCLOUD_KAFKA_CLUSTER.id}-topics`, + ); + }); + + it("should derive id suffix from multi-word label", () => { + const container = new KafkaClusterResourceContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_CCLOUD_KAFKA_CLUSTER.id, + TEST_CCLOUD_KAFKA_CLUSTER.environmentId, + "Consumer Groups", + ); + + assert.strictEqual( + container.id, + `${CCLOUD_CONNECTION_ID}-${TEST_CCLOUD_KAFKA_CLUSTER.id}-consumer-groups`, + ); + }); + }); + + describe("loggerName", () => { + it("should include label in loggerName", () => { + const label = "Test Resources"; + const container = new KafkaClusterResourceContainer( CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, TEST_CCLOUD_KAFKA_CLUSTER.id, TEST_CCLOUD_KAFKA_CLUSTER.environmentId, - groups, + label, ); - assert.deepStrictEqual(container.children, groups); + assert.strictEqual(container.loggerName, `models.KafkaClusterResourceContainer(${label})`); }); }); describe("connection types", () => { it("should work with Local connection type", () => { - const container = new ConsumerGroupContainer( + const container = new KafkaClusterResourceContainer( LOCAL_CONNECTION_ID, ConnectionType.Local, TEST_LOCAL_KAFKA_CLUSTER.id, TEST_LOCAL_KAFKA_CLUSTER.environmentId, + TEST_LABEL, ); assert.strictEqual(container.connectionId, LOCAL_CONNECTION_ID); assert.strictEqual(container.connectionType, ConnectionType.Local); assert.strictEqual( container.id, - `${LOCAL_CONNECTION_ID}-${TEST_LOCAL_KAFKA_CLUSTER.id}-consumer-groups`, + `${LOCAL_CONNECTION_ID}-${TEST_LOCAL_KAFKA_CLUSTER.id}-test`, ); }); it("should work with Direct connection type", () => { - const container = new ConsumerGroupContainer( + const container = new KafkaClusterResourceContainer( TEST_DIRECT_CONNECTION_ID, ConnectionType.Direct, TEST_DIRECT_KAFKA_CLUSTER.id, TEST_DIRECT_KAFKA_CLUSTER.environmentId, + TEST_LABEL, ); assert.strictEqual(container.connectionId, TEST_DIRECT_CONNECTION_ID); From 0f21b58963a8a2a431766092b4b00ef91594ef3f Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Tue, 24 Feb 2026 14:33:52 -0500 Subject: [PATCH 08/15] update describe block naming to include containers/ subdir --- src/models/containers/flinkDatabaseResourceContainer.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/models/containers/flinkDatabaseResourceContainer.test.ts b/src/models/containers/flinkDatabaseResourceContainer.test.ts index 2199d9b74..363461eac 100644 --- a/src/models/containers/flinkDatabaseResourceContainer.test.ts +++ b/src/models/containers/flinkDatabaseResourceContainer.test.ts @@ -5,7 +5,7 @@ import { CCLOUD_CONNECTION_ID } from "../../constants"; import type { FlinkDatabaseResource } from "../flinkDatabaseResource"; import { FlinkDatabaseResourceContainer } from "./flinkDatabaseResourceContainer"; -describe("models/flinkDatabaseResourceContainer", () => { +describe("models/containers/flinkDatabaseResourceContainer", () => { describe("FlinkDatabaseResourceContainer", () => { describe("constructor", () => { it("should set connectionId to CCLOUD_CONNECTION_ID", () => { From a95f49173b629e6581dae39647b64beb348edd07 Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Tue, 24 Feb 2026 17:21:21 -0500 Subject: [PATCH 09/15] remove unnecessary newline --- src/models/containers/kafkaClusterResourceContainer.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/models/containers/kafkaClusterResourceContainer.ts b/src/models/containers/kafkaClusterResourceContainer.ts index 53ae49af5..30f0668cf 100644 --- a/src/models/containers/kafkaClusterResourceContainer.ts +++ b/src/models/containers/kafkaClusterResourceContainer.ts @@ -4,7 +4,6 @@ import type { ConnectionId, EnvironmentId, ISearchable } from "../resource"; import { ResourceContainer } from "./resourceContainer"; /** A container {@link TreeItem} for resources to display in the Topics view. */ - export class KafkaClusterResourceContainer extends ResourceContainer { readonly connectionId: ConnectionId; readonly connectionType: ConnectionType; From f07980eccf8f9b19b65ff795d82af0c499dd93ff Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Thu, 26 Feb 2026 12:34:40 -0500 Subject: [PATCH 10/15] swap id generation from label to use replaceAll --- src/models/containers/kafkaClusterResourceContainer.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/models/containers/kafkaClusterResourceContainer.ts b/src/models/containers/kafkaClusterResourceContainer.ts index 30f0668cf..d5753fa74 100644 --- a/src/models/containers/kafkaClusterResourceContainer.ts +++ b/src/models/containers/kafkaClusterResourceContainer.ts @@ -27,8 +27,8 @@ export class KafkaClusterResourceContainer extends Resour super(label, children, contextValue, icon); // convert label to hyphenated id: - // "Consumer Groups" → "consumer-groups", "Topics" → "topics" - const suffix = label.toLowerCase().replace(/\s+/g, "-"); + // "Consumer Groups" -> "consumer-groups", "Topics" -> "topics" + const suffix = label.toLowerCase().replaceAll(/\s+/g, "-"); this.id = `${connectionId}-${clusterId}-${suffix}`; this.connectionId = connectionId; From 25d0ab15f9dafb67d105d871aceaa5e31c5dd953 Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Thu, 26 Feb 2026 14:05:27 -0500 Subject: [PATCH 11/15] simplify constructor to ignore connection/cluster properties --- .../kafkaClusterResourceContainer.test.ts | 174 ++---------------- .../kafkaClusterResourceContainer.ts | 32 +--- 2 files changed, 23 insertions(+), 183 deletions(-) diff --git a/src/models/containers/kafkaClusterResourceContainer.test.ts b/src/models/containers/kafkaClusterResourceContainer.test.ts index a1db301d0..c79585369 100644 --- a/src/models/containers/kafkaClusterResourceContainer.test.ts +++ b/src/models/containers/kafkaClusterResourceContainer.test.ts @@ -1,14 +1,6 @@ import * as assert from "assert"; import type { ThemeIcon } from "vscode"; -import { TEST_DIRECT_CONNECTION_ID } from "../../../tests/unit/testResources/connection"; import { TEST_CCLOUD_CONSUMER_GROUP } from "../../../tests/unit/testResources/consumerGroup"; -import { - TEST_CCLOUD_KAFKA_CLUSTER, - TEST_DIRECT_KAFKA_CLUSTER, - TEST_LOCAL_KAFKA_CLUSTER, -} from "../../../tests/unit/testResources/kafkaCluster"; -import { ConnectionType } from "../../clients/sidecar"; -import { CCLOUD_CONNECTION_ID, LOCAL_CONNECTION_ID } from "../../constants"; import { IconNames } from "../../icons"; import { KafkaClusterResourceContainer } from "./kafkaClusterResourceContainer"; @@ -18,117 +10,35 @@ const TEST_CONTEXT_VALUE = "test-container"; describe("models/containers/kafkaClusterResourceContainer", () => { describe("KafkaClusterResourceContainer", () => { describe("constructor", () => { - it("should set connectionId from constructor argument", () => { - const container = new KafkaClusterResourceContainer( - CCLOUD_CONNECTION_ID, - ConnectionType.Ccloud, - TEST_CCLOUD_KAFKA_CLUSTER.id, - TEST_CCLOUD_KAFKA_CLUSTER.environmentId, - TEST_LABEL, - ); - - assert.strictEqual(container.connectionId, CCLOUD_CONNECTION_ID); - }); - - it("should set connectionType from constructor argument", () => { - const container = new KafkaClusterResourceContainer( - CCLOUD_CONNECTION_ID, - ConnectionType.Ccloud, - TEST_CCLOUD_KAFKA_CLUSTER.id, - TEST_CCLOUD_KAFKA_CLUSTER.environmentId, - TEST_LABEL, - ); - - assert.strictEqual(container.connectionType, ConnectionType.Ccloud); - }); - - it("should set clusterId from constructor argument", () => { - const container = new KafkaClusterResourceContainer( - CCLOUD_CONNECTION_ID, - ConnectionType.Ccloud, - TEST_CCLOUD_KAFKA_CLUSTER.id, - TEST_CCLOUD_KAFKA_CLUSTER.environmentId, - TEST_LABEL, - ); - - assert.strictEqual(container.clusterId, TEST_CCLOUD_KAFKA_CLUSTER.id); - }); - - it("should set environmentId from constructor argument", () => { - const container = new KafkaClusterResourceContainer( - CCLOUD_CONNECTION_ID, - ConnectionType.Ccloud, - TEST_CCLOUD_KAFKA_CLUSTER.id, - TEST_CCLOUD_KAFKA_CLUSTER.environmentId, - TEST_LABEL, - ); - - assert.strictEqual(container.environmentId, TEST_CCLOUD_KAFKA_CLUSTER.environmentId); - }); - it("should set the label from constructor argument", () => { - const container = new KafkaClusterResourceContainer( - CCLOUD_CONNECTION_ID, - ConnectionType.Ccloud, - TEST_CCLOUD_KAFKA_CLUSTER.id, - TEST_CCLOUD_KAFKA_CLUSTER.environmentId, - TEST_LABEL, - ); + const container = new KafkaClusterResourceContainer(TEST_LABEL); assert.strictEqual(container.label, TEST_LABEL); }); it("should set contextValue when provided", () => { - const container = new KafkaClusterResourceContainer( - CCLOUD_CONNECTION_ID, - ConnectionType.Ccloud, - TEST_CCLOUD_KAFKA_CLUSTER.id, - TEST_CCLOUD_KAFKA_CLUSTER.environmentId, - TEST_LABEL, - [], - TEST_CONTEXT_VALUE, - ); + const container = new KafkaClusterResourceContainer(TEST_LABEL, [], TEST_CONTEXT_VALUE); assert.strictEqual(container.contextValue, TEST_CONTEXT_VALUE); }); it("should set icon when provided", () => { - const container = new KafkaClusterResourceContainer( - CCLOUD_CONNECTION_ID, - ConnectionType.Ccloud, - TEST_CCLOUD_KAFKA_CLUSTER.id, - TEST_CCLOUD_KAFKA_CLUSTER.environmentId, - TEST_LABEL, - [], - undefined, - { id: IconNames.CONSUMER_GROUP } as ThemeIcon, - ); + const container = new KafkaClusterResourceContainer(TEST_LABEL, [], undefined, { + id: IconNames.CONSUMER_GROUP, + } as ThemeIcon); assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.CONSUMER_GROUP); }); it("should default to empty children", () => { - const container = new KafkaClusterResourceContainer( - CCLOUD_CONNECTION_ID, - ConnectionType.Ccloud, - TEST_CCLOUD_KAFKA_CLUSTER.id, - TEST_CCLOUD_KAFKA_CLUSTER.environmentId, - TEST_LABEL, - ); + const container = new KafkaClusterResourceContainer(TEST_LABEL); assert.deepStrictEqual(container.children, []); }); it("should accept initial children", () => { const children = [TEST_CCLOUD_CONSUMER_GROUP]; - const container = new KafkaClusterResourceContainer( - CCLOUD_CONNECTION_ID, - ConnectionType.Ccloud, - TEST_CCLOUD_KAFKA_CLUSTER.id, - TEST_CCLOUD_KAFKA_CLUSTER.environmentId, - TEST_LABEL, - children, - ); + const container = new KafkaClusterResourceContainer(TEST_LABEL, children); assert.deepStrictEqual(container.children, children); }); @@ -136,81 +46,25 @@ describe("models/containers/kafkaClusterResourceContainer", () => { describe("id derivation", () => { it("should derive id suffix from single-word label", () => { - const container = new KafkaClusterResourceContainer( - CCLOUD_CONNECTION_ID, - ConnectionType.Ccloud, - TEST_CCLOUD_KAFKA_CLUSTER.id, - TEST_CCLOUD_KAFKA_CLUSTER.environmentId, - "Topics", - ); - - assert.strictEqual( - container.id, - `${CCLOUD_CONNECTION_ID}-${TEST_CCLOUD_KAFKA_CLUSTER.id}-topics`, - ); + const container = new KafkaClusterResourceContainer("Topics"); + + assert.strictEqual(container.id, "kafka-cluster-topics"); }); it("should derive id suffix from multi-word label", () => { - const container = new KafkaClusterResourceContainer( - CCLOUD_CONNECTION_ID, - ConnectionType.Ccloud, - TEST_CCLOUD_KAFKA_CLUSTER.id, - TEST_CCLOUD_KAFKA_CLUSTER.environmentId, - "Consumer Groups", - ); - - assert.strictEqual( - container.id, - `${CCLOUD_CONNECTION_ID}-${TEST_CCLOUD_KAFKA_CLUSTER.id}-consumer-groups`, - ); + const container = new KafkaClusterResourceContainer("Consumer Groups"); + + assert.strictEqual(container.id, "kafka-cluster-consumer-groups"); }); }); describe("loggerName", () => { it("should include label in loggerName", () => { const label = "Test Resources"; - const container = new KafkaClusterResourceContainer( - CCLOUD_CONNECTION_ID, - ConnectionType.Ccloud, - TEST_CCLOUD_KAFKA_CLUSTER.id, - TEST_CCLOUD_KAFKA_CLUSTER.environmentId, - label, - ); + const container = new KafkaClusterResourceContainer(label); assert.strictEqual(container.loggerName, `models.KafkaClusterResourceContainer(${label})`); }); }); - - describe("connection types", () => { - it("should work with Local connection type", () => { - const container = new KafkaClusterResourceContainer( - LOCAL_CONNECTION_ID, - ConnectionType.Local, - TEST_LOCAL_KAFKA_CLUSTER.id, - TEST_LOCAL_KAFKA_CLUSTER.environmentId, - TEST_LABEL, - ); - - assert.strictEqual(container.connectionId, LOCAL_CONNECTION_ID); - assert.strictEqual(container.connectionType, ConnectionType.Local); - assert.strictEqual( - container.id, - `${LOCAL_CONNECTION_ID}-${TEST_LOCAL_KAFKA_CLUSTER.id}-test`, - ); - }); - - it("should work with Direct connection type", () => { - const container = new KafkaClusterResourceContainer( - TEST_DIRECT_CONNECTION_ID, - ConnectionType.Direct, - TEST_DIRECT_KAFKA_CLUSTER.id, - TEST_DIRECT_KAFKA_CLUSTER.environmentId, - TEST_LABEL, - ); - - assert.strictEqual(container.connectionId, TEST_DIRECT_CONNECTION_ID); - assert.strictEqual(container.connectionType, ConnectionType.Direct); - }); - }); }); }); diff --git a/src/models/containers/kafkaClusterResourceContainer.ts b/src/models/containers/kafkaClusterResourceContainer.ts index d5753fa74..5c5a41911 100644 --- a/src/models/containers/kafkaClusterResourceContainer.ts +++ b/src/models/containers/kafkaClusterResourceContainer.ts @@ -1,39 +1,25 @@ import type { ThemeIcon } from "vscode"; -import type { ConnectionType } from "../../clients/sidecar"; -import type { ConnectionId, EnvironmentId, ISearchable } from "../resource"; +import type { ISearchable } from "../resource"; import { ResourceContainer } from "./resourceContainer"; +/** Labels for the top-level containers in the Topics view. */ +export enum KafkaClusterContainerLabel { + TOPICS = "Topics", + CONSUMER_GROUPS = "Consumer Groups", +} + /** A container {@link TreeItem} for resources to display in the Topics view. */ export class KafkaClusterResourceContainer extends ResourceContainer { - readonly connectionId: ConnectionId; - readonly connectionType: ConnectionType; - readonly clusterId: string; - readonly environmentId: EnvironmentId; - get loggerName() { return `models.KafkaClusterResourceContainer(${this.label})`; } - constructor( - connectionId: ConnectionId, - connectionType: ConnectionType, - clusterId: string, - environmentId: EnvironmentId, - label: string, - children: T[] = [], - contextValue?: string, - icon?: ThemeIcon, - ) { + constructor(label: string, children: T[] = [], contextValue?: string, icon?: ThemeIcon) { super(label, children, contextValue, icon); // convert label to hyphenated id: // "Consumer Groups" -> "consumer-groups", "Topics" -> "topics" const suffix = label.toLowerCase().replaceAll(/\s+/g, "-"); - this.id = `${connectionId}-${clusterId}-${suffix}`; - - this.connectionId = connectionId; - this.connectionType = connectionType; - this.clusterId = clusterId; - this.environmentId = environmentId; + this.id = `kafka-cluster-${suffix}`; } } From 978894dd21f6f5616df812638c9ba095a8b12b39 Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Thu, 26 Feb 2026 15:13:30 -0500 Subject: [PATCH 12/15] require connection ID+type for ResourceContainer to play nicely with BaseViewProviderData in downstream branch --- .../flinkDatabaseResourceContainer.ts | 7 +- .../kafkaClusterResourceContainer.test.ts | 94 +++++++++++++++++-- .../kafkaClusterResourceContainer.ts | 14 ++- .../containers/resourceContainer.test.ts | 4 +- src/models/containers/resourceContainer.ts | 18 +++- 5 files changed, 116 insertions(+), 21 deletions(-) diff --git a/src/models/containers/flinkDatabaseResourceContainer.ts b/src/models/containers/flinkDatabaseResourceContainer.ts index 24686e5f3..9911de1a9 100644 --- a/src/models/containers/flinkDatabaseResourceContainer.ts +++ b/src/models/containers/flinkDatabaseResourceContainer.ts @@ -3,7 +3,6 @@ import { ConnectionType } from "../../clients/sidecar"; import { CCLOUD_CONNECTION_ID } from "../../constants"; import type { FlinkArtifact } from "../flinkArtifact"; import type { FlinkDatabaseResource } from "../flinkDatabaseResource"; -import type { ConnectionId } from "../resource"; import { ResourceContainer } from "./resourceContainer"; /** Labels for the top-level containers in the Flink Database view. */ @@ -21,15 +20,13 @@ export enum FlinkDatabaseContainerLabel { export class FlinkDatabaseResourceContainer< T extends FlinkDatabaseResource | FlinkArtifact, > extends ResourceContainer { - readonly connectionId: ConnectionId = CCLOUD_CONNECTION_ID; - readonly connectionType: ConnectionType = ConnectionType.Ccloud; - get loggerName() { return `models.FlinkDatabaseResourceContainer(${this.label})`; } constructor(label: string, children: T[], contextValue?: string, icon?: ThemeIcon) { - super(label, children, contextValue, icon); + // Flink Database resources are always for the CCLOUD connection + super(CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, label, children, contextValue, icon); this.id = `${CCLOUD_CONNECTION_ID}-${label}`; } diff --git a/src/models/containers/kafkaClusterResourceContainer.test.ts b/src/models/containers/kafkaClusterResourceContainer.test.ts index c79585369..2fc3c42f4 100644 --- a/src/models/containers/kafkaClusterResourceContainer.test.ts +++ b/src/models/containers/kafkaClusterResourceContainer.test.ts @@ -1,6 +1,9 @@ import * as assert from "assert"; import type { ThemeIcon } from "vscode"; +import { TEST_DIRECT_CONNECTION_ID } from "../../../tests/unit/testResources/connection"; import { TEST_CCLOUD_CONSUMER_GROUP } from "../../../tests/unit/testResources/consumerGroup"; +import { ConnectionType } from "../../clients/sidecar"; +import { CCLOUD_CONNECTION_ID } from "../../constants"; import { IconNames } from "../../icons"; import { KafkaClusterResourceContainer } from "./kafkaClusterResourceContainer"; @@ -10,35 +13,79 @@ const TEST_CONTEXT_VALUE = "test-container"; describe("models/containers/kafkaClusterResourceContainer", () => { describe("KafkaClusterResourceContainer", () => { describe("constructor", () => { + it("should set connectionId from constructor argument", () => { + const container = new KafkaClusterResourceContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_LABEL, + ); + + assert.strictEqual(container.connectionId, CCLOUD_CONNECTION_ID); + }); + + it("should set connectionType from constructor argument", () => { + const container = new KafkaClusterResourceContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_LABEL, + ); + + assert.strictEqual(container.connectionType, ConnectionType.Ccloud); + }); + it("should set the label from constructor argument", () => { - const container = new KafkaClusterResourceContainer(TEST_LABEL); + const container = new KafkaClusterResourceContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_LABEL, + ); assert.strictEqual(container.label, TEST_LABEL); }); it("should set contextValue when provided", () => { - const container = new KafkaClusterResourceContainer(TEST_LABEL, [], TEST_CONTEXT_VALUE); + const container = new KafkaClusterResourceContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_LABEL, + [], + TEST_CONTEXT_VALUE, + ); assert.strictEqual(container.contextValue, TEST_CONTEXT_VALUE); }); it("should set icon when provided", () => { - const container = new KafkaClusterResourceContainer(TEST_LABEL, [], undefined, { - id: IconNames.CONSUMER_GROUP, - } as ThemeIcon); + const container = new KafkaClusterResourceContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_LABEL, + [], + undefined, + { id: IconNames.CONSUMER_GROUP } as ThemeIcon, + ); assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.CONSUMER_GROUP); }); it("should default to empty children", () => { - const container = new KafkaClusterResourceContainer(TEST_LABEL); + const container = new KafkaClusterResourceContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_LABEL, + ); assert.deepStrictEqual(container.children, []); }); it("should accept initial children", () => { const children = [TEST_CCLOUD_CONSUMER_GROUP]; - const container = new KafkaClusterResourceContainer(TEST_LABEL, children); + const container = new KafkaClusterResourceContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_LABEL, + children, + ); assert.deepStrictEqual(container.children, children); }); @@ -46,22 +93,49 @@ describe("models/containers/kafkaClusterResourceContainer", () => { describe("id derivation", () => { it("should derive id suffix from single-word label", () => { - const container = new KafkaClusterResourceContainer("Topics"); + const container = new KafkaClusterResourceContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + "Topics", + ); assert.strictEqual(container.id, "kafka-cluster-topics"); }); it("should derive id suffix from multi-word label", () => { - const container = new KafkaClusterResourceContainer("Consumer Groups"); + const container = new KafkaClusterResourceContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + "Consumer Groups", + ); assert.strictEqual(container.id, "kafka-cluster-consumer-groups"); }); + + it("should use the same id regardless of connection type", () => { + const ccloudContainer = new KafkaClusterResourceContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + TEST_LABEL, + ); + const directContainer = new KafkaClusterResourceContainer( + TEST_DIRECT_CONNECTION_ID, + ConnectionType.Direct, + TEST_LABEL, + ); + + assert.strictEqual(ccloudContainer.id, directContainer.id); + }); }); describe("loggerName", () => { it("should include label in loggerName", () => { const label = "Test Resources"; - const container = new KafkaClusterResourceContainer(label); + const container = new KafkaClusterResourceContainer( + CCLOUD_CONNECTION_ID, + ConnectionType.Ccloud, + label, + ); assert.strictEqual(container.loggerName, `models.KafkaClusterResourceContainer(${label})`); }); diff --git a/src/models/containers/kafkaClusterResourceContainer.ts b/src/models/containers/kafkaClusterResourceContainer.ts index 5c5a41911..334d60511 100644 --- a/src/models/containers/kafkaClusterResourceContainer.ts +++ b/src/models/containers/kafkaClusterResourceContainer.ts @@ -1,5 +1,6 @@ import type { ThemeIcon } from "vscode"; -import type { ISearchable } from "../resource"; +import type { ConnectionType } from "../../clients/sidecar"; +import type { ConnectionId, ISearchable } from "../resource"; import { ResourceContainer } from "./resourceContainer"; /** Labels for the top-level containers in the Topics view. */ @@ -14,8 +15,15 @@ export class KafkaClusterResourceContainer extends Resour return `models.KafkaClusterResourceContainer(${this.label})`; } - constructor(label: string, children: T[] = [], contextValue?: string, icon?: ThemeIcon) { - super(label, children, contextValue, icon); + constructor( + connectionId: ConnectionId, + connectionType: ConnectionType, + label: string, + children: T[] = [], + contextValue?: string, + icon?: ThemeIcon, + ) { + super(connectionId, connectionType, label, children, contextValue, icon); // convert label to hyphenated id: // "Consumer Groups" -> "consumer-groups", "Topics" -> "topics" diff --git a/src/models/containers/resourceContainer.test.ts b/src/models/containers/resourceContainer.test.ts index a530a412e..bc7af727b 100644 --- a/src/models/containers/resourceContainer.test.ts +++ b/src/models/containers/resourceContainer.test.ts @@ -2,6 +2,8 @@ import * as assert from "assert"; import * as sinon from "sinon"; import { ThemeIcon, TreeItemCollapsibleState } from "vscode"; import { createTestResource } from "../../../tests/unit/testResources/base"; +import { ConnectionType } from "../../clients/sidecar"; +import { CCLOUD_CONNECTION_ID } from "../../constants"; import { ERROR_ICON, IconNames } from "../../icons"; import type { BaseViewProviderData } from "../../viewProviders/baseModels/base"; import { LOADING_POLL_INTERVAL_MS, ResourceContainer } from "./resourceContainer"; @@ -16,7 +18,7 @@ class TestContainer extends ResourceContainer { contextValue?: string, icon?: ThemeIcon, ) { - super(label, children, contextValue, icon); + super(CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, label, children, contextValue, icon); this.id = `test-${label}`; } } diff --git a/src/models/containers/resourceContainer.ts b/src/models/containers/resourceContainer.ts index b64ffd3d5..aa88acf64 100644 --- a/src/models/containers/resourceContainer.ts +++ b/src/models/containers/resourceContainer.ts @@ -1,7 +1,8 @@ import { ThemeIcon, TreeItem, TreeItemCollapsibleState } from "vscode"; +import type { ConnectionType } from "../../clients/sidecar"; import { ERROR_ICON, IconNames } from "../../icons"; import { Logger } from "../../logging"; -import type { ISearchable } from "../resource"; +import type { ConnectionId, ISearchable } from "../resource"; /** Poll interval to use when waiting for a container to finish loading. */ export const LOADING_POLL_INTERVAL_MS = 100; @@ -17,6 +18,10 @@ export abstract class ResourceContainer // enforce string so subclasses set this after super() declare id: string; + // IResourceBase fields required by BaseViewProviderData + readonly connectionId: ConnectionId; + readonly connectionType: ConnectionType; + abstract loggerName: string; private _children: T[]; @@ -26,9 +31,18 @@ export abstract class ResourceContainer protected readonly _defaultContextValue: string | undefined; protected readonly _defaultIcon: ThemeIcon | undefined; - protected constructor(label: string, children: T[], contextValue?: string, icon?: ThemeIcon) { + protected constructor( + connectionId: ConnectionId, + connectionType: ConnectionType, + label: string, + children: T[], + contextValue?: string, + icon?: ThemeIcon, + ) { super(label, TreeItemCollapsibleState.Collapsed); + this.connectionId = connectionId; + this.connectionType = connectionType; this._children = children; this._defaultContextValue = contextValue; From ec16f3d7b1a9f1002d4896619e92d4292f924853 Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Thu, 26 Feb 2026 17:55:09 -0500 Subject: [PATCH 13/15] migrate setter methods to dedicated methods --- .../containers/resourceContainer.test.ts | 271 ++++++++++-------- src/models/containers/resourceContainer.ts | 58 ++-- 2 files changed, 181 insertions(+), 148 deletions(-) diff --git a/src/models/containers/resourceContainer.test.ts b/src/models/containers/resourceContainer.test.ts index bc7af727b..cc0a59b7c 100644 --- a/src/models/containers/resourceContainer.test.ts +++ b/src/models/containers/resourceContainer.test.ts @@ -1,6 +1,6 @@ import * as assert from "assert"; import * as sinon from "sinon"; -import { ThemeIcon, TreeItemCollapsibleState } from "vscode"; +import { MarkdownString, ThemeIcon, TreeItemCollapsibleState } from "vscode"; import { createTestResource } from "../../../tests/unit/testResources/base"; import { ConnectionType } from "../../clients/sidecar"; import { CCLOUD_CONNECTION_ID } from "../../constants"; @@ -84,231 +84,264 @@ describe("models/containers/resourceContainer.ts", () => { assert.strictEqual(container.contextValue, undefined); }); - it("should leave description undefined before children setter is called", () => { + it("should leave description undefined before any state transition", () => { const container = new TestContainer(testLabel, []); assert.strictEqual(container.description, undefined); }); - }); - describe("children setter", () => { - it("should update description when `children` is set", () => { + it("should start with isLoading=false and hasError=false", () => { const container = new TestContainer(testLabel, []); - // description is not set in constructor (before loading) - assert.strictEqual(container.description, undefined); - - // but is set when children is set - container.children = testResources; - assert.strictEqual(container.description, `(${testResources.length})`); - // and updates when children change - container.children = []; - assert.strictEqual(container.description, "(0)"); + assert.strictEqual(container.isLoading, false); + assert.strictEqual(container.hasError, false); }); + }); - it("should clear isLoading state when children are set", () => { + describe("searchableText", () => { + it("should return the label as searchable text", () => { const container = new TestContainer(testLabel, []); - container.isLoading = true; - - container.children = [testResource]; - assert.strictEqual(container.isLoading, false); + assert.strictEqual(container.searchableText(), testLabel); }); + }); - it("should clear hasError when non-empty children are set", () => { + describe("setLoading()", () => { + it("should set isLoading to true and hasError to false", () => { const container = new TestContainer(testLabel, []); - container.hasError = true; - container.children = [testResource]; + container.setLoading(); + assert.strictEqual(container.isLoading, true); assert.strictEqual(container.hasError, false); }); - it("should preserve hasError when empty children are set", () => { + it("should use the loading icon", () => { const container = new TestContainer(testLabel, []); - container.hasError = true; - container.children = []; + container.setLoading(); - assert.strictEqual(container.hasError, true); + assert.ok(container.iconPath); + assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.LOADING); + }); + + it("should clear a previous error state", () => { + const container = new TestContainer(testLabel, [], testContextValue); + container.setError("some error"); + + container.setLoading(); + + assert.strictEqual(container.hasError, false); + assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.LOADING); }); }); - describe("searchableText", () => { - it("should return the label as searchable text", () => { + describe("setLoaded()", () => { + it("should set children and update description", () => { const container = new TestContainer(testLabel, []); - assert.strictEqual(container.searchableText(), testLabel); + container.setLoaded(testResources); + + assert.deepStrictEqual(container.children, testResources); + assert.strictEqual(container.description, `(${testResources.length})`); }); - }); - describe("isLoading", () => { - it("should start with isLoading set to false", () => { + it("should update description for empty children", () => { const container = new TestContainer(testLabel, []); - assert.strictEqual(container.isLoading, false); + container.setLoaded([]); + + assert.strictEqual(container.description, "(0)"); }); - it("should use the loading icon when isLoading is set to true", () => { + it("should clear isLoading and hasError", () => { const container = new TestContainer(testLabel, []); + container.setLoading(); - container.isLoading = true; + container.setLoaded([testResource]); - assert.ok(container.iconPath); - assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.LOADING); + assert.strictEqual(container.isLoading, false); + assert.strictEqual(container.hasError, false); }); - it("should use the default icon when isLoading is set to false", () => { + it("should restore the default icon", () => { const icon = new ThemeIcon("symbol-folder"); const container = new TestContainer(testLabel, [], undefined, icon); + container.setLoading(); - container.isLoading = true; - assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.LOADING); + container.setLoaded([testResource]); - container.isLoading = false; assert.strictEqual(container.iconPath, icon); }); - it("should clear .iconPath when isLoading is set to false and no default icon is provided", () => { - const container = new TestContainer( - "A", - [], - // no default icon (nor contextValue) set - ); + it("should clear iconPath when no default icon is provided", () => { + const container = new TestContainer(testLabel, []); + container.setLoading(); - // set initial loading state - container.isLoading = true; - assert.ok(container.iconPath); - assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.LOADING); + container.setLoaded([testResource]); - container.isLoading = false; assert.strictEqual(container.iconPath, undefined); }); - }); - describe("hasError", () => { - it("should start with hasError set to false", () => { + it("should clear tooltip", () => { const container = new TestContainer(testLabel, []); - assert.strictEqual(container.hasError, false); + container.setError("some error"); + + container.setLoaded([testResource]); + + assert.strictEqual(container.tooltip, undefined); + }); + + it("should restore contextValue to default (clearing -error suffix)", () => { + const container = new TestContainer(testLabel, [], testContextValue); + container.setError("some error"); + assert.strictEqual(container.contextValue, `${testContextValue}-error`); + + container.setLoaded([testResource]); + + assert.strictEqual(container.contextValue, testContextValue); }); - it("should use the error icon when hasError is set to true", () => { + it("should not modify contextValue when none was provided", () => { const container = new TestContainer(testLabel, []); - container.hasError = true; + container.setLoaded([testResource]); - assert.deepStrictEqual(container.iconPath, ERROR_ICON); + assert.strictEqual(container.contextValue, undefined); }); + }); - it("should use the default icon when hasError is set to false", () => { - const icon = new ThemeIcon("symbol-folder"); - const container = new TestContainer(testLabel, [], undefined, icon); + describe("setError()", () => { + it("should set hasError to true and clear isLoading", () => { + const container = new TestContainer(testLabel, []); + container.setLoading(); - container.hasError = true; - assert.deepStrictEqual(container.iconPath, ERROR_ICON); + container.setError("something went wrong"); - container.hasError = false; - assert.strictEqual(container.iconPath, icon); + assert.strictEqual(container.hasError, true); + assert.strictEqual(container.isLoading, false); }); - it("should clear .iconPath when hasError is set to false and no default icon is provided", () => { + it("should clear children and set description to (0)", () => { const container = new TestContainer(testLabel, []); + container.setLoaded(testResources); - container.hasError = true; - assert.deepStrictEqual(container.iconPath, ERROR_ICON); + container.setError("something went wrong"); - container.hasError = false; - assert.strictEqual(container.iconPath, undefined); + assert.deepStrictEqual(container.children, []); + assert.strictEqual(container.description, "(0)"); }); - it("should toggle the contextValue between error and non-error states without suffix duplication", () => { - const container = new TestContainer(testLabel, [], testContextValue); + it("should use the error icon", () => { + const container = new TestContainer(testLabel, []); - container.hasError = true; - assert.strictEqual(container.contextValue, `${testContextValue}-error`); + container.setError("something went wrong"); - container.hasError = false; - assert.strictEqual(container.contextValue, testContextValue); + assert.deepStrictEqual(container.iconPath, ERROR_ICON); + }); - // verify no suffix duplication on repeated toggles - container.hasError = true; - assert.strictEqual(container.contextValue, `${testContextValue}-error`); + it("should set the tooltip from a string", () => { + const container = new TestContainer(testLabel, []); + + container.setError("something went wrong"); + + assert.strictEqual(container.tooltip, "something went wrong"); }); - it("should not modify .contextValue when no original contextValue was provided in the constructor", () => { + it("should set the tooltip from a MarkdownString", () => { const container = new TestContainer(testLabel, []); + const markdown = new MarkdownString("**Error**: something went wrong"); - container.hasError = true; - assert.strictEqual(container.contextValue, undefined); + container.setError(markdown); - container.hasError = false; - assert.strictEqual(container.contextValue, undefined); + assert.strictEqual(container.tooltip, markdown); + }); + + it("should append -error suffix to contextValue", () => { + const container = new TestContainer(testLabel, [], testContextValue); + + container.setError("something went wrong"); + + assert.strictEqual(container.contextValue, `${testContextValue}-error`); }); - }); - describe("state interactions", () => { - it("should settle loading state and description when setting children", () => { + it("should not modify contextValue when none was provided", () => { const container = new TestContainer(testLabel, []); - container.isLoading = true; - container.children = [testResource]; + container.setError("something went wrong"); - assert.strictEqual(container.isLoading, false); - assert.strictEqual(container.description, "(1)"); - assert.strictEqual(container.iconPath, undefined); + assert.strictEqual(container.contextValue, undefined); }); + }); - it("should clear hasError when setting non-empty children", () => { + describe("state transitions", () => { + it("loading -> loaded: should settle all state correctly", () => { const container = new TestContainer(testLabel, [], testContextValue); - container.hasError = true; + container.setLoading(); - container.children = [testResource]; + container.setLoaded([testResource]); + assert.strictEqual(container.isLoading, false); assert.strictEqual(container.hasError, false); assert.strictEqual(container.description, "(1)"); assert.strictEqual(container.iconPath, undefined); + assert.strictEqual(container.tooltip, undefined); assert.strictEqual(container.contextValue, testContextValue); }); - it("should not clear hasError when setting empty children array", () => { + it("loading -> error: should settle all state correctly", () => { const container = new TestContainer(testLabel, [], testContextValue); - container.hasError = true; + container.setLoading(); - container.children = []; + container.setError("failure"); + assert.strictEqual(container.isLoading, false); assert.strictEqual(container.hasError, true); + assert.deepStrictEqual(container.children, []); assert.strictEqual(container.description, "(0)"); + assert.deepStrictEqual(container.iconPath, ERROR_ICON); + assert.strictEqual(container.tooltip, "failure"); assert.strictEqual(container.contextValue, `${testContextValue}-error`); }); - it("should handle multiple state transitions", () => { + it("error -> loading -> loaded: full recovery cycle", () => { const container = new TestContainer(testLabel, [], testContextValue); - container.isLoading = true; - assert.ok(container.iconPath); + container.setError("initial failure"); + assert.strictEqual(container.hasError, true); - container.hasError = true; - assert.strictEqual(container.contextValue, `${testContextValue}-error`); - assert.ok(container.iconPath); + container.setLoading(); + assert.strictEqual(container.isLoading, true); + assert.strictEqual(container.hasError, false); - container.children = [testResource]; + container.setLoaded([testResource]); assert.strictEqual(container.isLoading, false); assert.strictEqual(container.hasError, false); assert.strictEqual(container.contextValue, testContextValue); - assert.strictEqual(container.iconPath, undefined); + assert.deepStrictEqual(container.children, [testResource]); }); - it("should handle error recovery with empty then non-empty children", () => { - const container = new TestContainer(testLabel, [], testContextValue); + it("loaded -> loading -> error -> loading -> loaded: repeated transitions", () => { + const icon = new ThemeIcon("symbol-folder"); + const container = new TestContainer(testLabel, [], testContextValue, icon); - container.hasError = true; - container.children = []; - assert.strictEqual(container.hasError, true); + container.setLoaded(testResources); + assert.strictEqual(container.iconPath, icon); + + container.setLoading(); + assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.LOADING); + + container.setError("transient failure"); + assert.deepStrictEqual(container.iconPath, ERROR_ICON); assert.strictEqual(container.contextValue, `${testContextValue}-error`); - container.children = [testResource]; - assert.strictEqual(container.hasError, false); + container.setLoading(); + assert.strictEqual((container.iconPath as ThemeIcon).id, IconNames.LOADING); + + container.setLoaded([testResource]); + assert.strictEqual(container.iconPath, icon); assert.strictEqual(container.contextValue, testContextValue); + assert.strictEqual(container.tooltip, undefined); }); }); @@ -322,20 +355,18 @@ describe("models/containers/resourceContainer.ts", () => { }); it("should resolve immediately when not loading", async () => { - container.isLoading = false; - await container.ensureDoneLoading(); assert.strictEqual(container.isLoading, false); }); it("should wait for loading to complete", async () => { - container.isLoading = true; + container.setLoading(); const waitPromise = container.ensureDoneLoading(); await clock.tickAsync(200); - container.isLoading = false; + container.setLoaded([]); await clock.tickAsync(LOADING_POLL_INTERVAL_MS + 1); await waitPromise; @@ -343,7 +374,7 @@ describe("models/containers/resourceContainer.ts", () => { }); it("should timeout if loading never completes", async () => { - container.isLoading = true; + container.setLoading(); const timeoutMs = 500; const waitPromise = container.ensureDoneLoading(timeoutMs); @@ -364,7 +395,7 @@ describe("models/containers/resourceContainer.ts", () => { it("should return children after calling ensureDoneLoading", async () => { const resources = [testResource]; - container.children = resources; + container.setLoaded(resources); const result = await container.gatherResources(); diff --git a/src/models/containers/resourceContainer.ts b/src/models/containers/resourceContainer.ts index aa88acf64..9da019d7a 100644 --- a/src/models/containers/resourceContainer.ts +++ b/src/models/containers/resourceContainer.ts @@ -1,4 +1,4 @@ -import { ThemeIcon, TreeItem, TreeItemCollapsibleState } from "vscode"; +import { type MarkdownString, ThemeIcon, TreeItem, TreeItemCollapsibleState } from "vscode"; import type { ConnectionType } from "../../clients/sidecar"; import { ERROR_ICON, IconNames } from "../../icons"; import { Logger } from "../../logging"; @@ -62,47 +62,49 @@ export abstract class ResourceContainer return this._logger; } - /** - * Child resources belonging to this container. - * Setting this will clear the internal {@linkcode isLoading} state. - * If the children array has items, this will also set {@linkcode hasError} to `false`. - */ + /** Child resources belonging to this container. */ get children(): T[] { return this._children; } - set children(children: T[]) { - this._children = children; - this.isLoading = false; - this.description = `(${children.length})`; - - if (children.length > 0) { - this.hasError = false; - } - } - get isLoading(): boolean { return this._isLoading; } - set isLoading(loading: boolean) { - this._isLoading = loading; - this.iconPath = loading ? new ThemeIcon(IconNames.LOADING) : this._defaultIcon; - } - get hasError(): boolean { return this._hasError; } - /** Set or clear the error state for this container. */ - set hasError(error: boolean) { - this._hasError = error; - this.iconPath = error ? ERROR_ICON : this._defaultIcon; + /** Transition to loading state. Shows loading spinner icon. */ + setLoading(): void { + this._isLoading = true; + this._hasError = false; + this.iconPath = new ThemeIcon(IconNames.LOADING); + } + + /** Transition to loaded state with results. Clears loading, error, and tooltip. */ + setLoaded(children: T[]): void { + this._children = children; + this._isLoading = false; + this._hasError = false; + this.description = `(${children.length})`; + this.iconPath = this._defaultIcon; + this.tooltip = undefined; + if (this._defaultContextValue) { + this.contextValue = this._defaultContextValue; + } + } + /** Transition to error state. Sets error icon, clears children, sets error tooltip. */ + setError(tooltip: string | MarkdownString): void { + this._children = []; + this._isLoading = false; + this._hasError = true; + this.description = "(0)"; + this.iconPath = ERROR_ICON; + this.tooltip = tooltip; if (this._defaultContextValue) { - // append or remove "-error" suffix to context value based on error state to toggle enablement - // of resource-specific commands - this.contextValue = error ? `${this._defaultContextValue}-error` : this._defaultContextValue; + this.contextValue = `${this._defaultContextValue}-error`; } } From e4d9676afe077422a3052243393b50111678ccb3 Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Fri, 27 Feb 2026 10:21:16 -0500 Subject: [PATCH 14/15] bring container method migration changes up from djs/consumer-groups-in-view --- src/viewProviders/flinkDatabase.test.ts | 26 ++++++++++++------------- src/viewProviders/flinkDatabase.ts | 17 +++++----------- 2 files changed, 18 insertions(+), 25 deletions(-) diff --git a/src/viewProviders/flinkDatabase.test.ts b/src/viewProviders/flinkDatabase.test.ts index 8aeb182d5..494a1506a 100644 --- a/src/viewProviders/flinkDatabase.test.ts +++ b/src/viewProviders/flinkDatabase.test.ts @@ -251,7 +251,7 @@ describe("viewProviders/flinkDatabase.ts", () => { it("should return the FlinkRelation parent for a FlinkRelationColumn", () => { const testRelation = TEST_FLINK_RELATION; const testColumn = TEST_VARCHAR_COLUMN; - viewProvider.relationsContainer.children = [testRelation]; + viewProvider.relationsContainer.setLoaded([testRelation]); const parent = viewProvider.getParent(testColumn); @@ -260,7 +260,7 @@ describe("viewProviders/flinkDatabase.ts", () => { it("should return undefined when a FlinkRelationColumn parent is not found", () => { const testColumn = TEST_VARCHAR_COLUMN; - viewProvider.relationsContainer.children = []; + viewProvider.relationsContainer.setLoaded([]); const parent = viewProvider.getParent(testColumn); @@ -352,7 +352,7 @@ describe("viewProviders/flinkDatabase.ts", () => { it("should reveal a FlinkArtifact by finding it in the artifacts container", async () => { const testArtifact = createFlinkArtifact({ id: "art1", name: "TestArtifact" }); - viewProvider.artifactsContainer.children = [testArtifact]; + viewProvider.artifactsContainer.setLoaded([testArtifact]); await viewProvider.revealResource(testArtifact); @@ -362,7 +362,7 @@ describe("viewProviders/flinkDatabase.ts", () => { it("should reveal a FlinkUdf by finding it in the UDFs container", async () => { const testUdf = createFlinkUDF("TestUDF"); - viewProvider.udfsContainer.children = [testUdf]; + viewProvider.udfsContainer.setLoaded([testUdf]); await viewProvider.revealResource(testUdf); @@ -372,7 +372,7 @@ describe("viewProviders/flinkDatabase.ts", () => { it("should reveal a FlinkRelation by finding it in the relations container", async () => { const testRelation = TEST_FLINK_RELATION; - viewProvider.relationsContainer.children = [testRelation]; + viewProvider.relationsContainer.setLoaded([testRelation]); await viewProvider.revealResource(testRelation); @@ -383,7 +383,7 @@ describe("viewProviders/flinkDatabase.ts", () => { it("should reveal a FlinkRelationColumn by finding its parent relation", async () => { const testRelation = TEST_FLINK_RELATION; const testColumn = TEST_VARCHAR_COLUMN; - viewProvider.relationsContainer.children = [testRelation]; + viewProvider.relationsContainer.setLoaded([testRelation]); await viewProvider.revealResource(testColumn); @@ -394,7 +394,7 @@ describe("viewProviders/flinkDatabase.ts", () => { it("should reveal a FlinkAIConnection by finding it in the AI connections container", async () => { const testConnection = createFlinkAIConnection("TestConnection"); - viewProvider.aiConnectionsContainer.children = [testConnection]; + viewProvider.aiConnectionsContainer.setLoaded([testConnection]); await viewProvider.revealResource(testConnection); @@ -404,7 +404,7 @@ describe("viewProviders/flinkDatabase.ts", () => { it("should reveal a FlinkAITool by finding it in the AI tools container", async () => { const testTool = createFlinkAITool("TestTool"); - viewProvider.aiToolsContainer.children = [testTool]; + viewProvider.aiToolsContainer.setLoaded([testTool]); await viewProvider.revealResource(testTool); @@ -414,7 +414,7 @@ describe("viewProviders/flinkDatabase.ts", () => { it("should reveal a FlinkAIModel by finding it in the AI models container", async () => { const testModel = createFlinkAIModel("TestModel"); - viewProvider.aiModelsContainer.children = [testModel]; + viewProvider.aiModelsContainer.setLoaded([testModel]); await viewProvider.revealResource(testModel); @@ -424,7 +424,7 @@ describe("viewProviders/flinkDatabase.ts", () => { it("should reveal a FlinkAIAgent by finding it in the AI agents container", async () => { const testAgent = createFlinkAIAgent("TestAgent"); - viewProvider.aiAgentsContainer.children = [testAgent]; + viewProvider.aiAgentsContainer.setLoaded([testAgent]); await viewProvider.revealResource(testAgent); @@ -437,7 +437,7 @@ describe("viewProviders/flinkDatabase.ts", () => { id: "nope-this-belongs-somewhere-else", name: "TestArtifact", }); - viewProvider.artifactsContainer.children = []; + viewProvider.artifactsContainer.setLoaded([]); await viewProvider.revealResource(testArtifact); @@ -446,7 +446,7 @@ describe("viewProviders/flinkDatabase.ts", () => { it("should use custom options when provided", async () => { const testArtifact = createFlinkArtifact({ id: "art1", name: "TestArtifact" }); - viewProvider.artifactsContainer.children = [testArtifact]; + viewProvider.artifactsContainer.setLoaded([testArtifact]); const customOptions = { select: false, @@ -461,7 +461,7 @@ describe("viewProviders/flinkDatabase.ts", () => { it("should handle treeView.reveal errors gracefully", async () => { const testArtifact = createFlinkArtifact({ id: "art1", name: "TestArtifact" }); - viewProvider.artifactsContainer.children = [testArtifact]; + viewProvider.artifactsContainer.setLoaded([testArtifact]); const error = new Error("TreeView reveal failed"); treeViewRevealStub.rejects(error); diff --git a/src/viewProviders/flinkDatabase.ts b/src/viewProviders/flinkDatabase.ts index c3f73a234..c4ac9c513 100644 --- a/src/viewProviders/flinkDatabase.ts +++ b/src/viewProviders/flinkDatabase.ts @@ -329,17 +329,13 @@ export class FlinkDatabaseViewProvider extends ParentedBaseViewProvider< this.logger.debug( `refreshing ${container.label} resources for ${database.name} (${database.id})...`, ); - // set initial loading state - container.isLoading = true; + container.setLoading(); this._onDidChangeTreeData.fire(container); let results: T[] = []; try { results = await loaderMethod(database, forceDeepRefresh); - // clear any loading/error state and only refresh the provided container to show updated items - container.children = results; - container.tooltip = new CustomMarkdownString(); - container.hasError = false; + container.setLoaded(results); this._onDidChangeTreeData.fire(container); } catch (error) { let errorMsg = String(error); @@ -353,12 +349,9 @@ export class FlinkDatabaseViewProvider extends ParentedBaseViewProvider< } const msg = `Failed to load ${container.label} for **${database.name}** (${database.id}):`; logError(error, `${msg} ${errorMsg}`); - // clear the loading state and show error info as tooltip (and icon through setting hasError) - container.children = []; - container.tooltip = new CustomMarkdownString() - .addWarning(msg) - .addCodeBlock(errorMsg, errorLanguage); - container.hasError = true; + container.setError( + new CustomMarkdownString().addWarning(msg).addCodeBlock(errorMsg, errorLanguage), + ); this._onDidChangeTreeData.fire(container); } return results; From 85d54c7ee46f23415a428a0ce89b050b772dabba Mon Sep 17 00:00:00 2001 From: Dave Shoup Date: Fri, 27 Feb 2026 17:26:15 -0500 Subject: [PATCH 15/15] simplify container constructor and required properties for subclasses --- .../flinkDatabaseResourceContainer.ts | 6 +---- .../kafkaClusterResourceContainer.test.ts | 14 +++++------ .../kafkaClusterResourceContainer.ts | 24 ++----------------- .../containers/resourceContainer.test.ts | 5 ++-- src/models/containers/resourceContainer.ts | 16 +++++++++---- 5 files changed, 23 insertions(+), 42 deletions(-) diff --git a/src/models/containers/flinkDatabaseResourceContainer.ts b/src/models/containers/flinkDatabaseResourceContainer.ts index 9911de1a9..1bed879dc 100644 --- a/src/models/containers/flinkDatabaseResourceContainer.ts +++ b/src/models/containers/flinkDatabaseResourceContainer.ts @@ -20,14 +20,10 @@ export enum FlinkDatabaseContainerLabel { export class FlinkDatabaseResourceContainer< T extends FlinkDatabaseResource | FlinkArtifact, > extends ResourceContainer { - get loggerName() { - return `models.FlinkDatabaseResourceContainer(${this.label})`; - } + protected readonly loggerNamePrefix = "FlinkDatabaseResourceContainer"; constructor(label: string, children: T[], contextValue?: string, icon?: ThemeIcon) { // Flink Database resources are always for the CCLOUD connection super(CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, label, children, contextValue, icon); - - this.id = `${CCLOUD_CONNECTION_ID}-${label}`; } } diff --git a/src/models/containers/kafkaClusterResourceContainer.test.ts b/src/models/containers/kafkaClusterResourceContainer.test.ts index 2fc3c42f4..63160a453 100644 --- a/src/models/containers/kafkaClusterResourceContainer.test.ts +++ b/src/models/containers/kafkaClusterResourceContainer.test.ts @@ -92,27 +92,27 @@ describe("models/containers/kafkaClusterResourceContainer", () => { }); describe("id derivation", () => { - it("should derive id suffix from single-word label", () => { + it("should derive id from connectionId and label", () => { const container = new KafkaClusterResourceContainer( CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, "Topics", ); - assert.strictEqual(container.id, "kafka-cluster-topics"); + assert.strictEqual(container.id, `${CCLOUD_CONNECTION_ID}-Topics`); }); - it("should derive id suffix from multi-word label", () => { + it("should preserve multi-word labels in id", () => { const container = new KafkaClusterResourceContainer( CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, "Consumer Groups", ); - assert.strictEqual(container.id, "kafka-cluster-consumer-groups"); + assert.strictEqual(container.id, `${CCLOUD_CONNECTION_ID}-Consumer Groups`); }); - it("should use the same id regardless of connection type", () => { + it("should use different ids for different connection types", () => { const ccloudContainer = new KafkaClusterResourceContainer( CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, @@ -124,7 +124,7 @@ describe("models/containers/kafkaClusterResourceContainer", () => { TEST_LABEL, ); - assert.strictEqual(ccloudContainer.id, directContainer.id); + assert.notStrictEqual(ccloudContainer.id, directContainer.id); }); }); @@ -137,7 +137,7 @@ describe("models/containers/kafkaClusterResourceContainer", () => { label, ); - assert.strictEqual(container.loggerName, `models.KafkaClusterResourceContainer(${label})`); + assert.strictEqual(container.loggerName, `KafkaClusterResourceContainer.${label}`); }); }); }); diff --git a/src/models/containers/kafkaClusterResourceContainer.ts b/src/models/containers/kafkaClusterResourceContainer.ts index 334d60511..406f63018 100644 --- a/src/models/containers/kafkaClusterResourceContainer.ts +++ b/src/models/containers/kafkaClusterResourceContainer.ts @@ -1,6 +1,4 @@ -import type { ThemeIcon } from "vscode"; -import type { ConnectionType } from "../../clients/sidecar"; -import type { ConnectionId, ISearchable } from "../resource"; +import type { ISearchable } from "../resource"; import { ResourceContainer } from "./resourceContainer"; /** Labels for the top-level containers in the Topics view. */ @@ -11,23 +9,5 @@ export enum KafkaClusterContainerLabel { /** A container {@link TreeItem} for resources to display in the Topics view. */ export class KafkaClusterResourceContainer extends ResourceContainer { - get loggerName() { - return `models.KafkaClusterResourceContainer(${this.label})`; - } - - constructor( - connectionId: ConnectionId, - connectionType: ConnectionType, - label: string, - children: T[] = [], - contextValue?: string, - icon?: ThemeIcon, - ) { - super(connectionId, connectionType, label, children, contextValue, icon); - - // convert label to hyphenated id: - // "Consumer Groups" -> "consumer-groups", "Topics" -> "topics" - const suffix = label.toLowerCase().replaceAll(/\s+/g, "-"); - this.id = `kafka-cluster-${suffix}`; - } + protected readonly loggerNamePrefix = "KafkaClusterResourceContainer"; } diff --git a/src/models/containers/resourceContainer.test.ts b/src/models/containers/resourceContainer.test.ts index cc0a59b7c..48de7c14b 100644 --- a/src/models/containers/resourceContainer.test.ts +++ b/src/models/containers/resourceContainer.test.ts @@ -10,7 +10,7 @@ import { LOADING_POLL_INTERVAL_MS, ResourceContainer } from "./resourceContainer /** Minimal concrete subclass to test abstract base. */ class TestContainer extends ResourceContainer { - readonly loggerName = "test.TestContainer"; + protected readonly loggerNamePrefix = "TestContainer"; constructor( label: string, @@ -19,7 +19,6 @@ class TestContainer extends ResourceContainer { icon?: ThemeIcon, ) { super(CCLOUD_CONNECTION_ID, ConnectionType.Ccloud, label, children, contextValue, icon); - this.id = `test-${label}`; } } @@ -48,7 +47,7 @@ describe("models/containers/resourceContainer.ts", () => { const container = new TestContainer(testLabel, testResources); assert.strictEqual(container.label, testLabel); - assert.strictEqual(container.id, `test-${testLabel}`); + assert.strictEqual(container.id, `${CCLOUD_CONNECTION_ID}-${testLabel}`); }); it("should always set the collapsible state to Collapsed", () => { diff --git a/src/models/containers/resourceContainer.ts b/src/models/containers/resourceContainer.ts index 9da019d7a..adfd8aa52 100644 --- a/src/models/containers/resourceContainer.ts +++ b/src/models/containers/resourceContainer.ts @@ -15,14 +15,14 @@ export abstract class ResourceContainer extends TreeItem implements ISearchable { - // enforce string so subclasses set this after super() + // narrow TreeItem.id from `string | undefined` to satisfy IdItem (required by BaseViewProviderData) declare id: string; // IResourceBase fields required by BaseViewProviderData readonly connectionId: ConnectionId; readonly connectionType: ConnectionType; - abstract loggerName: string; + protected abstract readonly loggerNamePrefix: string; private _children: T[]; @@ -31,16 +31,17 @@ export abstract class ResourceContainer protected readonly _defaultContextValue: string | undefined; protected readonly _defaultIcon: ThemeIcon | undefined; - protected constructor( + constructor( connectionId: ConnectionId, connectionType: ConnectionType, label: string, - children: T[], + children: T[] = [], contextValue?: string, icon?: ThemeIcon, ) { super(label, TreeItemCollapsibleState.Collapsed); + this.id = `${connectionId}-${label}`; this.connectionId = connectionId; this.connectionType = connectionType; this._children = children; @@ -53,7 +54,12 @@ export abstract class ResourceContainer this.iconPath = this._defaultIcon; } - // lazy because loggerName is abstract and not available during super() / constructor time + /** Logger name combining the subclass-provided {@link loggerNamePrefix} and instance label. */ + get loggerName(): string { + return `${this.loggerNamePrefix}.${this.label}`; + } + + // lazy to avoid allocating a Logger on every container construction private _logger?: Logger; private get logger(): Logger { if (!this._logger) {