-
Notifications
You must be signed in to change notification settings - Fork 660
Enhance frontend dashboard UX and backend SDK with improved error handling and type hints #699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,21 +23,24 @@ const Dashboard = ({ components, error, handleComponentUpdate }) => { | |
); | ||
} | ||
|
||
if (!isValidComponents) { | ||
if (!components || !components.rows) { | ||
return ( | ||
<div className="dashboard-loading-container"> | ||
<LoadingState | ||
isConnected={true} | ||
customText={!components ? 'Loading components' : 'Invalid components data'} | ||
/> | ||
<div className="dashboard-empty"> | ||
<p className="dashboard-empty-text"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the existing loadingstate component is good |
||
{!components | ||
? 'Loading components...' | ||
: 'Invalid components data. Please check your configuration or data source.'} | ||
</p> | ||
</div> | ||
); | ||
} | ||
|
||
if (components.rows.length === 0) { | ||
return ( | ||
<div className="dashboard-empty"> | ||
<p className="dashboard-empty-text">No components to display</p> | ||
<p className="dashboard-empty-text"> | ||
No components to display. Try running a query or verifying your configuration settings. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this messaging is right |
||
</p> | ||
</div> | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import logging | ||
import time | ||
|
||
import pandas as pd | ||
|
||
|
@@ -12,6 +13,9 @@ | |
def connect(): | ||
""" | ||
Connect to all listed data sources in preswald.toml | ||
|
||
Returns: | ||
A DuckDBPyConnection instance if connection is successful, otherwise None. | ||
""" | ||
try: | ||
service = PreswaldService.get_instance() | ||
|
@@ -21,25 +25,44 @@ def connect(): | |
return duckdb_conn | ||
except Exception as e: | ||
logger.error(f"Error connecting to datasources: {e}") | ||
return None | ||
|
||
|
||
def query(sql: str, source_name: str) -> pd.DataFrame: | ||
def query(sql: str, source_name: str) -> pd.DataFrame | None: | ||
""" | ||
Query a data source using sql from preswald.toml by name | ||
Query a data source using sql from preswald.toml by name. | ||
|
||
Args: | ||
sql: The SQL query to run. | ||
source_name: The name of the data source configured in preswald.toml. | ||
|
||
Returns: | ||
A pandas DataFrame if successful, otherwise None | ||
""" | ||
try: | ||
service = PreswaldService.get_instance() | ||
# Measure and log query execution time for performance monitoring | ||
start_time = time.time() | ||
df_result = service.data_manager.query(sql, source_name) | ||
elapsed_time = time.time() - start_time | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be removed since its not used |
||
logger.info(f"Successfully queried data source: {source_name}") | ||
return df_result | ||
except Exception as e: | ||
logger.error(f"Error querying data source: {e}") | ||
return None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. imo, should error out instead of returning None |
||
|
||
|
||
def get_df(source_name: str, table_name: str | None = None) -> pd.DataFrame: | ||
""" | ||
Get a dataframe from the named data source from preswald.toml | ||
If the source is a database/has multiple tables, you must specify a table_name | ||
|
||
Args: | ||
source_name: The name of the configured data source. | ||
table_name: Optional; name of the table to load if the source has multiple tables. | ||
|
||
Returns: | ||
A pandas DataFrame if successful, otherwise None. | ||
""" | ||
try: | ||
service = PreswaldService.get_instance() | ||
|
@@ -48,3 +71,4 @@ def get_df(source_name: str, table_name: str | None = None) -> pd.DataFrame: | |
return df_result | ||
except Exception as e: | ||
logger.error(f"Error getting a dataframe from data source: {e}") | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem wise to remove loadingstate component