Skip to content

Conversation

@lovincyrus
Copy link
Contributor

@lovincyrus lovincyrus commented Nov 18, 2025

This PR adds a Data Explorer UI for Data Warehouse and Databases. Closes https://linear.app/rilldata/issue/APP-516/data-explorer-ui-for-data-warehouse-and-databases

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

@lovincyrus
Copy link
Contributor Author

Once we land #8389, I'll revert this commit: d3a10e0

Copy link
Contributor

royendo commented Nov 24, 2025

lmk when its ready for another round of reviews

@lovincyrus lovincyrus requested a review from royendo November 25, 2025 10:54
Copy link
Contributor

@royendo royendo left a comment

Choose a reason for hiding this comment

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

looks great!

A few nit items

  • missing the comment section in the top of the source YAML generated on import
# Model YAML
# Reference documentation: https://docs.rilldata.com/reference/project-files/models

    • New XXX Connector in left panel (does the same as Back)

@lovincyrus
Copy link
Contributor Author

  • New XXX Connector in left panel (does the same as Back)

Can you rephrase this? Thank you!

@royendo
Copy link
Contributor

royendo commented Nov 25, 2025

  • New XXX Connector in left panel (does the same as Back)

Can you rephrase this? Thank you!

As the design has in the left panel, theres a ui button to create new connector

@ericpgreen2
Copy link
Contributor

Component Complexity: AddDataForm.svelte needs refactoring before merge

The new explorer state tracking (lines 142-152) highlights a concerning architectural issue with AddDataForm.svelte that should be addressed before merging.

The Problem

This PR adds 5+ new state variables and a 30-line model creation function to an already complex component:

// Lines 142-152: New state for explorer step
let selectedConnectorForModel = "";
let selectedDatabaseForModel = "";
let selectedSchemaForModel = "";
let selectedTableForModel = "";
let creatingModel = false;
$: modelingSupportQuery = useIsModelingSupportedForConnector(...)
$: isModelingSupportedForSelected = $modelingSupportQuery.data || false;

// Lines 325-352: New handleCreateModel function (~30 lines)

Before this PR: 513 lines
After this PR: 592 lines (+15%)

Why This Matters

AddDataForm.svelte is now handling too many concerns:

  1. Form rendering and validation for multiple connector types
  2. Multi-step navigation (connector → source → explorer)
  3. Error management (DSN, params, connector-specific)
  4. File upload handling
  5. YAML preview computation
  6. "Save anyway" flow
  7. NEW: Table selection tracking (4 variables)
  8. NEW: Model creation logic with connector support detection

The explorer step has fundamentally different concerns than form submission:

  • Form steps: Validate inputs → Submit → Handle errors
  • Explorer step: Browse data → Select table → Create resource

These shouldn't share the same state container.

Required Changes

Option 1: Extract to Separate Component(Recommended)

Create AddDataExplorerStep.svelte to encapsulate all explorer-specific logic:

<!-- AddDataExplorerStep.svelte -->
<script lang="ts">
  export let connector: V1ConnectorDriver;
  export let onModelCreated: (path: string) => void;
  export let onBack: () => void;

  // All explorer state lives here
  let selectedConnector = "";
  let selectedDatabase = "";
  let selectedSchema = "";
  let selectedTable = "";
  let creatingModel = false;

  $: modelingSupportQuery = useIsModelingSupportedForConnector(
    instanceId,
    selectedConnector
  );

  async function handleCreateModel() {
    // Model creation logic (moved from AddDataForm)
    const [path] = await createModelFromTable(...);
    onModelCreated(path);
  }
</script>

<DataExplorerDialog
  connectorDriver={connector}
  onSelect={(detail) => {
    selectedConnector = detail.connector;
    selectedDatabase = detail.database;
    selectedSchema = detail.schema;
    selectedTable = detail.table;
  }}
/>

<Button
  disabled={!selectedTable || creatingModel}
  loading={creatingModel}
  onClick={handleCreateModel}
>
  Create model
</Button>

Then simplify AddDataForm.svelte:

{#if stepState.step === "explorer"}
  <AddDataExplorerStep 
    {connector} 
    onModelCreated={async (path) => {
      await goto(`/files${path}`);
      onClose();
    }}
    onBack={formManager.handleBack}
  />
{:else}
  <!-- Existing form steps - connector and source -->
{/if}

Benefits:

  • Reduces AddDataForm.svelte from 592 → ~520 lines
  • Clear separation of concerns (form logic vs. data browsing)
  • Explorer can be tested independently
  • State is colocated with the step that uses it
  • Easier to maintain and extend

Option 2: Extract to Utility Function (Minimum acceptable)

If you prefer not to create a new component, at minimum extract handleCreateModel to a separate utility:

// web-common/src/features/sources/modal/model-creation-utils.ts
export async function createModelFromExplorerSelection(
  queryClient: QueryClient,
  options: {
    connector: string;
    database: string;
    schema: string;
    table: string;
    isModelingSupported: boolean;
  }
): Promise<[string, string]> {
  return options.isModelingSupported
    ? await createSqlModelFromTable(
        queryClient,
        options.connector,
        options.database,
        options.schema,
        options.table
      )
    : await createYamlModelFromTable(
        queryClient,
        options.connector,
        options.database,
        options.schema,
        options.table
      );
}

This reduces duplication and makes the logic testable, but doesn't address the state management concern.

Recommendation

Please implement Option 1 (extract to separate component) before merging. This will:

  • Keep the codebase maintainable as features grow
  • Make testing easier
  • Follow single responsibility principle
  • Set a good precedent for future multi-step flows

The refactoring shouldn't take long since the logic is already written—it just needs to be moved into the right component boundary.


Developed in collaboration with Claude Code

Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

Can you please add e2e tests to cover this new functionality?

Copy link

@Di7design Di7design left a comment

Choose a reason for hiding this comment

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

  1. Table items listed in the data explorer modal are too small( use design component, do not reuse left nav table list as it's served for limited space).
  2. missing add connector action on the left side of the modal
  3. missing source name and search functions.
    Functionally it's a good start, please implement based on design for follow up implementation.

@royendo
Copy link
Contributor

royendo commented Dec 10, 2025

Additionally, like we implemented in CloudStoarge, can you add the Already connected? Dialogue in the right panel?
Screenshot 2025-12-10 at 10 03 58

@lovincyrus
Copy link
Contributor Author

Additionally, like we implemented in CloudStoarge, can you add the Already connected? Dialogue in the right panel? Screenshot 2025-12-10 at 10 03 58

Since this is a change from #8467, we should wait for #8467 to land before duplicating the same lines of code.

@lovincyrus
Copy link
Contributor Author

Closing this as we focus on the schema-driven refactor.

@lovincyrus lovincyrus closed this Dec 22, 2025
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.

5 participants