Skip to content

Conversation

@derek1ee
Copy link
Contributor

@derek1ee derek1ee commented Aug 25, 2025

Summary of Changes

  • Allow user to submit a Flink statement, without setting catalog/database via CodeLens.
  • Currently, user must set compute pool, catalog & database before "Submit statement" action is enabled in CodeLens.
  • However, the selection of catalog & database is not required for successful SQL statement submission, as long as the table name(s) is fully qualified.
  • This change also allow users without any catalog/database to submit statement against the example catalog / marketplace database.
  • Fixes Codelens UX tweaks: persist "Submit Statement" codelens and update doc metadata #1711

Any additional details or context that should be provided?

Submit action enabled as soon as a pool is selected:
Screenshot 2025-08-25 at 3 03 35 PM

Option to skip catalog/database selection after selecting submit statement:
Screenshot 2025-08-25 at 3 03 52 PM

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

Copilot AI review requested due to automatic review settings August 25, 2025 22:04
@derek1ee derek1ee requested a review from a team as a code owner August 25, 2025 22:04
@derek1ee derek1ee changed the title Allow Allow submitting statement w/o catalog/database Aug 25, 2025
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 modifies the Flink SQL statement submission workflow to allow users to submit statements without selecting a catalog and database, provided they use fully qualified table names. Previously, users were required to select both a compute pool and database before the "Submit Statement" action became available.

Key changes:

  • Enable "Submit Statement" CodeLens action as soon as a compute pool is selected
  • Add optional "Skip" functionality to database selection quickpick
  • Handle optional database/catalog parameters in statement submission

Reviewed Changes

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

Show a summary per file
File Description
src/quickpicks/kafkaClusters.ts Adds allowSkip option to kafka cluster quickpick with skip functionality
src/quickpicks/kafkaClusters.test.ts Updates tests to verify skip option appears in quickpick items
src/commands/flinkStatements.ts Makes database selection optional in statement submission flow
src/codelens/flinkSqlProvider.ts Enables submit button when only compute pool is selected
src/codelens/flinkSqlProvider.test.ts Adds test coverage for submit action with pool-only configuration

label: "Skip",
description: "Continue without selecting a database",
iconPath: new ThemeIcon("testing-skipped-icon"),
value: null as any,
Copy link

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

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

Using null as any bypasses TypeScript's type safety. Consider defining the type properly or using undefined consistently with the return type KafkaCluster | undefined."

Suggested change
value: null as any,
value: undefined,

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

@derek1ee I disagree with Copilot here and would rather we use null explicitly so the extension can treat it as "user intentionally chose to skip" rather than the current cancellation treatment.

* @param placeholder Optionally override the placeholder text for the quickpick.
* Defaults to "Select the Kafka cluster to use as the default database for the statement".
* @returns chosen Kafka cluster or undefined if the user cancels the quickpick.
* @returns chosen Kafka cluster or undefined if the user cancels the quickpick or selects "Skip".
Copy link

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

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

The JSDoc comment update is inconsistent with the actual return behavior. When "Skip" is selected, the function returns null (due to value: null as any), but the documentation says it returns undefined."

Copilot uses AI. Check for mistakes.
@sonarqube-confluent
Copy link

Passed

Analysis Details

1 Issue

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

Coverage and Duplications

  • Coverage 88.90% Coverage (70.70% Estimated after merge)
  • Duplications No duplication information (0.90% Estimated after merge)

Project ID: vscode

View in SonarQube

@shouples shouples self-assigned this Aug 26, 2025
Copy link
Contributor

@shouples shouples left a comment

Choose a reason for hiding this comment

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

Thanks @derek1ee! I like the idea of a dedicated skip option, but there are a few things that need to be updated for consistency and to make sure we're still following TypeScript best practices. I added a few suggestions below, and in addition I suggest:

  • we update the return type of kafkaClusterQuickPick() from Promise<KafkaCluster | undefined> to Promise<KafkaCluster | null | undefined>:
    export async function kafkaClusterQuickPick(
    options: KafkaClusterQuickPickOptions = {},
    ): Promise<KafkaCluster | undefined> {
  • the command function for setting a document's database metadata should check if (database !== undefined) instead of the more general if (!database) (to allow null):
    const database: KafkaCluster | undefined = await flinkDatabaseQuickpick(computePool);
    if (!database) {
    return;
    }

): Promise<KafkaCluster | undefined> {
return await kafkaClusterQuickPick({
placeHolder: placeholder,
allowSkip: true,
Copy link
Contributor

@shouples shouples Aug 26, 2025

Choose a reason for hiding this comment

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

Let's also add allowSkip as a parameter to the flinkDatabaseQuickpick() function so it can be passed through from any callers. Currently, any callers of flinkDatabaseQuickpick() (like setting Flink defaults for the language server integration) will show the option to skip, and we only want this for the codelens implementation and setting document metadata, right?

* @param placeholder Optionally override the placeholder text for the quickpick.
* Defaults to "Select the Kafka cluster to use as the default database for the statement".
* @returns chosen Kafka cluster or undefined if the user cancels the quickpick.
* @returns chosen Kafka cluster or undefined if the user cancels the quickpick or selects "Skip".
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @returns chosen Kafka cluster or undefined if the user cancels the quickpick or selects "Skip".
* @returns chosen Kafka cluster, `null` if the user choose "Skip" (for Flink SQL statements), or undefined if the user cancels

export async function flinkDatabaseQuickpick(
computePool: CCloudFlinkComputePool,
placeholder: string = "Select the Kafka cluster to use as the default database for the statement",
): Promise<KafkaCluster | undefined> {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should update the return type of kafkaClusterQuickPick() as well as here:

Suggested change
): Promise<KafkaCluster | undefined> {
): Promise<KafkaCluster | null | undefined> {

label: "Skip",
description: "Continue without selecting a database",
iconPath: new ThemeIcon("testing-skipped-icon"),
value: null as any,
Copy link
Contributor

Choose a reason for hiding this comment

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

@derek1ee I disagree with Copilot here and would rather we use null explicitly so the extension can treat it as "user intentionally chose to skip" rather than the current cancellation treatment.

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.

Codelens UX tweaks: persist "Submit Statement" codelens and update doc metadata

3 participants