cleanup: first batch of fixes for `no-floating-promises` eslint rule #2787

shouples · 2025-10-03T18:55:42Z

Summary of Changes

No functional changes here, just getting one step closer toward closing #1498 and setting the rule to error.

On main, this rule shows ~180 violations, but this branch takes care of the majority of them, to the point where we can set the rule to warn without reaching the warn-limit (50) and causing CI to fail static analysis.

Takeaways:

most offenders were calls to logError(), which was previously an async function, but is now a synchronous function wrapping the async logic with a basic .catch() so all callers don't need to explicitly await or void
the next largest group of offenders were anything involving show*NotificationWithButtons(), which now also use void since the vast majority of the time we don't need to block on the underlying window.show[Information|Warning|Error]Message() or its (potentially-async) button callbacks
after that, awaits were needed in tests either for assert.rejects or in the test logic where an async function / class method was being called
remaining specific areas that needed fixing have specific PR comments for explanations/context

Pull request checklist

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

Tests

Added new
Updated existing
Deleted existing

Release notes

Does anything in this PR need to be mentioned in the user-facing CHANGELOG?

src/extensionSettings/listener.ts

shouples · 2025-10-03T19:21:52Z

src/flinkSql/flinkLanguageClientManager.ts

      // Don't send with undefined settings, server will override existing settings with empty/undefined values
      if (settings.databaseName && settings.computePoolId && settings.catalogName) {
-        this.languageClient.sendNotification("workspace/didChangeConfiguration", {
+        await this.languageClient.sendNotification("workspace/didChangeConfiguration", {


@noeldevelops I'm assuming we want to await this, right?

yes please!

shouples · 2025-10-03T19:22:57Z

src/flinkSql/languageClient.ts

  await languageClient.start();
  logger.debug("FlinkSQL Language Server started");
-  languageClient.setTrace(Trace.Compact);
+  await languageClient.setTrace(Trace.Compact);


(cc @noeldevelops this also seemed fine to await instead of void)

it doesn't return anything so void is ok here I think? It is a Promise though

shouples · 2025-10-03T19:24:37Z

src/sidecar/connections/ccloud.test.ts

+  for (const value of [false, undefined]) {
+    it(`hasCCloudAuthSession() should return false when the context value is ${value}`, async () => {
+      await setContextValue(ContextValues.ccloudConnectionAvailable, value);


Split to an outer-for loop so the dynamically-created tests could be made async instead of dealing with "await-in-for-loop"

shouples · 2025-10-03T19:25:19Z

src/sidecar/connections/watcher.test.ts


    // As if had been just sent from sidecar.
-    connectionStateWatcher.handleConnectionUpdateEvent(websocketMessage);
+    await connectionStateWatcher.handleConnectionUpdateEvent(websocketMessage);


This was the main offender in the file, but then required the test-callers to await its containing function

shouples · 2025-10-03T19:30:15Z

src/sidecar/sidecarManager.ts


  /** Called whenever websocket connection goes CONNECTED or DISCONNECTED. */
-  private onWebsocketStateChange(event: WebsocketStateEvent) {
+  private async onWebsocketStateChange(event: WebsocketStateEvent) {


Only caller is a few lines up, in the event listener callback binding:

vscode/src/sidecar/sidecarManager.ts

Line 276 in 7faf124

this.websocketManager.registerStateChangeHandler(this.onWebsocketStateChange.bind(this));

shouples · 2025-10-03T19:32:58Z

src/scheduler.test.ts

  it("should await for release", async () => {
    const { acquire, release } = semaphore(1);
-    acquire();
+    await acquire();


This one was a little ironic

shouples · 2025-10-03T19:33:38Z

src/extension.ts

+    void logError(new Error(msg, { cause: e }), msg, { extra: {} });
  }
-  closeSentryClient();
+  void closeSentryClient();


Not trying to await or catch errors during disposal, cc @noeldevelops

shouples · 2025-10-03T19:40:43Z

src/extension.ts

-  topicViewProvider.reset();
-  schemasViewProvider.reset();


@jlrobins do you think we should expand this behavior to the rest of the views, or remove this entirely?

I don't know if this is even necessary anymore since we still wipe workspace state on activation, but the only area this might cause issues is during extension version updates where the Topics and Schemas views may reset in the middle of use (from one extension version to another). I don't think we've tested that path for the Flink-related views though.

Hmm, good question, but at least not needing answering here in for this branch.

I think that it'd be safer to include to do for all views (but not needed in this branch).

shouples · 2025-10-03T19:41:13Z

src/extension.ts

  for (const instance of getRefreshableViewProviders()) {
    refreshCommands.push(
-      registerCommandWithLogging(`confluent.${instance.kind}.refresh`, (): boolean => {
-        instance.refresh(true);
-        return true;
-      }),
+      registerCommandWithLogging(
+        `confluent.${instance.kind}.refresh`,
+        async (): Promise<boolean> => {
+          await instance.refresh(true);
+          return true;
+        },
+      ),


View-level refreshing still working as expected ✅

shouples · 2025-10-03T19:48:00Z

src/webview/flink-statement-results.ts

+      await this.submitSearch(value);
    } else {
      // otherwise, we keep debouncing search submittion until the user stops typing
      if (this.searchTimer != null) clearTimeout(this.searchTimer);
      this.searchTimer = setTimeout(async () => {
        const value = target.value.trim();
-        this.submitSearch(value);
+        await this.submitSearch(value);


Flink statement result search still seems to behave as expected, but I'm curious if there are any nuances here with timing/performance we need to be cautious handling.

shouples · 2025-10-03T19:50:37Z

src/schemas/validateDocument.ts

        docChangeSub.dispose();
        docSaveSub.dispose();
-        validateDocument(documentUri, schema);
+        await validateDocument(documentUri, schema);


Produce-message validation still happening as expected on document content change ✅

shouples · 2025-10-03T19:51:55Z

src/schemas/validateDocument.ts

      docChangeSub.dispose();
      docSaveSub.dispose();
-      validateDocument(documentUri, schema);
+      await validateDocument(documentUri, schema);


This one is harder to test since validation errors will persist before/after a document is saved, but nothing out of the ordinary is happening after this change

Copilot

Pull Request Overview

This PR addresses the no-floating-promises ESLint rule violations by ensuring promises are properly handled throughout the codebase. The changes focus on either awaiting promises where appropriate or using void for fire-and-forget operations.

Key Changes:

Added await keywords for promises that need to be awaited, particularly in tests and critical code paths
Used void prefix for fire-and-forget operations like logging and notifications
Updated test methods to be async where promise handling is required

Reviewed Changes

Copilot reviewed 67 out of 67 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
eslint.config.mjs	Enabled `no-floating-promises` rule as warning with TypeScript parser configuration
src/extension.ts	Fixed promise handling in extension activation and deactivation
src/notifications.ts	Added void prefix for error logging in notification callbacks
src/viewProviders/topics.test.ts	Made test methods async and added await for async handlers
src/viewProviders/schemas.ts	Added void prefix for error logging
src/sidecar/ files	Consistent void prefixing for logError calls
src/commands/ files	Added void prefixes for notification and logging calls
src/flinkSql/ files	Fixed promise handling in language client and statement management
src/featureFlags/ files	Added void prefixes for error logging
src/utils/ files	Fixed test assertions and worker pool error handling
src/webview/ files	Added await for async operations in form handling
Various test files	Made test methods async and added await for assert.rejects

src/sidecar/connections/ccloud.test.ts

src/commands/schemas.ts

…rrors (ha) when calling existing async logic via _logError

src/errors.ts

sonarqube-confluent · 2025-10-03T20:50:50Z

67.90% Coverage on New Code (is less than 80.00%)

Analysis Details

2 Issues

0 Bugs
0 Vulnerabilities
2 Code Smells

Coverage and Duplications

67.90% Coverage (72.10% Estimated after merge)
No duplication information (0.60% Estimated after merge)

Project ID: vscode

View in SonarQube

jlrobins

Things still work! Excellent, thanks!

src/extensionSettings/listener.ts

jlrobins · 2025-10-03T21:10:00Z

src/extension.ts

-  topicViewProvider.reset();
-  schemasViewProvider.reset();


Hmm, good question, but at least not needing answering here in for this branch.

I think that it'd be safer to include to do for all views (but not needed in this branch).

sentry · 2025-10-10T09:57:37Z

Issues attributed to commits in this pull request

This pull request was merged and Sentry observed the following issues:

‼️ CodeExpectedError: confluent.schemas.uploadForSubj... in production

shouples added 30 commits October 3, 2025 13:20

add awaits for authn/ccloudProvider.ts

7248fb0

add void operator for notification

95b84f1

void logError

88ff4e3

void logError calls

c7c9b1a

void notification calls, await view refreshes

a5ea615

void notifications

afb40d7

void logError and notification

284c318

void logError

b4f16ac

void logError and notifications

664c870

void notifications and logError

a688414

fix promise map; void notifications and logError calls

f0cf193

void logError and notifications

c56f456

void logError calls

ec3db79

void logError and notification

56e0b57

void logError

1104d6a

void notification

e087ac9

void notifications

bc6bbbe

await assert.rejects

a542834

void notifications and Sentry client closing

c482a6f

await view resets

adab236

await view refresh

bec22bb

await LD client identify call

1ab8cd4

await setting context value

b1dcaf3

void logError and notification

343d808

await getting LD client

2677c07

void logError and closing client

9349731

void notification

6f82447

void logError

a566c59

void logError

9d1ac00

await onDidChangeActiveTextEditorHandler

e5aea2e

shouples commented Oct 3, 2025

View reviewed changes

src/extensionSettings/listener.ts Show resolved Hide resolved

shouples commented Oct 3, 2025

View reviewed changes

shouples marked this pull request as ready for review October 3, 2025 19:52

shouples requested a review from a team as a code owner October 3, 2025 19:52

Copilot AI review requested due to automatic review settings October 3, 2025 19:52

Copilot AI reviewed Oct 3, 2025

View reviewed changes

src/sidecar/connections/ccloud.test.ts Show resolved Hide resolved

src/commands/schemas.ts Show resolved Hide resolved

shouples added 2 commits October 3, 2025 16:31

logError switcheroo: make exported function synchronous and log any e…

0a520d1

…rrors (ha) when calling existing async logic via _logError

revert all the void logErroring

fb4fc5c

shouples commented Oct 3, 2025

View reviewed changes

src/errors.ts Outdated Show resolved Hide resolved

no need to export inner async _logError that nobody should call

05d8770

jlrobins self-assigned this Oct 3, 2025

jlrobins approved these changes Oct 3, 2025

View reviewed changes

shouples merged commit e016882 into main Oct 3, 2025
13 of 14 checks passed

shouples deleted the djs/no-floating-promises-fix1 branch October 3, 2025 21:14

This was referenced Oct 6, 2025

cleanup: first batch of fixes for no-explicit-any eslint rule #2799

Draft

Indicate Docker Engine availability / existence in the Local Connection row #2803

Merged

jlrobins mentioned this pull request Oct 16, 2025

Fix kafka topic "flinkability" when possible compute pools are in separate ccloud environments. #2879

Merged

4 tasks

cleanup: first batch of fixes for no-floating-promises eslint rule #2787

cleanup: first batch of fixes for no-floating-promises eslint rule #2787

Uh oh!

Conversation

shouples commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Pull request checklist

Tests

Release notes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shouples Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shouples Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes:

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqube-confluent bot commented Oct 3, 2025

Analysis Details

2 Issues

Coverage and Duplications

Uh oh!

jlrobins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sentry bot commented Oct 10, 2025

Issues attributed to commits in this pull request

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cleanup: first batch of fixes for `no-floating-promises` eslint rule #2787

cleanup: first batch of fixes for `no-floating-promises` eslint rule #2787

shouples commented Oct 3, 2025 •

edited

Loading

shouples Oct 3, 2025 •

edited

Loading

shouples Oct 3, 2025 •

edited

Loading