refactor: dedupe marimo dropdown and connection helpers#3
Conversation
Extract shared option builders into _options.py, reuse resolve_connection_picker in TableBrowser, and resolve SQL engine connection ids via runtime.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| def resolve_connection_picker( | ||
| client: HotdataClient, | ||
| *, | ||
| label: str = "Connection", | ||
| full_width: bool = True, | ||
| ) -> tuple[Any | None, str | None]: | ||
| """Return ``(dropdown_or_none, implicit_connection_id)`` for table browsers.""" | ||
| listing = client.connections().list_connections() | ||
| conns = listing.connections | ||
| if not conns: | ||
| return None, "" | ||
| if len(conns) == 1: | ||
| return None, conns[0].id | ||
| return connection_picker(client, label=label, full_width=full_width), None |
There was a problem hiding this comment.
nit: when len(conns) > 1, resolve_connection_picker calls client.connections().list_connections() here, then immediately calls connection_picker(client, ...) which fetches the same listing again. Since you've just fetched conns, consider passing them through to avoid the duplicate API round-trip (e.g., split connection_picker so the dropdown can be built from a pre-fetched conns list). The pre-refactor code had the same duplicate fetch, so this isn't a regression — just an opportunity now that the logic is in one place. (not blocking)
Build the multi-connection dropdown from a pre-fetched connections list via connection_picker_from_connections.
Use mo.ui.tabs for workspace, status, browser, SQL editor, recent results, and run history instead of a single vertical stack.
WorkspaceSelector, TableBrowser, SqlEditor, and RecentResults expose Marimo widgets via .ui; passing the wrapper objects printed reprs instead of UI. Restore query result cells for the SQL editor and recent-results picker.
Keep editor and recent-results cells as Marimo dependency triggers only; query output lives inside each tab via editor.ui and recent.ui.
| @app.cell | ||
| def _(editor, hm): | ||
| def _(editor): | ||
| # Explicitly touch nested widget values so Marimo reruns this cell on clicks. | ||
| _run = editor.run.value | ||
| _rerun = editor.rerun.value | ||
| _clear = editor.clear.value | ||
| return hm.query_result(editor.result), _clear, _rerun, _run | ||
| return | ||
|
|
||
|
|
||
| @app.cell | ||
| def _(hm, recent): | ||
| def _(recent): | ||
| _selected = recent.pick.value | ||
| return hm.query_result(recent.result, label="Recent result"), _selected | ||
| return |
There was a problem hiding this comment.
nit: this looks like a behavior regression that is out of scope for a dedupe refactor. The previous version had these cells return hm.query_result(editor.result) / hm.query_result(recent.result, label="Recent result") — accessing .result is what triggers the SQL execution (and rendered the result component). After this change nothing in the demo accesses editor.result or recent.result, so clicking Run in the SQL editor or picking a recent result produces no visible output and the underlying query is never invoked. If the tab restructuring is intentional, consider re-adding a SQL result / Recent result tab (or rendering the result component inside the existing tabs) so the editor and recent picker still do something. Otherwise this seems orthogonal to the helper-dedupe goal and could move to its own PR. (not blocking)
| def _connection_id(self, connection_name: str) -> str | None: | ||
| return self._connection_ids().get(connection_name) | ||
| try: | ||
| return self._connection.connection_id_by_name().get(connection_name) | ||
| except RuntimeError as e: | ||
| LOGGER.warning("%s", e) | ||
| return None |
There was a problem hiding this comment.
nit: this drops the local _connection_id_cache and now calls self._connection.connection_id_by_name() on every invocation. _connection_id is hit from get_schemas, get_tables_in_schema, and get_table_details — each of which can fire several times per render — so unless connection_id_by_name() is cheap or memoized on the runtime side, this will multiply backend calls during catalog browsing. Worth confirming the runtime caches the mapping, or memoize locally (e.g., one cached dict reused alongside _connections_cache). (not blocking)
| from hotdata_marimo._options import ( | ||
| connection_picker, | ||
| empty_dropdown, | ||
| resolve_connection_picker, | ||
| ) |
There was a problem hiding this comment.
super nit: connection_picker isn't referenced anywhere in this module's body — it's imported solely so that from hotdata_marimo.table_browser import connection_picker in __init__.py keeps working. That's fine, but a one-line comment (or # re-exported for backwards-compat) would make the intent obvious to a future reader who might otherwise drop it as an unused import. (not blocking)
Add connections_panel with health callout plus a paginated table of name, id, and source_type. Use it in the demo Connections tab.
Extract _sync_table_catalog on TableBrowser, add demo watcher cells so Marimo reruns the datasets panel when the connection dropdown changes, and improve empty-state hints.
Marimo does not infer browser_ui from return (browser.ui,); assign before return.
Wire editor.result and recent.result into dedicated cells and stack them below the SQL editor and recent-results pickers inside each tab.
Remove Datasets and SQL query tabs from the explorer demo, show recent results in a selectable table instead of a dropdown, and add tab_ui helpers so mo.stop in result cells does not block the tab layout.
Remove debug click counters from the SQL editor, align README examples with the current demo, and document Marimo auth tokens for shared hosts.
1 participant
Summary
hotdata_marimo/_options.pywith sharedunique_label_options,empty_dropdown, and connection picker helpersresolve_connection_pickerinTableBrowserinstead of duplicating 0/1/many connection logicHotdataClient.connection_id_by_name()Test plan
uv run pytest(46 passed)