feat(platform): manual automation step editing#341
Conversation
Add wfStepAuditLogs table and audit helper to track changes to workflow step definitions. Captures change type, edit mode, and before/after state.
Add circular dependency validator using DFS with cycle detection. Include dry run executor for workflow execution testing.
Add createStepPublic and updateStepPublic mutations with RLS protection. Include validateStep query for real-time step configuration validation.
Add visual step editor in automation sidepanel with JSON editing support. Include NextStepsEditor for configuring step connections. Add hooks for step creation, updates, and validation. Improve automation tester with better test scenario configuration.
Enhance Select component with icon support in options. Improve JsonInput with better string collapsing. Minor formatting improvements in chat components.
📝 WalkthroughWalkthroughThis PR introduces a comprehensive workflow step editing and validation system. It adds a new database table for auditing workflow step changes, implements public mutations for creating and updating workflow steps with RLS protection and audit logging, and creates server-side validation and dry-run execution capabilities. On the frontend, it replaces read-only JSON viewers with editable JSON inputs, adds a new next-steps transition editor, and implements a validation display in the automation sidepanel. The dry-run executor simulates workflow execution to visualize step execution paths. Supporting infrastructure includes circular dependency detection, new permission resources for audit logs, and localization strings for new UI elements. Minor UI refinements to chat components add mobile-responsive header controls. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
services/platform/app/components/error-boundaries/displays/global-error-display.tsx (1)
92-94: 🧹 Nitpick | 🔵 TrivialRemove unused
logoSrcvariable.The
logoSrcvariable is computed but never used in the component's JSX. This appears to be leftover code from the removed logo header.🧹 Proposed fix to remove dead code
export function GlobalErrorDisplay({ error, reset }: GlobalErrorDisplayProps) { const router = useRouter(); const [showError, setShowError] = useState(false); const isDarkMode = useIsDarkMode(); - const logoSrc = isDarkMode - ? '/assets/logo-white.svg' - : '/assets/logo-black.svg'; const textColor = isDarkMode ? '#f3f4f6' : '#111827'; const mutedColor = isDarkMode ? '#9ca3af' : '#6b7280';services/platform/app/features/automations/components/step-create-dialog.tsx (2)
102-176:⚠️ Potential issue | 🟠 MajorReset/normalize
nextStepswhenstepTypechanges to avoid submitting stale transition keys.
NextStepsEditor hides non-applicable keys but the state persists across step-type changes, so stale transitions can be sent inonCreateStep.🛠️ Proposed fix (reset on step type change)
useEffect(() => { const defaults = getDefaultTemplates(stepType); setValue('config', defaults.config); + setNextSteps({}); }, [stepType, setValue]);
160-180:⚠️ Potential issue | 🟡 MinorSurface create-step failures to users.
The catch block only logs to console; add a destructive toast so the UI reflects failure.Based on learnings: "Use the required error handling pattern for async UI actions: try/catch with toast on error and result.error checks".💬 Suggested toast on failure
} catch (error) { console.error('Failed to create step:', error); + toast({ + title: t('createStep.createFailed'), + variant: 'destructive', + }); }
🤖 Fix all issues with AI agents
In @.gitignore:
- Around line 73-75: The .gitignore currently adds a specific entry for
services/operator/Dockerfile.arm64; keep this if you intend to only ignore the
operator's ARM64 Dockerfile, or change to a broader pattern such as
**/Dockerfile.arm64 or **/*.arm64 if you want to ignore ARM64 Dockerfiles across
all services; update the .gitignore by replacing or adding the chosen pattern
(services/operator/Dockerfile.arm64 vs **/Dockerfile.arm64 or **/*.arm64)
accordingly.
In `@services/platform/app/features/automations/components/automation-step.tsx`:
- Around line 63-103: getActionIcon is duplicated (also present in
next-steps-editor.tsx) and is currently declared inside the component causing
re-creation on every render; extract this logic into a shared utility function
(e.g., export a getActionIcon helper) and replace the inline definition in
automation-step.tsx with an import, or at minimum move the getActionIcon
declaration outside the component body so it’s not redefined per render; update
both automation-step.tsx and next-steps-editor.tsx to import/use the single
shared getActionIcon to remove duplication.
In `@services/platform/app/features/automations/components/automation-tester.tsx`:
- Around line 61-94: The render currently updates state directly when the
dry-run query finishes (using isDryRunning, dryRunQuery, setDryRunResult,
setIsDryRunning) which causes React warnings; move that logic into a useEffect
that depends on isDryRunning and dryRunQuery (and
setDryRunResult/setIsDryRunning) so state updates run in an effect, and add
useEffect to the imports; leave parsedInput and handleDryRun as-is but ensure
handleDryRun only sets isDryRunning and clears dryRunResult so the effect
handles storing the dryRunQuery result and clearing the running flag.
In `@services/platform/app/features/automations/hooks/use-step-validation.ts`:
- Around line 65-70: The current return sets isValid to validationResult?.valid
?? true which treats an undefined validationResult (while loading) as valid and
can enable actions prematurely; update the logic in use-step-validation
(referencing validationResult, isValid, and isValidating) so that when
isValidating is true isValid defaults to false (e.g., isValid = isValidating ?
false : (validationResult?.valid ?? false)) or otherwise require consumers to
use !isValidating && isValid before enabling saves—implement the former change
in the return object to prevent transient enabled state.
In `@services/platform/app/features/chat/components/chat-header.tsx`:
- Line 65: Extract the hardcoded "767px" used in chat-header.tsx into a named
constant (e.g., MOBILE_BREAKPOINT = 767) and replace the inline string in the
isMobile calculation with a template that uses that constant (for example
window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT}px)`).matches); either define
and export MOBILE_BREAKPOINT from a shared breakpoints/theme module and import
it into chat-header.tsx, or add it to an existing constants file so other
components (and Tailwind breakpoint changes) stay in sync; update any references
to the old literal in chat-header.tsx to use the constant.
In `@services/platform/convex/wf_definitions/queries.ts`:
- Around line 174-202: Add a new internal counterpart dryRunWorkflowInternal
using internalQuery that mirrors dryRunWorkflow's signature and logic but uses
internalQuery instead of queryWithRLS; implement it to fetch the workflow (using
ctx.db.get(args.wfDefinitionId)), load wfStepDefs via the same by_definition
index into a steps array, and then delegate to executeDryRun(workflow, steps,
args.input || {}), returning the same result shape so callers can use the
internal query for privileged access while preserving existing behavior.
- Line 200: The return call to executeDryRun currently uses args.input || {}
which treats legitimate falsy JsonValue inputs (false, 0, "", null) as absent;
change this to use the nullish coalescing operator so only undefined becomes the
default: pass args.input ?? {} into executeDryRun (locate the call that returns
executeDryRun(workflow, steps, args.input || {})). Ensure you update the
argument there so falsy valid inputs are preserved.
In `@services/platform/convex/wf_step_defs/mutations.ts`:
- Around line 24-75: The audit currently uses client-supplied args.changedBy (in
updateStepPublic) which can be spoofed; replace that with the server-derived
authenticated user ID from the request context (e.g., use ctx.auth.userId or the
canonical user id on ctx) and pass that value to auditStepChange instead of
args.changedBy; update any other mutations in this file (the other handlers
mentioned around the 77-137 region) that call auditStepChange to use the
server-side user id rather than client-supplied fields.
In `@services/platform/convex/wf_step_defs/queries.ts`:
- Around line 40-49: The validateStep query (created via queryWithRLS) lacks an
explicit returns validator; add a returns field to its args object by specifying
the expected return type validator (e.g., v.object({...}) or v.optional(v.any())
matching the function's actual returned shape) to follow Convex best practices.
Update the validateStep call to include returns: <appropriate validator> so the
runtime and generated types know what validateStep returns, ensuring consistency
with the logic inside validateStep and any referenced types used in the query.
In `@services/platform/convex/workflow_engine/execution/dry_run_executor.ts`:
- Around line 170-188: The code silently treats a missing branch mapping as an
end by using currentStep.nextSteps[branch] || null; update the logic in
dry_run_executor.ts around simulateStepOutput/currentStep to detect when branch
is not a key in currentStep.nextSteps and surface that as a warning/error
instead of silently ending: when branch is missing, add a warning entry (or set
a warning property) into the stepResults object pushed by stepResults.push
(include stepSlug, branch and a descriptive message), and then break;
alternatively call the existing logger (e.g., processLogger.warn) with the same
details before breaking so misconfigured steps are visible in dry-run output.
Ensure nextStep remains null when missing to preserve behavior but that the
missing-branch condition is explicitly reported.
In
`@services/platform/convex/workflow_engine/helpers/validation/validate_workflow_definition.ts`:
- Line 18: The circular-dependency check can throw if steps contain
nulls/primitives, so in validateWorkflowDefinition (before calling
validateCircularDependencies) filter or guard the steps array to only pass plain
objects representing steps; e.g., build a filteredSteps = steps.filter(s => s &&
typeof s === 'object') and call validateCircularDependencies(filteredSteps)
instead of the raw steps, and apply the same guard at the other invocation site
in the function (the second call around the 133-141 area) so malformed steps
produce validation errors rather than runtime exceptions.
Replace error.message with translated messages in toasts across the codebase. Raw error messages can be verbose, untranslated, and expose internal details. Errors are now logged to console for debugging while users see clean translated messages. Also rename availableSteps to stepOptions for clarity.
- Extract duplicated icon utilities to shared step-icons.tsx - Fix isValid defaulting to true during validation (now false) - Use nullish coalescing (??) instead of || for input handling - Use server-authenticated user for audit logging instead of client-supplied changedBy - Guard steps array before circular dependency validation
Summary
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores