Skip to content

Conversation

@jlrobins
Copy link
Contributor

@jlrobins jlrobins commented Jul 30, 2025

Summary of Changes

  • Make ParentedBaseViewProvider.parentResourceChangedEmitter required to be assigned in any concrete subclass. Simplifies its setEventListeners() implementation. The whole point of ParentedBaseViewProvider is that it has a "parent" resource, and our design pattern is that changing that parent resource is indirectly done through an event emitter. Both of the existing subclasses (FlinkStatementsViewProvider, FlinkArtifactsViewProvider) and upcoming FlinkUDFViewProvider will conform, so make things a little simpler. If a new subclass were created that violated this rule, setEventListeners() will fail at extension startup with trying to reference .event() off of undefined.
  • Only wire up explicit named methods as event handlers within BaseViewProvider and ParentedBaseViewProvider as is our new standard. In BaseViewProvider's case, the wiring is made a little more complicated due to needing to do so conditionally based on if the concrete subclass has the search change event assigned.
  • Test overall setEventListers() behavior across BaseViewProvider and ParentedBaseViewProvider from perspective of FlinkStatementsViewProvider, which passes the has-search-term-changed-event-emitter condition condition in BaseViewController.
  • Test overall setEventListers() behavior across BaseViewProvider and ParentedBaseViewProvider from perspectives of NewResourceViewController, which goes into the else block of the has-search-term-changed-event-emitter condition condition in BaseViewController (at this time. Search will be implemented Soon).

Any additional details or context that should be provided?

Pull request checklist

Please check if your PR fulfills the following (if applicable):

Tests
  • Added new
  • Updated existing
  • Deleted existing
Other
  • All new disposables (event listeners, views, channels, etc.) collected as for eventual cleanup?
  • Does anything in this PR need to be mentioned in the user-facing CHANGELOG or README?
  • Have you validated this change locally by packaging and installing the extension .vsix file?
    gulp clicktest

…entedBaseViewProvider from perspective of FlinkStatementsViewProvider.
@jlrobins jlrobins changed the title Bring BaseViewProvider and ParentedBaseViewProvider event handling wiring up to standards Bring BaseViewProvider and ParentedBaseViewProvider event handling wiring and testing up to standards Jul 30, 2025
@sonarqube-confluent

This comment has been minimized.

@sonarqube-confluent
Copy link

Passed

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 100.00% Coverage (69.10% Estimated after merge)
  • Duplications No duplication information (0.80% Estimated after merge)

Project ID: vscode

View in SonarQube

@jlrobins jlrobins marked this pull request as ready for review July 30, 2025 19:22
Copilot AI review requested due to automatic review settings July 30, 2025 19:22
@jlrobins jlrobins requested a review from a team as a code owner July 30, 2025 19:22
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR standardizes event handling wiring and testing for BaseViewProvider and ParentedBaseViewProvider classes to follow established patterns. The main purpose is to ensure that only explicit named methods are used as event handlers, improving code maintainability and testability.

Key changes:

  • Made ParentedBaseViewProvider.parentResourceChangedEmitter required instead of optional to simplify event wiring logic
  • Refactored event handler registration to use explicit method binding with .bind(this) instead of inline arrow functions
  • Updated test suites to verify complete event listener behavior across the inheritance hierarchy

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/viewProviders/base.ts Made parentResourceChangedEmitter required, refactored event handlers to use explicit method binding, renamed handleCCloudConnectionChange to ccloudConnectedHandler
src/viewProviders/base.test.ts Updated tests to use new handler method name and removed outdated search emitter tests
src/viewProviders/flinkStatements.test.ts Updated test descriptions and coverage to include all event handlers from inheritance hierarchy
src/viewProviders/newResources.test.ts Updated test descriptions to clarify event listener testing scope
Comments suppressed due to low confidence (1)

src/viewProviders/base.test.ts:351

  • The test description still references the old method name handleCCloudConnectionChange() but the method was renamed to ccloudConnectedHandler. Update the test description to match the new method name.
    it("handleCCloudConnectionChange() should call reset() when the `ccloudConnected` event fires and a CCloud resource is focused", () => {

Copy link
Member

@noeldevelops noeldevelops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for the clarifying code comments!

@jlrobins jlrobins merged commit 0ed2509 into main Jul 31, 2025
14 checks passed
@jlrobins jlrobins deleted the jlr/refactor-ParentedBaseViewProvider-event-handling branch July 31, 2025 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants