Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR adds a user interface for creating workflow runs directly from the dashboard. It introduces a modal dialog with a form that allows users to trigger new workflow runs by providing workflow name, optional input JSON, scheduling parameters, and version information. The implementation includes both client-side form handling and a new server-side API endpoint.
Changes:
- Added a new Dialog UI component library based on Base UI primitives
- Created a CreateRunForm component with validation and error handling
- Added a createWorkflowRunServerFn API endpoint for workflow run creation
- Enhanced RunList component with optional header and count display controls
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/dashboard/src/components/ui/dialog.tsx | New reusable Dialog component library with customizable size and styling |
| packages/dashboard/src/components/create-run-form.tsx | Form component for creating workflow runs with input validation and error handling |
| packages/dashboard/src/lib/api.ts | Added createWorkflowRunServerFn endpoint and supporting validation functions |
| packages/dashboard/src/components/run-list.tsx | Added showHeader and showCount props for flexible display control |
| packages/dashboard/src/routes/index.tsx | Integrated create run dialog into homepage with trigger button |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onSuccess?.(); | ||
| await navigate({ | ||
| to: "/runs/$runId", | ||
| params: { runId: run.id }, | ||
| }); |
There was a problem hiding this comment.
After successfully creating a workflow run, the form navigates to the run detail page, but the homepage data won't be immediately refreshed. While the usePolling hook will eventually update the list (every 2 seconds), consider calling router.invalidate() before navigation to ensure the homepage shows the new run when users navigate back. This would provide a better user experience.
| <Field> | ||
| <FieldLabel htmlFor="create-run-deadline-at">Deadline</FieldLabel> | ||
| <Input | ||
| id="create-run-deadline-at" | ||
| name="deadlineAt" | ||
| type="datetime-local" | ||
| value={deadlineAt} | ||
| onChange={(event) => { | ||
| setDeadlineAt(event.currentTarget.value); | ||
| }} | ||
| disabled={isSubmitting} | ||
| /> | ||
| <FieldDescription>Leave empty for no deadline.</FieldDescription> | ||
| </Field> |
There was a problem hiding this comment.
The datetime-local input type is used for availableAt and deadlineAt fields, but there's no validation to ensure that deadlineAt is after availableAt if both are provided. Consider adding this logical validation to prevent users from setting a deadline before the scheduled start time, which would be an invalid configuration.
| export function CreateRunForm({ onCancel, onSuccess }: CreateRunFormProps) { | ||
| const navigate = useNavigate(); | ||
|
|
||
| const [workflowName, setWorkflowName] = useState(""); | ||
| const [version, setVersion] = useState(""); | ||
| const [input, setInput] = useState(""); | ||
| const [availableAt, setAvailableAt] = useState(""); | ||
| const [deadlineAt, setDeadlineAt] = useState(""); | ||
| const [inputError, setInputError] = useState<string | null>(null); | ||
| const [submitError, setSubmitError] = useState<string | null>(null); | ||
| const [isSubmitting, setIsSubmitting] = useState(false); | ||
|
|
||
| async function submitForm() { | ||
| if (!workflowName.trim()) { | ||
| setSubmitError("Workflow name is required"); | ||
| return; | ||
| } | ||
|
|
||
| const normalizedAvailableAt = normalizeOptionalField(availableAt); | ||
| const normalizedDeadlineAt = normalizeOptionalField(deadlineAt); | ||
| let availableAtIso: string | null = null; | ||
| let deadlineAtIso: string | null = null; | ||
|
|
||
| try { | ||
| if (normalizedAvailableAt) { | ||
| availableAtIso = toIsoDateTime(normalizedAvailableAt, "Schedule for"); | ||
| } | ||
| if (normalizedDeadlineAt) { | ||
| deadlineAtIso = toIsoDateTime(normalizedDeadlineAt, "Deadline"); | ||
| } | ||
| } catch (error) { | ||
| setSubmitError(getErrorMessage(error)); | ||
| return; | ||
| } | ||
|
|
||
| const normalizedInput = normalizeOptionalField(input); | ||
| if (normalizedInput) { | ||
| try { | ||
| JSON.parse(normalizedInput); | ||
| } catch { | ||
| setInputError("Input must be valid JSON"); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| setInputError(null); | ||
| setSubmitError(null); | ||
| setIsSubmitting(true); | ||
|
|
||
| try { | ||
| const run = await createWorkflowRunServerFn({ | ||
| data: { | ||
| workflowName: workflowName.trim(), | ||
| version: normalizeOptionalField(version), | ||
| input: normalizedInput, | ||
| availableAt: availableAtIso, | ||
| deadlineAt: deadlineAtIso, | ||
| }, | ||
| }); | ||
|
|
||
| onSuccess?.(); | ||
| await navigate({ | ||
| to: "/runs/$runId", | ||
| params: { runId: run.id }, | ||
| }); | ||
| } catch (error) { | ||
| setSubmitError(getErrorMessage(error)); | ||
| } finally { | ||
| setIsSubmitting(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
When the dialog closes via onSuccess or onCancel, the form state (workflowName, version, input, etc.) is not reset. This means if a user opens the dialog again after creating a run, the form will still contain the previous values. Consider resetting the form state when the dialog closes or when it opens to provide a clean form experience.
| } catch { | ||
| setInputError("Input must be valid JSON"); |
There was a problem hiding this comment.
The JSON.parse call at line 80 is wrapped in a try-catch, but the error is caught and a generic message "Input must be valid JSON" is set. Consider providing more specific error feedback that includes the parsing error details (e.g., the specific JSON syntax error) to help users fix their input more easily. For example: "Input must be valid JSON: unexpected token at position X".
| } catch { | |
| setInputError("Input must be valid JSON"); | |
| } catch (error) { | |
| const baseMessage = "Input must be valid JSON"; | |
| if (error instanceof SyntaxError && error.message) { | |
| setInputError(`${baseMessage}: ${error.message}`); | |
| } else if (error instanceof Error && error.message) { | |
| setInputError(`${baseMessage}: ${error.message}`); | |
| } else { | |
| setInputError(baseMessage); | |
| } |
| if (normalizedInput) { | ||
| try { | ||
| JSON.parse(normalizedInput); | ||
| } catch { | ||
| setInputError("Input must be valid JSON"); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate JSON validation logic exists in both the form and the API handler. The JSON parsing is performed here in the form and again in the API handler at line 122-126 of packages/dashboard/src/lib/api.ts. Consider removing this client-side validation and relying on the server-side validation, or ensure they remain synchronized if both are necessary for UX reasons.
| if (normalizedInput) { | |
| try { | |
| JSON.parse(normalizedInput); | |
| } catch { | |
| setInputError("Input must be valid JSON"); | |
| return; | |
| } | |
| } |
| function parseOptionalDate( | ||
| value: string | null | undefined, | ||
| fieldName: string, | ||
| ): Date | null { | ||
| if (!value) { | ||
| return null; | ||
| } | ||
|
|
||
| const parsedDate = new Date(value); | ||
| if (Number.isNaN(parsedDate.getTime())) { | ||
| throw new TypeError(`${fieldName} must be a valid date and time`); | ||
| } | ||
|
|
||
| return parsedDate; | ||
| } |
There was a problem hiding this comment.
Duplicate date validation logic exists in both the form (toIsoDateTime at line 25-31 of packages/dashboard/src/components/create-run-form.tsx) and the API handler (parseOptionalDate). Both functions perform the same validation with similar error messages. Consider consolidating this validation to avoid duplication and potential inconsistencies.
1 participant
No description provided.