Open
Conversation
✅ Deploy preview added
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted. Your PR title is: Requirements:
Expected format: Details: Available types:
|
Contributor
WalkthroughThis pull request extends AI-powered workflow generation with new step manipulation capabilities. It introduces three new workflow tools: ADD_STEP_IN_BETWEEN, UPDATE_STEP_CONDITIONS, and MOVE_STEP. The changes refactor prompt and schema handling to use explicit variable schemas instead of deriving them from draft workflow state. Supporting infrastructure is updated, including condition schema restructuring, new input/output schemas for step operations, and modular helper functions. Environment tag integration is added to the workflow generation flow. Dashboard components are updated to render the new tool types in the chat interface. The AI provider in workflow editing is made optional based on feature flags. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
apps/dashboard/src/components/workflow-editor/workflow-tabs.tsx (1)
468-471: AlignAiChatProvidermounting with AI UI visibility.At Line 489, the provider mounts when the feature flag is on, but AI consumers are still hidden outside dev (Lines 468 and 471). Consider gating provider mount with the same condition to avoid unnecessary chat initialization/fetching in non-dev contexts.
Suggested refactor
- return isAiWorkflowGenerationEnabled ? <AiChatProvider config={aiChatConfig}>{content}</AiChatProvider> : content; + const isAiSidekickEnabled = isAiWorkflowGenerationEnabled && isDevEnvironment; + + return isAiSidekickEnabled ? <AiChatProvider config={aiChatConfig}>{content}</AiChatProvider> : content;Also applies to: 489-489
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/workflow-editor/workflow-tabs.tsx` around lines 468 - 471, The AiChatProvider is mounted unconditionally while AI UI components are only rendered when isAiWorkflowGenerationEnabled && isDevEnvironment; update the mount so AiChatProvider is only instantiated under the same condition to avoid unnecessary chat initialization and fetching — wrap or gate AiChatProvider with the same boolean check used for AiSidekickPanel and WorkflowCanvasToast (referencing AiChatProvider, AiSidekickPanel, WorkflowCanvasToast, isAiWorkflowGenerationEnabled, isDevEnvironment) so provider and consumers mount/unmount together.apps/api/src/app/ai/utils/variable-schema.utils.ts (1)
330-344: Consider the implications of emptystepIdvalues.When
stepIdis undefined, it defaults to an empty string. This could cause issues if the step is later used inbuildPreviousStepsSchema(line 51-72), wherestepIdbecomes an object property key. Multiple steps without IDs would collide on the empty string key.If steps without IDs shouldn't contribute to
previousStepsschema, consider filtering them out or generating a temporary ID:♻️ Suggested approach
export function toGeneratedSteps( steps: Array<{ stepId?: string; name: string; type: StepTypeEnum; controlValues?: Record<string, unknown> | null; }> ): GeneratedStep[] { - return steps.map((s) => ({ - stepId: s.stepId ?? '', + return steps.map((s, index) => ({ + stepId: s.stepId ?? `step-${index}`, name: s.name, type: s.type, controlValues: (s.controlValues ?? {}) as Record<string, unknown>, })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/ai/utils/variable-schema.utils.ts` around lines 330 - 344, toGeneratedSteps currently maps undefined stepId to an empty string which will collide when used as object keys in buildPreviousStepsSchema; update toGeneratedSteps to either (A) filter out steps with no stepId before mapping so they don't become keys in buildPreviousStepsSchema, or (B) assign a stable unique temporary id for missing stepId (e.g., based on index or a generated UUID) instead of an empty string; adjust any consumers if you choose B to be aware of temp ids. Ensure you modify the toGeneratedSteps implementation (and, if needed, update buildPreviousStepsSchema expectations) rather than leaving empty string defaults.apps/api/src/app/ai/tools/workflow-generation.tools.ts (5)
167-173: Consider extracting repeated step mapping pattern.The same step-to-minimal-object transformation is repeated in
updateWorkflow,removeWorkflowStep, andmoveWorkflowStep. Extracting a helper function would reduce duplication and ensure consistency.♻️ Example helper extraction
const toMinimalSteps = (steps: WorkflowResponseDto['steps']) => steps.map((s) => ({ _id: s._id, stepId: s.stepId, name: s.name, type: s.type, controlValues: s.controlValues ?? {}, }));Also applies to: 367-375, 507-513
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/ai/tools/workflow-generation.tools.ts` around lines 167 - 173, Extract the repeated step-mapping into a single helper (e.g., toMinimalSteps) and replace the inline mapping in updateWorkflow, removeWorkflowStep, and moveWorkflowStep with calls to that helper; the helper should accept WorkflowResponseDto['steps'] (or the same steps type used across those functions) and return the minimal step objects with _id, stepId, name, type, and controlValues defaulted to {} to ensure consistency and remove duplication across the mappings currently at lines around the mapping in each function.
470-533: Missing error handling and silent index clamping inmoveWorkflowStep.Two observations:
- This function lacks try-catch around the upsert call, unlike other step manipulation helpers.
- Line 501 silently clamps
toIndexwithout logging. If the caller provides an out-of-bounds index, this could mask bugs or produce unexpected behavior.Consider adding error handling and a warning log when the index is clamped.
♻️ Proposed refactor
const clampedIndex = Math.max(0, Math.min(toIndex, latestWorkflow.steps.length - 1)); + if (clampedIndex !== toIndex) { + logger.warn({ stepId, toIndex, clampedIndex }, 'Step move index clamped to valid range'); + } const steps = [...latestWorkflow.steps]; const [step] = steps.splice(fromIndex, 1); steps.splice(clampedIndex, 0, step); const reorderedSteps = steps.map((s) => ({ _id: s._id, stepId: s.stepId, name: s.name, type: s.type, controlValues: s.controlValues ?? {}, })); + try { const persistedWorkflow = await upsertWorkflowUseCase.execute( // ... existing code ); draftState.setWorkflow(persistedWorkflow); // ... logging return persistedWorkflow; + } catch (error) { + logger.error({ error }, 'Failed to move workflow step'); + + throw error; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/ai/tools/workflow-generation.tools.ts` around lines 470 - 533, The moveWorkflowStep function needs explicit error handling and visibility when requested index is out of bounds: wrap the upsertWorkflowUseCase.execute (and subsequent draftState.setWorkflow and logger.info) in a try-catch, log an error via logger.error including workflowId/_id, stepId and the caught error, and rethrow (or throw a new Error with context) so callers can handle failures; additionally, detect when clampedIndex !== toIndex and emit a logger.warn (including stepId, requested toIndex, clampedIndex and workflowId) before mutating steps so out-of-range requests are visible; update references: moveWorkflowStep, upsertWorkflowUseCase.execute, draftState.setWorkflow, and logger.info to reflect this change.
740-796: Code duplication betweenaddStepToolandaddStepInBetweenTool.Lines 741-772 are nearly identical to lines 684-715 in
addStepTool. The only difference is the insertion method (addWorkflowStepvsaddWorkflowStepInBetween). Consider extracting the common step generation logic to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/ai/tools/workflow-generation.tools.ts` around lines 740 - 796, Extract the duplicated step-generation logic from addStepTool and addStepInBetweenTool into a shared async helper (e.g., generateAndPrepareStep) that accepts parameters: input, draftState, llmService, stepTypeToSchemaAndPrompt, and any context needed; inside it perform the stepConfig lookup (stepTypeToSchemaAndPrompt[input.stepType]), buildFullVariableSchema(using toGeneratedSteps), call llmService.generateObject with buildStepSystemPrompt and buildStepUserPrompt, apply the StepTypeEnum.EMAIL block-editor JSON conversion, and apply the input.skip controlValues merge; then replace the duplicated blocks in addStepTool and addStepInBetweenTool to call this helper and pass the resulting newStep into addWorkflowStep or addWorkflowStepInBetween respectively, ensuring runtime.writer and logger calls remain in the original tools.
343-396: Missing error handling inremoveWorkflowStep.Unlike
addWorkflowStep,updateWorkflowStep, andaddWorkflowStepInBetween, this function lacks a try-catch block around theupsertWorkflowUseCase.executecall. For consistency and better error diagnostics, consider wrapping the upsert in try-catch and logging failures.♻️ Proposed refactor
const { payloadSchema, validatePayload } = computePayloadSchemaFromSteps(steps); + try { const persistedWorkflow = await upsertWorkflowUseCase.execute( UpsertWorkflowCommand.create({ workflowDto: { ...latestWorkflow, steps, validatePayload, payloadSchema: validatePayload ? payloadSchema : undefined, }, user: command.user, workflowIdOrInternalId: workflowId, }) ); draftState.setWorkflow(persistedWorkflow); logger.info({ _id: persistedWorkflow._id, slug: persistedWorkflow.slug }, `AI Workflow step removed: ${stepId}`); return persistedWorkflow; + } catch (error) { + logger.error({ error }, 'Failed to remove workflow step'); + + throw error; + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/ai/tools/workflow-generation.tools.ts` around lines 343 - 396, Wrap the upsert step in removeWorkflowStep with a try-catch around the call to upsertWorkflowUseCase.execute (and subsequent draftState.setWorkflow/logger.info) to mirror other functions; in the catch, call logger.error and include identifying context (workflowId, stepId and any persistedWorkflow._id/slug if available) and the caught error details, then rethrow the error so callers still receive the failure. Ensure you reference removeWorkflowStep, upsertWorkflowUseCase.execute, draftState.setWorkflow and logger in the change.
70-76: Consider usinginterfaceinstead oftype.As per coding guidelines for backend code, prefer
interfaceovertypefor object shapes.♻️ Proposed refactor
-export type WorkflowMetadata = { +export interface WorkflowMetadata { description?: string | null; tags?: string[] | null; name: string; severity: SeverityLevelEnum; critical: boolean; -}; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/ai/tools/workflow-generation.tools.ts` around lines 70 - 76, Replace the exported type alias WorkflowMetadata with an exported interface named WorkflowMetadata to follow backend coding guidelines; update the declaration for WorkflowMetadata (keeping properties description?: string | null; tags?: string[] | null; name: string; severity: SeverityLevelEnum; critical: boolean;) as an interface, leaving all types and optional/null unions intact and ensuring any imports or references to SeverityLevelEnum remain unchanged so consumers of WorkflowMetadata continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard/src/components/ai-sidekick/chat-chain-of-thought.tsx`:
- Around line 353-375: The component is missing UI handling for
AiWorkflowToolsEnum.MOVE_STEP and AiWorkflowToolsEnum.REMOVE_STEP; add blocks
analogous to the existing ones (e.g., the ADD_STEP_IN_BETWEEN,
EDIT_STEP_CONTENT, UPDATE_STEP_CONDITIONS blocks) that check tool.toolName ===
AiWorkflowToolsEnum.MOVE_STEP and === AiWorkflowToolsEnum.REMOVE_STEP, use
corresponding parts arrays (e.g., moveStepParts and removeStepParts) filtered by
p.state === 'output-available' && p.output, push entries into toolItems like {
type: 'moveStep', steps: stepsSoFar } and { type: 'removeStep', steps:
stepsSoFar }, set flags (e.g., moveStepAdded, removeStepAdded) to avoid
duplicates, and update the toolItems union/type definition to include 'moveStep'
and 'removeStep' variants so the UI can render them.
---
Nitpick comments:
In `@apps/api/src/app/ai/tools/workflow-generation.tools.ts`:
- Around line 167-173: Extract the repeated step-mapping into a single helper
(e.g., toMinimalSteps) and replace the inline mapping in updateWorkflow,
removeWorkflowStep, and moveWorkflowStep with calls to that helper; the helper
should accept WorkflowResponseDto['steps'] (or the same steps type used across
those functions) and return the minimal step objects with _id, stepId, name,
type, and controlValues defaulted to {} to ensure consistency and remove
duplication across the mappings currently at lines around the mapping in each
function.
- Around line 470-533: The moveWorkflowStep function needs explicit error
handling and visibility when requested index is out of bounds: wrap the
upsertWorkflowUseCase.execute (and subsequent draftState.setWorkflow and
logger.info) in a try-catch, log an error via logger.error including
workflowId/_id, stepId and the caught error, and rethrow (or throw a new Error
with context) so callers can handle failures; additionally, detect when
clampedIndex !== toIndex and emit a logger.warn (including stepId, requested
toIndex, clampedIndex and workflowId) before mutating steps so out-of-range
requests are visible; update references: moveWorkflowStep,
upsertWorkflowUseCase.execute, draftState.setWorkflow, and logger.info to
reflect this change.
- Around line 740-796: Extract the duplicated step-generation logic from
addStepTool and addStepInBetweenTool into a shared async helper (e.g.,
generateAndPrepareStep) that accepts parameters: input, draftState, llmService,
stepTypeToSchemaAndPrompt, and any context needed; inside it perform the
stepConfig lookup (stepTypeToSchemaAndPrompt[input.stepType]),
buildFullVariableSchema(using toGeneratedSteps), call llmService.generateObject
with buildStepSystemPrompt and buildStepUserPrompt, apply the StepTypeEnum.EMAIL
block-editor JSON conversion, and apply the input.skip controlValues merge; then
replace the duplicated blocks in addStepTool and addStepInBetweenTool to call
this helper and pass the resulting newStep into addWorkflowStep or
addWorkflowStepInBetween respectively, ensuring runtime.writer and logger calls
remain in the original tools.
- Around line 343-396: Wrap the upsert step in removeWorkflowStep with a
try-catch around the call to upsertWorkflowUseCase.execute (and subsequent
draftState.setWorkflow/logger.info) to mirror other functions; in the catch,
call logger.error and include identifying context (workflowId, stepId and any
persistedWorkflow._id/slug if available) and the caught error details, then
rethrow the error so callers still receive the failure. Ensure you reference
removeWorkflowStep, upsertWorkflowUseCase.execute, draftState.setWorkflow and
logger in the change.
- Around line 70-76: Replace the exported type alias WorkflowMetadata with an
exported interface named WorkflowMetadata to follow backend coding guidelines;
update the declaration for WorkflowMetadata (keeping properties description?:
string | null; tags?: string[] | null; name: string; severity:
SeverityLevelEnum; critical: boolean;) as an interface, leaving all types and
optional/null unions intact and ensuring any imports or references to
SeverityLevelEnum remain unchanged so consumers of WorkflowMetadata continue to
work.
In `@apps/api/src/app/ai/utils/variable-schema.utils.ts`:
- Around line 330-344: toGeneratedSteps currently maps undefined stepId to an
empty string which will collide when used as object keys in
buildPreviousStepsSchema; update toGeneratedSteps to either (A) filter out steps
with no stepId before mapping so they don't become keys in
buildPreviousStepsSchema, or (B) assign a stable unique temporary id for missing
stepId (e.g., based on index or a generated UUID) instead of an empty string;
adjust any consumers if you choose B to be aware of temp ids. Ensure you modify
the toGeneratedSteps implementation (and, if needed, update
buildPreviousStepsSchema expectations) rather than leaving empty string
defaults.
In `@apps/dashboard/src/components/workflow-editor/workflow-tabs.tsx`:
- Around line 468-471: The AiChatProvider is mounted unconditionally while AI UI
components are only rendered when isAiWorkflowGenerationEnabled &&
isDevEnvironment; update the mount so AiChatProvider is only instantiated under
the same condition to avoid unnecessary chat initialization and fetching — wrap
or gate AiChatProvider with the same boolean check used for AiSidekickPanel and
WorkflowCanvasToast (referencing AiChatProvider, AiSidekickPanel,
WorkflowCanvasToast, isAiWorkflowGenerationEnabled, isDevEnvironment) so
provider and consumers mount/unmount together.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/api/src/app/ai/ai.module.tsapps/api/src/app/ai/prompts/step.prompt.tsapps/api/src/app/ai/prompts/workflow.prompt.tsapps/api/src/app/ai/schemas/steps-control.schema.tsapps/api/src/app/ai/tools/workflow-generation.tools.tsapps/api/src/app/ai/usecases/stream-workflow-generation/stream-workflow-generation.usecase.tsapps/api/src/app/ai/utils/variable-schema.utils.tsapps/dashboard/src/components/ai-sidekick/chat-body.tsxapps/dashboard/src/components/ai-sidekick/chat-chain-of-thought.tsxapps/dashboard/src/components/workflow-editor/workflow-tabs.tsxpackages/shared/src/types/ai.ts
Comment on lines
+353
to
+375
| if (tool.toolName === AiWorkflowToolsEnum.ADD_STEP_IN_BETWEEN && !addStepInBetweenAdded) { | ||
| const stepsSoFar = addStepInBetweenParts.filter((p) => p.state === 'output-available' && p.output); | ||
| if (stepsSoFar.length > 0) { | ||
| toolItems.push({ type: 'addStepInBetween', steps: stepsSoFar }); | ||
| addStepInBetweenAdded = true; | ||
| } | ||
| } | ||
|
|
||
| if (tool.toolName === AiWorkflowToolsEnum.EDIT_STEP_CONTENT && !editStepContentAdded) { | ||
| const stepsSoFar = editStepParts.filter((p) => p.state === 'output-available' && p.output); | ||
| if (stepsSoFar.length > 0) { | ||
| toolItems.push({ type: 'editStep', steps: stepsSoFar }); | ||
| editStepContentAdded = true; | ||
| } | ||
| } | ||
|
|
||
| if (tool.toolName === AiWorkflowToolsEnum.UPDATE_STEP_CONDITIONS && !updateStepConditionsAdded) { | ||
| const stepsSoFar = updateStepConditionsParts.filter((p) => p.state === 'output-available' && p.output); | ||
| if (stepsSoFar.length > 0) { | ||
| toolItems.push({ type: 'updateStepConditions', steps: stepsSoFar }); | ||
| updateStepConditionsAdded = true; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg "MOVE_STEP" --type ts -C 2Repository: novuhq/novu
Length of output: 2515
🏁 Script executed:
# Also search for the enum definition to understand the context
rg "AiWorkflowToolsEnum" --type ts -A 10 | head -50Repository: novuhq/novu
Length of output: 4446
🏁 Script executed:
# Check if there's any MOVE_STEP handling elsewhere in the chat-chain-of-thought file
cat -n apps/dashboard/src/components/ai-sidekick/chat-chain-of-thought.tsx | grep -i "move"Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
# Search for REMOVE_STEP in the chat-chain-of-thought component
rg "REMOVE_STEP|removeStep" apps/dashboard/src/components/ai-sidekick/chat-chain-of-thought.tsx -C 3Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
# Get a broader view of all tool type handling in the component
rg "tool\.toolName|type:" apps/dashboard/src/components/ai-sidekick/chat-chain-of-thought.tsx | head -30Repository: novuhq/novu
Length of output: 1453
🏁 Script executed:
# Check what tool types are handled in toolItems
rg "toolItems\.push|type:" apps/dashboard/src/components/ai-sidekick/chat-chain-of-thought.tsxRepository: novuhq/novu
Length of output: 1004
Add UI rendering for MOVE_STEP and REMOVE_STEP tools.
Both MOVE_STEP and REMOVE_STEP are defined in AiWorkflowToolsEnum and have backend implementations, but they lack corresponding UI handling in this component. The toolItems union type only includes handlers for workflowInit, addStep, addStepInBetween, editStep, and updateStepConditions. To maintain consistency with other workflow tools and provide visibility into AI-driven step operations, these tools should have similar UI rendering blocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/src/components/ai-sidekick/chat-chain-of-thought.tsx` around
lines 353 - 375, The component is missing UI handling for
AiWorkflowToolsEnum.MOVE_STEP and AiWorkflowToolsEnum.REMOVE_STEP; add blocks
analogous to the existing ones (e.g., the ADD_STEP_IN_BETWEEN,
EDIT_STEP_CONTENT, UPDATE_STEP_CONDITIONS blocks) that check tool.toolName ===
AiWorkflowToolsEnum.MOVE_STEP and === AiWorkflowToolsEnum.REMOVE_STEP, use
corresponding parts arrays (e.g., moveStepParts and removeStepParts) filtered by
p.state === 'output-available' && p.output, push entries into toolItems like {
type: 'moveStep', steps: stepsSoFar } and { type: 'removeStep', steps:
stepsSoFar }, set flags (e.g., moveStepAdded, removeStepAdded) to avoid
duplicates, and update the toolItems union/type definition to include 'moveStep'
and 'removeStep' variants so the UI can render them.
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
1 participant
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.
What changed? Why was the change needed?
Novu Sidekick:
Screenshots