data sources: surface env detections, hide them once connected - #10575
Open
Light2Dark wants to merge 9 commits into
Open
data sources: surface env detections, hide them once connected#10575Light2Dark wants to merge 9 commits into
Light2Dark wants to merge 9 commits into
Conversation
Annotate discovered integrations with live connection state so quick-add prompts only appear when they are useful.
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Light2Dark
changed the title
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 41 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/hooks/useDataSourceDiscovery.ts">
<violation number="1" location="frontend/src/hooks/useDataSourceDiscovery.ts:47">
P2: The new filtering behavior is untested at the hook boundary, so configured suggestions or database/storage grouping could regress without failing tests. Add focused hook tests covering configured sources being excluded and unconfigured catalog, database, and object-storage sources being grouped correctly.
(Based on your team's feedback about regression tests for behavior fixes.) .</violation>
</file>
<file name="frontend/src/components/editor/connections/__tests__/quick-add-data-sources.test.tsx">
<violation number="1" location="frontend/src/components/editor/connections/__tests__/quick-add-data-sources.test.tsx:31">
P3: The new `configured` fixtures fields are inert: `QuickAddDataSources` (the component under test) never reads `configured`, so these changes don't exercise the PR's actual behavior (hiding configured sources via `useUnconfiguredDataSources`, plus the new badges). Add a regression test that covers the filtering/badge behavior instead of only satisfying the type.</violation>
</file>
Architecture diagram
sequenceDiagram
participant FE as Frontend Components
participant Hook as useDataSourceDiscovery Hook
participant Cache as Discovery Cache/Epoch
participant WS as WebSocket Kernel
participant Py as Python Kernel
participant Plugins as Discovery Plugins
participant NS as Namespace State
Note over FE,NS: Data Source Discovery with Configuration Awareness
FE->>Hook: useUnconfiguredDataSources(group)
Hook->>Cache: fetchDataSourceDiscovery()
Cache->>Cache: Check epoch & in-flight request
alt No cached promise or new epoch
Cache->>WS: waitForConnectionOpenIfNotebook()
Cache->>NS: getDiscoveryNamespace()
NS-->>Cache: dialects, storageProtocols, storageBackendTypes
Cache->>Py: DiscoverDataSources.request(namespace)
Py->>Plugins: discover_data_sources(env, namespace)
Plugins->>Plugins: Run isolated detection plugins
Plugins->>Py: Return discovered sources
Py->>Py: annotate_configured_sources()
Py-->>Cache: Sources with configured flag
Cache-->>Hook: DetectedDataSource[]
else Cache hit
Cache-->>Hook: Cached sources
end
Hook->>Hook: Filter by group & !configured
Hook-->>FE: Unconfigured sources
Note over WS,NS: Namespace Change Detection (Variables message)
WS->>WS: Handle "variables" message
WS->>NS: invalidateDataSourceDiscoveryIfNamespaceChanged()
NS->>NS: Compare namespace key
alt Namespace changed
NS->>Cache: invalidateDataSourceDiscovery()
Cache-->>FE: Trigger refetch
end
Note over WS,NS: Connection/Storage Updates
WS->>NS: "data-source-connections" / "storage-namespaces" message
NS->>NS: Compare namespace key
alt Namespace changed
NS->>Cache: invalidateDataSourceDiscovery()
end
Note over WS,Cache: Kernel Restart
WS->>Cache: invalidateDataSourceDiscovery() (fresh kernel)
Cache-->>FE: Clear stale suggestions
Note over FE: UI Consumers
FE->>FE: Session Panel (database badge)
FE->>FE: File Explorer Panel (storage badge)
FE->>FE: Quick-add component
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return []; | ||
| } | ||
| return data.filter( | ||
| (source) => matchesGroup(source, group) && !source.configured, |
Contributor
There was a problem hiding this comment.
P2: The new filtering behavior is untested at the hook boundary, so configured suggestions or database/storage grouping could regress without failing tests. Add focused hook tests covering configured sources being excluded and unconfigured catalog, database, and object-storage sources being grouped correctly.
(Based on your team's feedback about regression tests for behavior fixes.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/hooks/useDataSourceDiscovery.ts, line 47:
<comment>The new filtering behavior is untested at the hook boundary, so configured suggestions or database/storage grouping could regress without failing tests. Add focused hook tests covering configured sources being excluded and unconfigured catalog, database, and object-storage sources being grouped correctly.
(Based on your team's feedback about regression tests for behavior fixes.) .</comment>
<file context>
@@ -1,18 +1,49 @@
+ return [];
+ }
+ return data.filter(
+ (source) => matchesGroup(source, group) && !source.configured,
+ );
}
</file context>
| }, | ||
| ], | ||
| code: "engine = create_engine()", | ||
| configured: false, |
Contributor
There was a problem hiding this comment.
P3: The new configured fixtures fields are inert: QuickAddDataSources (the component under test) never reads configured, so these changes don't exercise the PR's actual behavior (hiding configured sources via useUnconfiguredDataSources, plus the new badges). Add a regression test that covers the filtering/badge behavior instead of only satisfying the type.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/editor/connections/__tests__/quick-add-data-sources.test.tsx, line 31:
<comment>The new `configured` fixtures fields are inert: `QuickAddDataSources` (the component under test) never reads `configured`, so these changes don't exercise the PR's actual behavior (hiding configured sources via `useUnconfiguredDataSources`, plus the new badges). Add a regression test that covers the filtering/badge behavior instead of only satisfying the type.</comment>
<file context>
@@ -28,6 +28,7 @@ const sources: DetectedDataSource[] = [
},
],
code: "engine = create_engine()",
+ configured: false,
},
{
</file context>
Drop the kernel configured-matcher so discovery stays a single env scan per kernel. Hide matching suggestions locally by dialect and storage type, and keep panel badges visible whenever unmatched detections remain.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 38 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Drop the local gitignore entry, accidental schema regen, leftover discovery request plumbing, and formatting-only diffs that were not part of connected-source filtering.
Match rules live on each suggestion so the frontend no longer maintains a plugin-id table that can drift.
Contributor
There was a problem hiding this comment.
2 issues found across 25 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="marimo/_data/data_source_discovery/models.py">
<violation number="1" location="marimo/_data/data_source_discovery/models.py:93">
P2: Existing callers of the exported `DetectedDataSource` now raise `TypeError` unless they supply `hides_when`. Give this field a no-op default so older constructors and payloads remain usable while migrated producers provide their rules.</violation>
</file>
<file name="packages/openapi/api.yaml">
<violation number="1" location="packages/openapi/api.yaml:1539">
P2: When the new frontend talks to a pre-change kernel, discovery responses omit `hidesWhen`, but `useDetectedDataSources` unconditionally reads `source.hidesWhen.kind`, causing the discovery filtering path to throw. Keep this response field optional during rollout and skip connected-source filtering when metadata is absent, or add an explicit protocol-version guard.
(Based on your team's feedback about backward compatibility for existing public behavior.) .</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| origins: tuple[DetectedDataSourceOrigin, ...] | ||
| configuration: tuple[DetectedDataSourceConfiguration, ...] | ||
| code: str | ||
| hides_when: DetectedDataSourceHidesWhen |
Contributor
There was a problem hiding this comment.
P2: Existing callers of the exported DetectedDataSource now raise TypeError unless they supply hides_when. Give this field a no-op default so older constructors and payloads remain usable while migrated producers provide their rules.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At marimo/_data/data_source_discovery/models.py, line 93:
<comment>Existing callers of the exported `DetectedDataSource` now raise `TypeError` unless they supply `hides_when`. Give this field a no-op default so older constructors and payloads remain usable while migrated producers provide their rules.</comment>
<file context>
@@ -62,3 +90,4 @@ class DetectedDataSource(msgspec.Struct, frozen=True, rename="camel"):
origins: tuple[DetectedDataSourceOrigin, ...]
configuration: tuple[DetectedDataSourceConfiguration, ...]
code: str
+ hides_when: DetectedDataSourceHidesWhen
</file context>
Suggested change
| hides_when: DetectedDataSourceHidesWhen | |
| hides_when: DetectedDataSourceHidesWhen = DialectHidesWhen(substrings=()) |
| - origins | ||
| - configuration | ||
| - code | ||
| - hidesWhen |
Contributor
There was a problem hiding this comment.
P2: When the new frontend talks to a pre-change kernel, discovery responses omit hidesWhen, but useDetectedDataSources unconditionally reads source.hidesWhen.kind, causing the discovery filtering path to throw. Keep this response field optional during rollout and skip connected-source filtering when metadata is absent, or add an explicit protocol-version guard.
(Based on your team's feedback about backward compatibility for existing public behavior.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/openapi/api.yaml, line 1539:
<comment>When the new frontend talks to a pre-change kernel, discovery responses omit `hidesWhen`, but `useDetectedDataSources` unconditionally reads `source.hidesWhen.kind`, causing the discovery filtering path to throw. Keep this response field optional during rollout and skip connected-source filtering when metadata is absent, or add an explicit protocol-version guard.
(Based on your team's feedback about backward compatibility for existing public behavior.) .</comment>
<file context>
@@ -1527,6 +1536,7 @@ components:
- origins
- configuration
- code
+ - hidesWhen
title: DetectedDataSource
type: object
</file context>
Import DataSourceDiscoveryGroup from the module that exports it so typecheck passes, and pluralize the tooltip noun when count is above one.
Light2Dark
changed the title
Light2Dark
changed the title
Light2Dark
changed the title
Member
Author
|
@mscolnick, wdyt? I didn't want to pepper new places where we discovered data source connections when it's already connected. Unfortunately, that requires an api addition to make it robust & maintainable imo. not sure if this breaks compat with vscode |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends marimo’s data-source discovery model with “hide-when-connected” rules (dialect/storage matching) so environment-detected suggestions can be surfaced in the UI (badges, dialogs) but automatically disappear once an equivalent live connection/storage namespace exists. It also introduces a shared, epoch-based discovery cache so discovery is fetched once per kernel lifetime and invalidated on kernel restart.
Changes:
- Added
hidesWhenmetadata toDetectedDataSourceacross backend models + OpenAPI schema, with helper constructors and plugin updates. - Implemented frontend filtering (
matchesDiscoveryGroup,isDetectedSourceConnected) and a shared discovery fetch cache invalidated on kernel restart. - Added UI surfaces for pending discovered connections (panel sparkle badges) and updated “Add Connection” dialog/quick-add to respect group + “already connected” filtering.
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/_data/data_source_discovery/test_trino.py | Updates expected discovery payloads to include hidesWhen for Trino. |
| tests/_data/data_source_discovery/test_pyspark.py | Updates expected discovery payloads to include hidesWhen for Spark. |
| tests/_data/data_source_discovery/test_postgres.py | Updates expected discovery payloads to include hidesWhen for Postgres. |
| tests/_data/data_source_discovery/test_mysql.py | Updates expected discovery payloads to include hidesWhen for MySQL. |
| tests/_data/data_source_discovery/test_discovery_pyiceberg.py | Updates expected discovery payloads to include hidesWhen for PyIceberg. |
| tests/_data/data_source_discovery/test_discovery_huggingface.py | Updates expected discovery payloads to include hidesWhen for HuggingFace storage. |
| tests/_data/data_source_discovery/test_discover.py | Updates discovery-level tests to construct DetectedDataSource with hides_when. |
| tests/_data/data_source_discovery/test_aws.py | Updates expected discovery payloads to include hidesWhen for S3-compatible storage. |
| packages/openapi/src/api.ts | Updates generated TS API types to include DetectedDataSource.hidesWhen union. |
| packages/openapi/api.yaml | Adds DialectHidesWhen/StorageHidesWhen schemas and requires hidesWhen on DetectedDataSource. |
| marimo/_schemas/generated/notifications.yaml | Mirrors OpenAPI schema updates for notifications payloads. |
| marimo/_data/data_source_discovery/plugins/trino.py | Attaches dialect hide rules to Trino environment suggestions. |
| marimo/_data/data_source_discovery/plugins/pyspark.py | Attaches dialect hide rules to Spark environment suggestions. |
| marimo/_data/data_source_discovery/plugins/pyiceberg.py | Attaches dialect hide rules and preserves them during merge logic. |
| marimo/_data/data_source_discovery/plugins/postgres.py | Attaches dialect hide rules to Postgres environment suggestions. |
| marimo/_data/data_source_discovery/plugins/mysql.py | Attaches dialect hide rules to MySQL environment suggestions. |
| marimo/_data/data_source_discovery/plugins/huggingface.py | Attaches storage hide rules (protocol/backend type) to HuggingFace suggestions. |
| marimo/_data/data_source_discovery/plugins/aws.py | Attaches storage hide rules (protocols) to S3-compatible suggestions. |
| marimo/_data/data_source_discovery/models.py | Adds tagged DialectHidesWhen/StorageHidesWhen structs and makes DetectedDataSource.hides_when required. |
| marimo/_data/data_source_discovery/helpers.py | Adds helper constructors hides_when_dialect / hides_when_storage. |
| marimo/_data/data_source_discovery/init.py | Exposes new hide-when model types in package exports. |
| frontend/src/hooks/useDataSourceDiscovery.ts | Switches to epoch-based discovery fetching and adds useDetectedDataSources filtering hook. |
| frontend/src/hooks/tests/useDataSourceDiscovery.test.ts | Removes obsolete test for the previous request wrapper. |
| frontend/src/core/websocket/useMarimoKernelConnection.tsx | Invalidates discovery cache on kernel restart; minor refactor for variable name extraction. |
| frontend/src/core/datasets/data-source-discovery.ts | Adds filtering/grouping logic plus a shared cached discovery fetch with explicit invalidation. |
| frontend/src/core/datasets/data-source-connections.ts | Moves connectionsAtom to core layer and sorts internal engines last. |
| frontend/src/core/datasets/tests/data-source-discovery.test.ts | Adds unit tests for caching/invalidation + matching/hiding logic. |
| frontend/src/components/editor/connections/quick-add-data-sources.tsx | Uses useDetectedDataSources and supports optional submit handler + group filtering. |
| frontend/src/components/editor/connections/add-connection-dialog.tsx | Narrows discovered suggestions shown in the dialog by active tab (“storage” vs “database”). |
| frontend/src/components/editor/connections/tests/quick-add-data-sources.test.tsx | Updates fixtures to include required hidesWhen field. |
| frontend/src/components/editor/chrome/panels/session-panel.tsx | Adds “sparkles” badge showing pending discovered database sources. |
| frontend/src/components/editor/chrome/panels/file-explorer-panel.tsx | Adds “sparkles” badge showing pending discovered storage sources. |
| frontend/src/components/editor/chrome/panels/components.tsx | Introduces DiscoveredSourcesBadge UI component (tooltip + count). |
| frontend/src/components/datasources/datasources.tsx | Updates imports to use the new core connectionsAtom (removes local atom). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+99
to
+102
| noun = count === 1 ? "database" : "databases"; | ||
| } else { | ||
| noun = count === 1 ? "remote storage" : "remote storages"; | ||
| } |
PanelBadge is not a ref-forwarding component, so Radix asChild could not bind hover handlers to the sparkle count.
Main landed the plugin without hides_when, so the merge failed typecheck and DetectedDataSource construction in CI.
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.
This pull request was authored by a coding agent.
📝 Summary
We want environment-detected connections to show up in more of the UI — panel badges, the Add Connection dialog, and later surfaces — so people can quick-add what the kernel already found. Those prompts should disappear once a matching engine or remote storage is connected. Repeating a suggestion next to a live connection is noise.
This PR is the foundation for that. Each plugin ships a hide-when rule with the suggestion (dialect substring, or storage protocol / backend type). The frontend applies it locally against live connections and storage namespaces, so every new surface can reuse the same filter instead of inventing its own matching. Discovery is fetched once per kernel epoch and refetched only on a genuine restart.
This change adds the first two surfaces: sparkle badges on the Data sources and Remote storage headers
Screen.Recording.2026-08-18.at.12.16.21.AM.mov
📋 Pre-Review Checklist
✅ Merge Checklist