-
Notifications
You must be signed in to change notification settings - Fork 0
Added orchestration workflow support #7
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
Conversation
Greptile Summary
Important Files Changed
Confidence score: 3/5
Sequence DiagramsequenceDiagram
participant User
participant OrchestrationHome
participant PlanBrowser
participant PlanDetail
participant OrchestrationAPI
participant RunDetail
participant StepCompletionModal
User->>OrchestrationHome: "Visit orchestration page"
OrchestrationHome->>PlanBrowser: "Browse plans"
PlanBrowser->>OrchestrationAPI: "Query plans"
OrchestrationAPI-->>PlanBrowser: "Return available plans"
PlanBrowser-->>User: "Display plan grid"
User->>PlanDetail: "Select plan"
PlanDetail->>OrchestrationAPI: "Get plan details"
OrchestrationAPI-->>PlanDetail: "Return plan with steps"
PlanDetail-->>User: "Show workflow visualizer"
User->>PlanDetail: "Start run"
PlanDetail->>OrchestrationAPI: "Start run request"
OrchestrationAPI-->>PlanDetail: "Return new run"
PlanDetail->>RunDetail: "Navigate to run"
RunDetail->>OrchestrationAPI: "Get run status"
OrchestrationAPI-->>RunDetail: "Return run with step states"
RunDetail-->>User: "Display workflow progress"
User->>RunDetail: "Click manual step"
RunDetail->>StepCompletionModal: "Open completion modal"
User->>StepCompletionModal: "Complete step with note"
StepCompletionModal->>OrchestrationAPI: "Complete step request"
OrchestrationAPI-->>StepCompletionModal: "Step completed"
StepCompletionModal->>RunDetail: "Reload run data"
RunDetail-->>User: "Show updated progress"
|
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.
35 files reviewed, 26 comments
| } | ||
|
|
||
| export function ManualStepNode({ data }: NodeProps) { | ||
| const { step, status, statusColor, onClick } = data as unknown as ManualStepNodeData; |
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.
style: Type assertion bypasses type checking which could hide type mismatches
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/components/workflow-nodes/ManualStepNode.tsx
Line: 15:15
Comment:
**style:** Type assertion bypasses type checking which could hide type mismatches
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.| if (index % 2 === 1) { | ||
| return ( | ||
| <code | ||
| key={`code-${index}`} | ||
| className="rounded bg-gray-100 px-1 py-0.5 font-mono text-[12px] text-gray-700" | ||
| > | ||
| {part} | ||
| </code> | ||
| ); |
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.
logic: Empty code blocks will be rendered when consecutive backticks appear in the input (e.g., 'test``code' creates an empty code element). Consider filtering out empty segments.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/components/MarkdownText.tsx
Line: 28:36
Comment:
**logic:** Empty code blocks will be rendered when consecutive backticks appear in the input (e.g., 'test``code' creates an empty code element). Consider filtering out empty segments.
How can I resolve this? If you propose a fix, please make it concise.| <h3 className="text-lg font-semibold text-slate-900 mb-2">Error Loading Plans</h3> | ||
| <p className="text-sm text-slate-600 mb-4">{error}</p> | ||
| <button | ||
| onClick={() => window.location.reload()} |
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.
style: Using window.location.reload() causes a full page reload instead of retrying just the data fetch. Would it be better to accept a retry callback prop instead of forcing a full page reload?
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/components/PlanGrid.tsx
Line: 55:55
Comment:
**style:** Using `window.location.reload()` causes a full page reload instead of retrying just the data fetch. Would it be better to accept a retry callback prop instead of forcing a full page reload?
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.
app/lib/types.ts
Outdated
| description: string; | ||
| type: string; | ||
| dependsOn?: string[]; | ||
| metadata: Record<string, any>; |
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.
style: Consider using Record<string, unknown> instead of Record<string, any> for better type safety
| metadata: Record<string, any>; | |
| metadata: Record<string, unknown>; |
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/lib/types.ts
Line: 343:343
Comment:
**style:** Consider using `Record<string, unknown>` instead of `Record<string, any>` for better type safety
```suggestion
metadata: Record<string, unknown>;
```
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.
app/lib/types.ts
Outdated
| updatedAt: string; | ||
| actor?: string; | ||
| note?: string; | ||
| metadata: Record<string, any>; |
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.
style: Consider using Record<string, unknown> instead of Record<string, any> for better type safety
| metadata: Record<string, any>; | |
| metadata: Record<string, unknown>; |
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/lib/types.ts
Line: 354:354
Comment:
**style:** Consider using `Record<string, unknown>` instead of `Record<string, any>` for better type safety
```suggestion
metadata: Record<string, unknown>;
```
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const getRunStepStates = (currentRun: OrchestrationRun): OrchestrationStepState[] => { | ||
| const directStates = currentRun.stepStates; | ||
| const fallbackStates = (currentRun as unknown as { steps?: OrchestrationStepState[] }).steps; |
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.
logic: unsafe type assertion could cause runtime issues if the object structure doesn't match. Is there a safer way to handle the fallback without using as unknown?
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/components/RunDetail.tsx
Line: 55:55
Comment:
**logic:** unsafe type assertion could cause runtime issues if the object structure doesn't match. Is there a safer way to handle the fallback without using `as unknown`?
How can I resolve this? If you propose a fix, please make it concise.| if (runStepStates.length === 0) return { completed: 0, total: 0, percentage: 0 }; | ||
|
|
||
| const total = runStepStates.length; | ||
| const completed = runStepStates.filter(step => step.status === 'succeeded').length; |
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.
logic: progress calculation uses raw status instead of normalized status from normalizeStepStatus function
| const completed = runStepStates.filter(step => step.status === 'succeeded').length; | |
| const completed = runStepStates.filter(step => normalizeStepStatus(step.status) === 'succeeded').length; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/components/RunDetail.tsx
Line: 131:131
Comment:
**logic:** progress calculation uses raw status instead of normalized status from normalizeStepStatus function
```suggestion
const completed = runStepStates.filter(step => normalizeStepStatus(step.status) === 'succeeded').length;
```
How can I resolve this? If you propose a fix, please make it concise.| <StepCompletionModal | ||
| runId={run.id} | ||
| stepId={selectedStepId} | ||
| step={plan.steps.find(s => s.id === selectedStepId)!} |
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.
logic: non-null assertion operator could throw if step is not found despite the conditional check
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/components/RunDetail.tsx
Line: 348:348
Comment:
**logic:** non-null assertion operator could throw if step is not found despite the conditional check
How can I resolve this? If you propose a fix, please make it concise.
app/lib/workflow.ts
Outdated
| const outgoing = new Map<string, string[]>(); | ||
| steps.forEach(step => { | ||
| (step.dependsOn || []).forEach(depId => { | ||
| if (!outgoing.has(depId)) { | ||
| outgoing.set(depId, []); | ||
| } | ||
| outgoing.get(depId)!.push(step.id); | ||
| }); | ||
| }); |
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.
style: duplicates logic from getOutgoingTargets function (lines 188-201) - consider reusing the existing function or extracting common logic
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/lib/workflow.ts
Line: 353:361
Comment:
**style:** duplicates logic from getOutgoingTargets function (lines 188-201) - consider reusing the existing function or extracting common logic
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.| useLayoutEffect(() => { | ||
| const container = containerRef.current; | ||
| const viewport = viewportRef.current; | ||
| if (!container || !viewport) return; | ||
|
|
||
| const updateColumns = () => { | ||
| const width = viewport.getBoundingClientRect().width; | ||
| const minCardWidth = 220; | ||
| const gap = 24; | ||
| const maxColumns = Math.max(1, Math.floor((width + gap) / (minCardWidth + gap))); | ||
| const nextColumns = Math.max(3, Math.min(maxParallel, maxColumns)); | ||
| setColumnCount(nextColumns); | ||
| }; | ||
|
|
||
| const computeEdges = () => { | ||
| const containerRect = container.getBoundingClientRect(); | ||
| const nodeRects = new Map<string, { left: number; right: number; top: number; bottom: number }>(); | ||
|
|
||
| cardRefs.current.forEach((node, stepId) => { | ||
| const rect = node.getBoundingClientRect(); | ||
| nodeRects.set(stepId, { | ||
| left: rect.left - containerRect.left, | ||
| right: rect.right - containerRect.left, | ||
| top: rect.top - containerRect.top, | ||
| bottom: rect.bottom - containerRect.top, | ||
| }); | ||
| }); | ||
|
|
||
| const statusById = new Map( | ||
| (stepStates || []).map(state => [state.stepId, normalizeStepStatus(state.status)]) | ||
| ); | ||
|
|
||
| const paths = computeEdgePaths({ | ||
| steps, | ||
| stepById, | ||
| nodeRects, | ||
| statusById, | ||
| edgeCounts, | ||
| outgoingTargets, | ||
| showStatus, | ||
| highlightedEdges, | ||
| hoveredStepId, | ||
| }); | ||
|
|
||
| setEdgePaths(paths); | ||
| setSvgSize({ width: container.scrollWidth, height: container.scrollHeight }); | ||
| }; | ||
|
|
||
| updateColumns(); | ||
| computeEdges(); | ||
| const resizeObserver = new ResizeObserver(() => { | ||
| updateColumns(); | ||
| computeEdges(); | ||
| }); | ||
| resizeObserver.observe(viewport); | ||
| window.addEventListener('resize', computeEdges); | ||
|
|
||
| return () => { | ||
| resizeObserver.disconnect(); | ||
| window.removeEventListener('resize', computeEdges); | ||
| }; | ||
| }, [steps, orderedLevels, stepStates, showStatus, hoveredStepId, highlightedEdges, edgeCounts, outgoingTargets, stepById, maxParallel]); |
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.
style: expensive DOM operations run on every dependency change including hover state, which could cause performance issues with large workflows. Have you considered throttling or debouncing the edge computation, especially for hover state changes?
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/components/WorkflowVisualizer.tsx
Line: 168:229
Comment:
**style:** expensive DOM operations run on every dependency change including hover state, which could cause performance issues with large workflows. Have you considered throttling or debouncing the edge computation, especially for hover state changes?
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.Introduced the Orchestration feature to the console, enabling users to create, manage, and visualize operation workflows. - added workflow visualization and state management. - added Orchestration navigation item to AppShell and a dashboard card to the home page. - defined comprehensive types for Plans, Runs, Steps, and their execution states. - created orchestration home and scaffolding for workflow management (PlanBrowser, visualizers, etc).
d673288 to
7c43d87
Compare
Added orchestration workflow support
Introduced the Orchestration feature to the console, enabling users to create, manage, and visualize operation workflows.