Display Status of latest run in Editor #40
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduce a new optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant WorkflowRunStatus
participant WorkflowRunHistory
User->>API: Request workflow runs with limit
API->>WorkflowRunStatus: Fetch latest workflow run
WorkflowRunStatus-->>API: Return latest run data
API-->>User: Return workflow runs
User->>WorkflowRunHistory: Display workflow runs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (12)
web/src/components/workflow-editor/run-history/workflow-run-status-indicator.tsx (2)
5-9: Consider restructuring props destructuring for better maintainability.The current implementation destructures nested properties directly in the function parameters. This pattern can become harder to maintain as more properties are added.
Consider this alternative structure:
-export default function WorkflowRunStatusIndicator({ - workflowRun: { failedAt, completedAt }, -}: { - workflowRun: TWorkflowRunMetadata; -}) { +export default function WorkflowRunStatusIndicator({ + workflowRun, +}: { + workflowRun: TWorkflowRunMetadata; +}) { + const { failedAt, completedAt } = workflowRun;
10-34: Consider adding a tooltip to provide more context.The status indicators could benefit from tooltips to provide more detailed information about the workflow state.
Consider wrapping each status indicator with Radix UI's Tooltip component to show timestamps or additional status details when users hover over the icons.
Example implementation:
import { Tooltip } from '@radix-ui/themes'; // Inside your render logic <Tooltip content={`Failed at ${new Date(failedAt).toLocaleString()}`}> <Flex align="center" gap="2"> <CrossCircledIcon color={STATUS_COLORS.failure} aria-label="Workflow run failed" /> </Flex> </Tooltip>web/src/components/workflow-editor/run-history/latest-workflow-run-status.tsx (2)
8-10: Consider adding prop validation and documentation.While the implementation is functional, consider these improvements:
- Add prop-types or TypeScript interface for better type safety
- Document the hardcoded limit value of 1
- Consider adding error boundaries for better error handling
+interface WorkflowRunStatusProps { + /** The unique identifier of the workflow to display status for */ + workflowId: string; +} -export function WorkflowRunStatus({ workflowId }: { workflowId: string }) { +export function WorkflowRunStatus({ workflowId }: WorkflowRunStatusProps) { // Consider adding a comment explaining why limit is set to 1 const { data, isPending, error } = useListWorkflowRunsApi(workflowId, 1);
26-31: Enhance accessibility and responsiveness.The component could benefit from improved accessibility and responsive design:
return ( - <Flex align="center" gap="2" height="25px"> + <Flex + align="center" + gap="2" + className="min-h-[25px]" + role="status" + aria-label={`Workflow status: ${latestRun.status}`} + > <WorkflowRunStatusIndicator workflowRun={latestRun} /> </Flex> );web/src/hooks/use-list-workflow-runs-api.ts (1)
13-19: LGTM! Consider adding input validation for the limit parameter.The implementation correctly handles the optional limit parameter and maintains type safety.
Consider adding input validation to ensure the limit is a positive number:
const buildListWorkflowRunsApi = (workflowId: string, limit?: number) => { + if (limit !== undefined && limit <= 0) { + throw new Error('Limit must be a positive number'); + } return api< z.infer<typeof ListWorkflowRunsRequest>, z.infer<typeof ListWorkflowRunsResponse> >({ method: HTTPMethod.GET, path: `/api/v1/runs/${workflowId}${limit ? `?limit=${limit}` : ""}`, requestSchema: ListWorkflowRunsRequest, responseSchema: ListWorkflowRunsResponse, }); };admyral/server/endpoints/workflow_run_endpoints.py (1)
32-32: Update function docstring to document the limit parameter.The docstring should be updated to include the new
limitparameter.Update the docstring like this:
""" List all workflow runs. Args: workflow_id: The workflow id + limit: Maximum number of workflow runs to return (default: 100) + authenticated_user: The authenticated user Returns: A list of workflow runs. """web/src/components/workflow-editor/run-history/workflow-run-trace.tsx (1)
81-90: LGTM! Consider making dimensions responsive.The new Flex container structure improves layout consistency and alignment. The fixed dimensions ensure proper icon sizing and spacing.
Consider using responsive units (rem/em) instead of fixed pixel values for better adaptability across different screen sizes:
<Flex - width="24px" - height="24px" + width="1.5rem" + height="1.5rem" justify="center" >web/src/components/workflow-editor/run-history/workflow-run-history.tsx (3)
16-18: Consider locale-aware date formatting.The date formatting is currently hardcoded to 'en-US'. Consider making this configurable or using a locale-aware solution for better internationalization support.
-const createdAtAsReadableString = new Date( - workflowRun.createdAt, -).toLocaleString("en-US"); +const createdAtAsReadableString = new Date( + workflowRun.createdAt +).toLocaleString(navigator.language || 'en-US');
12-24: Consider memoizing the WorkflowRunRow component.Since this component is used within a list and receives objects as props, memoizing it could help prevent unnecessary re-renders.
-function WorkflowRunRow({ +const WorkflowRunRow = React.memo(function WorkflowRunRow({ workflowRun, }: { workflowRun: TWorkflowRunMetadata; -}) { +}) { const createdAtAsReadableString = new Date( workflowRun.createdAt ).toLocaleString("en-US"); return ( <Flex align="center" justify="between" width="100%"> <Text size="1">{createdAtAsReadableString}</Text> <WorkflowRunStatusIndicator workflowRun={workflowRun} /> </Flex> ); -} +});
Based on the findings from the API endpoint implementation and frontend code, I can now generate the final response:
Implement pagination support to prevent loss of selected run state
The current implementation has confirmed limitations:
- Backend API (
list_workflow_runs) supports alimitparameter but no offset/cursor- Frontend uses
useListWorkflowRunsApiwith only limit parameter- No pagination UI or state management exists
To address the TODO comment, we need:
- Add offset/cursor support in the backend API
- Extend the frontend hook to support pagination
- Implement UI controls for navigating through run history
- Store selected run data separately to preserve selection state
🔗 Analysis chain
Line range hint
40-44: Address potential loss of selected run state.The current implementation might lose the selected run when it falls outside the 100-run window. Consider:
- Implementing pagination to allow access to older runs
- Preserving the selected run data even if it falls outside the window
- Adding a user notification when the selected run is no longer in the list
Let's check if the API supports pagination:
Would you like me to propose a detailed solution for handling this edge case?
Let me check the frontend implementation to understand if there's any existing pagination or offset support:
Let me try a different search pattern to find the workflow runs fetching logic:
Let me check the hook implementation to understand the API usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for API endpoint implementation rg -A 5 "list_workflow_runs"Length of output: 2162
Script:
#!/bin/bash # Search for workflow run fetching logic in frontend rg -A 10 "getWorkflowRuns|fetchWorkflowRuns" web/ # Check if there's any pagination UI or state management ast-grep --pattern 'const [offset|page|cursor]'Length of output: 101
Script:
#!/bin/bash # Search for API calls related to workflow runs rg -A 10 "workflow.*runs.*api" web/ # Look for the component's data fetching logic rg -A 10 "useEffect.*runs" web/src/components/workflow-editor/run-history/Length of output: 2894
Script:
#!/bin/bash # Look for the hook implementation rg -A 15 "useListWorkflowRunsApi" web/src/hooks/ # Check if there's any pagination/offset logic in the types rg -A 5 "TWorkflowRunMetadata" web/src/types/Length of output: 1052
web/src/components/workflow-editor/workflow-editor.tsx (2)
146-147: Move cursor styles to CSS classes.Instead of inline styles, consider moving the cursor styles to CSS classes for better maintainability and consistency.
-style={{ cursor: "pointer" }} +className="cursor-pointer"Also applies to: 152-153
138-161: Consider responsive design and spacing.A few suggestions to improve the implementation:
- Add overflow handling for narrow screens where tabs and status might not fit
- Define consistent spacing between tabs and status indicator
-<Flex align="center"> +<Flex + align="center" + gap="2" + style={{ + minWidth: 0, + overflow: 'hidden' + }} +> <Tabs.List size="1"> {/* ... */} </Tabs.List> <WorkflowRunStatus workflowId={workflowId} /> </Flex>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
admyral/server/endpoints/workflow_run_endpoints.py(2 hunks)web/src/components/workflow-editor/run-history/latest-workflow-run-status.tsx(1 hunks)web/src/components/workflow-editor/run-history/workflow-run-history.tsx(1 hunks)web/src/components/workflow-editor/run-history/workflow-run-status-indicator.tsx(1 hunks)web/src/components/workflow-editor/run-history/workflow-run-trace.tsx(1 hunks)web/src/components/workflow-editor/workflow-editor.tsx(3 hunks)web/src/hooks/use-list-workflow-runs-api.ts(1 hunks)
🧰 Additional context used
🪛 Ruff
admyral/server/endpoints/workflow_run_endpoints.py
20-20: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (10)
web/src/components/workflow-editor/run-history/workflow-run-status-indicator.tsx (1)
1-4: LGTM! Well-organized imports following best practices.
The imports are properly organized, separating UI components, icons, and types. Using Radix UI ensures consistent styling and accessibility.
web/src/components/workflow-editor/run-history/latest-workflow-run-status.tsx (1)
1-7: LGTM! Imports are well-organized.
The imports are properly structured and include all necessary dependencies for the component's functionality.
web/src/hooks/use-list-workflow-runs-api.ts (1)
26-29: LGTM! Verify the refetch interval's impact on performance.
The implementation correctly handles the limit parameter in both the query key and query function. The 1-second refetch interval is suitable for real-time status updates but might need monitoring.
Let's check if this hook is used with small limits when real-time updates are needed:
✅ Verification successful
The verification results show that the hook is used in two components:
WorkflowRunStatuscomponent which useslimit=1specifically for showing latest run statusWorkflowRunHistorycomponent which doesn't specify a limit, fetching all runs
Since the component fetching real-time updates (WorkflowRunStatus) uses a limit of 1, the refetch interval's performance impact is minimal as it's only fetching a single record. The other usage without a limit is in the history view where we want to show all runs anyway.
The refetch interval implementation is appropriate and performant
The real-time status updates are correctly implemented with a minimal payload (limit=1) where frequent updates matter, while the full history view fetches all records where immediate updates are less critical.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usage of useListWorkflowRunsApi to verify the refetch interval's impact
# Test: Look for components using this hook with different limit values
# Search for imports and usages
echo "Searching for hook usage patterns..."
rg "useListWorkflowRunsApi" -A 3
# Search specifically for the latest run status component
echo -e "\nSearching for latest run status component..."
fd "latest-workflow-run-status" --exec cat {}
Length of output: 3638
admyral/server/endpoints/workflow_run_endpoints.py (1)
32-32: Verify store implementation of the limit parameter.
Let's ensure the store layer properly implements the limit parameter.
✅ Verification successful
Store implementation correctly handles the limit parameter
The store layer properly implements the limit parameter in list_workflow_runs:
- Accepts the limit parameter with default value of 100
- Applies the limit in the SQL query using
.limit(limit) - Orders results by creation date descending to get latest runs first
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the store implementation of list_workflow_runs
ast-grep --pattern 'def list_workflow_runs($$$)'
# Search for any existing pagination/limit implementations in store
rg -l 'limit.*workflow.*run' --type py
Length of output: 91
Script:
#!/bin/bash
# Let's try a broader search for workflow runs related functions in Python files
rg -A 5 "list_workflow_runs" --type py
# Also search for workflow store implementations
rg -A 5 "class.*WorkflowStore" --type py
# Search for any workflow run related queries
rg -A 5 "workflow.*run.*limit" --type py --ignore-case
Length of output: 2272
Script:
#!/bin/bash
# Let's see the complete implementation of list_workflow_runs in admyral_store.py
rg -A 15 "async def list_workflow_runs" admyral/db/admyral_store.py
Length of output: 805
web/src/components/workflow-editor/run-history/workflow-run-trace.tsx (2)
92-92: LGTM! Consistent text alignment.
The explicit left alignment improves readability and matches the PR requirement for left-aligned Steps in the Workflow History.
Line range hint 1-1: Verify implementation of latest run status display.
While the layout improvements are good, I notice that this component doesn't implement the "latest run status display" feature mentioned in the PR objectives. This might be implemented in another component, but let's verify.
web/src/components/workflow-editor/run-history/workflow-run-history.tsx (1)
3-3: LGTM! Import changes align with the new status display architecture.
The removal of direct icon imports in favor of the new WorkflowRunStatusIndicator component promotes better separation of concerns and reusability.
Also applies to: 9-9
web/src/components/workflow-editor/workflow-editor.tsx (3)
25-25: LGTM!
The import statement follows the project's conventions and is properly scoped.
117-117: Good improvement to the layout!
Changing the middle column from a fixed width to auto provides better flexibility for the tabs and status indicator while maintaining the balanced layout.
158-160: Verify error handling in WorkflowRunStatus.
The WorkflowRunStatus component should handle loading and error states gracefully to maintain a good user experience.
✅ Verification successful
Loading and error states are properly implemented in WorkflowRunStatus
The component already implements proper error and loading handling:
- Uses
isPendingstate to handle loading - Returns
ErrorCalloutcomponent when there's an error - Safely handles the case when no run data is available
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if WorkflowRunStatus implements proper loading and error states
ast-grep --pattern 'export function WorkflowRunStatus($_) {
$$$
return $_
}'
Length of output: 2543
web/src/components/workflow-editor/run-history/workflow-run-status-indicator.tsx
Outdated
Show resolved
Hide resolved
web/src/components/workflow-editor/run-history/workflow-run-status-indicator.tsx
Outdated
Show resolved
Hide resolved
web/src/components/workflow-editor/run-history/latest-workflow-run-status.tsx
Outdated
Show resolved
Hide resolved
web/src/components/workflow-editor/run-history/latest-workflow-run-status.tsx
Outdated
Show resolved
Hide resolved
leon-schmid
force-pushed
the
ADM-360/display-run-status-in-editor
branch
from
008c9dc
to
ed9b9cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
admyral/server/endpoints/workflow_run_endpoints.py (1)
Line range hint
18-28: Update docstring to document thelimitparameter.The docstring should be updated to include documentation for the new
limitparameter.Apply this diff to update the docstring:
""" List all workflow runs. Args: workflow_id: The workflow id + limit: Maximum number of workflow runs to return (default: 100) Returns: A list of workflow runs. """🧰 Tools
🪛 Ruff
20-20: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
admyral/server/endpoints/workflow_run_endpoints.py(2 hunks)web/src/components/workflow-editor/run-history/latest-workflow-run-status.tsx(1 hunks)web/src/components/workflow-editor/run-history/workflow-run-history.tsx(1 hunks)web/src/components/workflow-editor/run-history/workflow-run-status-indicator.tsx(1 hunks)web/src/components/workflow-editor/run-history/workflow-run-trace.tsx(1 hunks)web/src/components/workflow-editor/workflow-editor.tsx(3 hunks)web/src/hooks/use-list-workflow-runs-api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- web/src/components/workflow-editor/run-history/latest-workflow-run-status.tsx
- web/src/components/workflow-editor/run-history/workflow-run-history.tsx
- web/src/components/workflow-editor/run-history/workflow-run-status-indicator.tsx
- web/src/components/workflow-editor/run-history/workflow-run-trace.tsx
- web/src/components/workflow-editor/workflow-editor.tsx
- web/src/hooks/use-list-workflow-runs-api.ts
🧰 Additional context used
🪛 Ruff
admyral/server/endpoints/workflow_run_endpoints.py
20-20: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (1)
admyral/server/endpoints/workflow_run_endpoints.py (1)
32-32: LGTM!
The store method call is correctly updated to pass the limit parameter using named argument syntax, which improves code readability.
| if (error) { | ||
| return <ErrorCallout />; | ||
| } |
There was a problem hiding this comment.
Did you actually test how this looks? I think this would look really strange. I actually think in case of an error, we should just return null to not render anything.
| }, [data]); | ||
|
|
||
| if (isPending || !latestRun) { | ||
| return; |
There was a problem hiding this comment.
| return; | |
| return null; |
| api< | ||
| z.infer<typeof ListWorkflowRunsRequest>, | ||
| z.infer<typeof ListWorkflowRunsResponse> | ||
| >({ | ||
| method: HTTPMethod.GET, | ||
| path: `/api/v1/runs/${workflowId}`, | ||
| path: `/api/v1/runs/${workflowId}${limit ? `?limit=${limit}` : ""}`, |
There was a problem hiding this comment.
this is not how query parameters are supposed to be handled. The api builder automatically builds the query parameters for you, you just need to pass the parameters. It'll know what to do because you specify the HTTP method as GET.
See useGetWorkflowApi as a reference and review api.ts.
This PR changes the following:
Summary by CodeRabbit
Release Notes
New Features
limitparameter for thelist_workflow_runsendpoint, allowing users to specify the maximum number of workflow runs returned.WorkflowRunStatuscomponent to display the status of the latest workflow run.WorkflowRunStatusIndicatorcomponent for visual representation of workflow run statuses.Improvements
WorkflowRunRowcomponent to simplify status display logic.WorkflowEditorfor better organization and real-time status updates.WorkflowRunTracecomponent for improved visual consistency.Bug Fixes