BLDX-547: feat: SDK UI config fetch route added#1053
Draft
sachi-atlan wants to merge 2 commits intonativefrom
Draft
BLDX-547: feat: SDK UI config fetch route added#1053sachi-atlan wants to merge 2 commits intonativefrom
sachi-atlan wants to merge 2 commits intonativefrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📜 Docstring Coverage ReportRESULT: PASSED (minimum: 30.0%, actual: 77.3%) Detailed Coverage Report |
sachi-atlan
changed the title
📦 Trivy Vulnerability Scan Results
Report SummaryCould not generate summary table (data length mismatch: 9 vs 8). Scan Result Detailsrequirements.txtuv.lock |
📦 Trivy Secret Scan Results
Report Summary
Scan Result Details✅ No secrets found during the scan for |
|
🛠 Docs available at: https://k.atlan.dev/application-sdk/ui-config |
Collaborator
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
|
🛠 Full Test Coverage Report: https://k.atlan.dev/coverage/application-sdk/pr/1053 |
Contributor
|
@greptile review |
Greptile SummaryAdds GET Key Changes:
Issues Found:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| application_sdk/handlers/init.py | Added get_uiconfig() static method to HandlerInterface with default empty dict return |
| application_sdk/server/fastapi/init.py | Added GET /uiconfigs endpoint with get_uiconfig() handler; exception handling pattern differs from similar endpoints |
| application_sdk/server/fastapi/models.py | Added UIConfigResponse model with success, message, and data fields following existing patterns |
Sequence Diagram
sequenceDiagram
participant Client
participant FastAPI as FastAPI Server
participant Handler as HandlerInterface
participant App as App Implementation
Client->>FastAPI: GET /uiconfigs
FastAPI->>FastAPI: get_uiconfig()
FastAPI->>Handler: Check handler initialized
FastAPI->>App: await handler.get_uiconfig()
App-->>Handler: Return UI config dict
Handler-->>FastAPI: UI config data
FastAPI->>FastAPI: Build UIConfigResponse
FastAPI-->>Client: {success, message, data}
Note over FastAPI,App: Default implementation returns {}<br/>Apps override to serve custom config
Last reviewed commit: 7267a3f
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+755
to
+761
| except Exception as e: | ||
| logger.error(f"Error fetching UI configuration: {e}") | ||
| return UIConfigResponse( | ||
| success=False, | ||
| message=f"Failed to fetch UI configuration: {str(e)}", | ||
| data={}, | ||
| ) |
There was a problem hiding this comment.
Exception is caught and swallowed instead of re-raised. Other similar endpoints (test_auth, fetch_metadata, preflight_check) re-raise after logging. This violates the exception handling standard which requires re-raising exceptions for critical operations.
Suggested change
| except Exception as e: | |
| logger.error(f"Error fetching UI configuration: {e}") | |
| return UIConfigResponse( | |
| success=False, | |
| message=f"Failed to fetch UI configuration: {str(e)}", | |
| data={}, | |
| ) | |
| except Exception as e: | |
| logger.error(f"Error fetching UI configuration: {e}") | |
| raise e |
Context Used: Context from dashboard - .cursor/rules/exception-handling.mdc (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+738
to
+761
| async def get_uiconfig(self) -> UIConfigResponse: | ||
| """Get the UI configuration for this application. | ||
|
|
||
| Returns: | ||
| UIConfigResponse: Response containing the UI configuration. | ||
| """ | ||
| try: | ||
| if not self.handler: | ||
| raise Exception("Handler not initialized") | ||
|
|
||
| uiconfig_data = await self.handler.get_uiconfig() | ||
|
|
||
| return UIConfigResponse( | ||
| success=True, | ||
| message="UI configuration fetched successfully", | ||
| data=uiconfig_data, | ||
| ) | ||
| except Exception as e: | ||
| logger.error(f"Error fetching UI configuration: {e}") | ||
| return UIConfigResponse( | ||
| success=False, | ||
| message=f"Failed to fetch UI configuration: {str(e)}", | ||
| data={}, | ||
| ) |
There was a problem hiding this comment.
Missing metrics collection. Similar endpoints (test_auth, fetch_metadata, preflight_check) track success/error counts and duration. Consider adding metrics for observability:
metrics.record_metric(
name="uiconfig_requests_total",
value=1.0,
metric_type=MetricType.COUNTER,
labels={"status": "success|error"},
description="Total number of UI config fetch requests"
)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+738
to
+761
| async def get_uiconfig(self) -> UIConfigResponse: | ||
| """Get the UI configuration for this application. | ||
|
|
||
| Returns: | ||
| UIConfigResponse: Response containing the UI configuration. | ||
| """ | ||
| try: | ||
| if not self.handler: | ||
| raise Exception("Handler not initialized") | ||
|
|
||
| uiconfig_data = await self.handler.get_uiconfig() | ||
|
|
||
| return UIConfigResponse( | ||
| success=True, | ||
| message="UI configuration fetched successfully", | ||
| data=uiconfig_data, | ||
| ) | ||
| except Exception as e: | ||
| logger.error(f"Error fetching UI configuration: {e}") | ||
| return UIConfigResponse( | ||
| success=False, | ||
| message=f"Failed to fetch UI configuration: {str(e)}", | ||
| data={}, | ||
| ) |
There was a problem hiding this comment.
Verify tests exist for the new /uiconfigs endpoint following patterns in tests/unit/server/fastapi/test_fastapi.py
Context Used: Context from dashboard - .cursor/rules/testing.mdc (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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
4 participants
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.
Description
Adds a new GET /uiconfigs endpoint to the workflow router that allows apps to serve their UI configuration directly from the application server instead of relying on ConfigMaps stored in Elasticsearch/Argo.
Changes:
This is part of the migration away from Argo-managed ConfigMaps. Each app bundles its own UI config YAML and serves it via this endpoint.
Additional context (e.g. screenshots, logs, links)
Checklist
Copyleft License Compliance