improve(loaders): add getClusterForTopic() helper - #3444
Draft
Dave Shoup (shouples) wants to merge 1 commit into
Draft
improve(loaders): add getClusterForTopic() helper#3444Dave Shoup (shouples) wants to merge 1 commit into
getClusterForTopic() helper#3444Dave Shoup (shouples) wants to merge 1 commit into
Conversation
The old ResourceManager lookup was connection-unaware and failed for direct-connection topics. Route the scaffold and delete-topic commands through the new method, and log to Sentry when a topic's cluster can't be resolved during scaffolding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR consolidates “topic → Kafka cluster” resolution behind a new ResourceLoader.getClusterForTopic() helper, and updates the project-scaffolding and topic-deletion commands to use it. This aligns cluster lookup behavior across Confluent Cloud, local, and direct connections, and adds Sentry logging for an unexpected “topic can’t resolve to cluster” condition during scaffolding.
Changes:
- Added
ResourceLoader.getClusterForTopic(topic)helper with a connection-id guard and environment-scoped cluster lookup. - Refactored project scaffolding and topic deletion to call the helper instead of inlining
getKafkaClustersForEnvironmentId(...).find(...). - Updated unit tests to cover the helper and scaffolding’s new Sentry logging path.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/loaders/resourceLoader.ts | Adds getClusterForTopic() helper on the loader base class. |
| src/loaders/resourceLoader.test.ts | Adds unit tests for getClusterForTopic(). |
| src/commands/scaffold.ts | Uses getClusterForTopic() and logs to Sentry when a topic can’t resolve to a cluster. |
| src/commands/scaffold.test.ts | Updates scaffolding tests to stub getClusterForTopic() and assert Sentry logging. |
| src/commands/kafkaClusters.ts | Uses getClusterForTopic() when firing topicChanged after deletion. |
| src/commands/kafkaClusters.test.ts | Updates the “topic deleted” test to stub getClusterForTopic(). |
Suppressed comments (2)
src/loaders/resourceLoader.test.ts:500
- Test name doesn’t follow the project’s convention of using a "should …" prefix (CodingGuidelineID 1000004).
it("Returns the cluster matching the topic's clusterId", async () => {
src/loaders/resourceLoader.test.ts:510
- Test name doesn’t follow the project’s convention of using a "should …" prefix (CodingGuidelineID 1000004).
it("Returns undefined when no cluster in the environment matches the topic's clusterId", async () => {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| sandbox.restore(); | ||
| }); | ||
|
|
||
| it("Raises error for a topic from a mismatched connection", async () => { |
Comment on lines
453
to
+455
| it("should fire the topicChanged event with change='deleted' after successful deletion", async () => { | ||
| showInputBoxStub.resolves(TEST_CCLOUD_KAFKA_TOPIC.name); | ||
| stubbedLoader.getKafkaClustersForEnvironmentId.resolves([TEST_CCLOUD_KAFKA_CLUSTER]); | ||
| stubbedLoader.getClusterForTopic.resolves(TEST_CCLOUD_KAFKA_CLUSTER); |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of Changes
Consolidates how the extension looks up the Kafka cluster a topic belongs to. Two commands previously hand-rolled the same
getKafkaClustersForEnvironmentId(...).find(...)lookup; both now call a newResourceLoader.getClusterForTopic(topic)helper instead, which returns the cluster (orundefinedif none matches) and works for Confluent Cloud, local, and direct-connection topics alike.getClusterForTopic()on the abstractResourceLoaderbase, mirroring the existing siblinggetTopicSubjectGroups()(same connection-id guard, same lookup bytopic.environmentId).resourceScaffoldProjectCommand(project scaffolding) anddeleteTopicnow route through the helper instead of inlining the lookup.logError()when a topic can't be resolved to a cluster, so we learn if that unexpected condition ever happens in the wild.Closes #1694.
Click-testing instructions
This is internal plumbing with no user-visible change on the happy path. To confirm nothing regressed:
topicChangedevent still fires for the parent cluster).Optional: Any additional details or context that should be provided?
ResourceManager.getClusterForTopic()referenced in Implement ResourceLoader.getClusterForTopic(topic: KafkaTopic): KafkaCluster | undefined #1694 was already removed in a prior change; this adds the connection-aware replacement on the loader and wires the remaining callers to it.connectionIdmismatch guard verbatim; extracting a sharedassertSameConnection()helper is out of scope here and left as a follow-up.Pull request checklist
Please check if your PR fulfills the following (if applicable):
Tests
Release notes