refactor: migrate to frappe-react-sdk#1
Conversation
- Replace custom API layer with frappe-react-sdk hooks - Remove @/lib/request.ts and @/queries/frappe.ts - Update main.tsx to use FrappeProvider instead of QueryClientProvider - Update all components to use useFrappeGetDoc, useFrappeGetDocList, useFrappeCreateDoc, useFrappeUpdateDoc, and useFrappeDeleteDoc - Update __root.tsx to remove React Query dependencies - Simplify route loaders (data fetching now happens in components)
- Add node_id, position_x, position_y fields to Hazel Node doctype - Create new Hazel Node Connection doctype for storing edges - Supports source_handle for conditional branching (default/true/false) - Add connections child table to Hazel Workflow doctype - Implement graph-based workflow execution in HazelWorkflow.execute() - Supports conditional branching based on node result - Maintains backward compatibility with linear execution - Update TypeScript types for frontend
- Create ConditionNode handler with support for:
- Multiple comparison operators (equals, not_equals, gt, lt, contains, etc.)
- Variable resolution from context using {{variable}} syntax
- Nested property access with dot notation
- Add Condition node type to fixtures with parameters:
- left_operand: Value or variable to compare
- operator: Comparison operator
- right_operand: Value or variable to compare against
- Returns {branch: 'true'/'false'} for workflow graph execution
- Update editor utilities to handle node_id and connections from backend - Add ConditionNode component with dual output handles (Yes/No) - Update WorkflowNode to include node_id in data structure - Enable edge creation with onConnect handler - Add color-coded edges for conditional branches (green=Yes, red=No) - Update editor store with generateNodeId and proper node management - Update configPanel to save nodes and connections to backend - Add nodesToHazelNodes and edgesToHazelConnections utility functions - Support draggable nodes and fitView on load
- Add NodePalette component for dragging nodes to canvas - Implement drag-and-drop handlers in workflow editor - Add addNodeAtPosition method to editor store - Update layout with left-side node palette panel - Fix proxyOptions.ts to handle missing site config gracefully - Clean up configPanel by moving action buttons to palette
Frontend: - Create constants/editor.ts with shared edge styling, node types, and DragData - Extract reusable ParamForm component from configPanel - DRY up node creation logic in editor store with createFlowNode helper - Use shared constants across editor, node-palette, and utils - Fix missing React keys in SelectItem components Backend: - Create nodes/utils.py with parse_json_field and render_template_field - Refactor node handlers to use shared utilities - Add ensure_context helper for consistent context handling
📝 WalkthroughWalkthroughAdds graph-based workflow execution and editor support, new backend DocType for connections, multiple action nodes and utilities, extensive frontend React Flow integration (drag/drop, node palette, node/edge processing), migration from custom React Query to frappe-react-sdk, CI/runtime updates, and related type/schema changes. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Palette as NodePalette
participant UI as React Flow Editor
participant Store as Editor Store
User->>Palette: drag node
Palette->>UI: start drag (DRAG_DATA_TYPE)
UI->>UI: onDragOver -> compute drop coords
User->>UI: drop node
UI->>Store: addNodeAtPosition(nodeData, {x,y})
Store->>Store: generateNodeId(), create FlowNode
Store-->>UI: update flowNodes
UI->>UI: render new node
sequenceDiagram
participant HazelWorkflow as Backend
participant Executor as _execute_node
participant Node as Action/Condition Nodes
participant Context as Execution Context
HazelWorkflow->>HazelWorkflow: execute(context)
alt connections exist
HazelWorkflow->>HazelWorkflow: _execute_graph()
HazelWorkflow->>HazelWorkflow: get_start_node()
loop traverse graph
HazelWorkflow->>Executor: _execute_node(node, context)
Executor->>Node: execute(params, context)
Node-->>Executor: updated context / branch result
Executor-->>HazelWorkflow: return context
HazelWorkflow->>HazelWorkflow: _get_next_node_id(current, result)
end
else linear fallback
HazelWorkflow->>HazelWorkflow: _execute_linear()
loop sequential nodes
HazelWorkflow->>Executor: _execute_node(node, context)
Executor-->>HazelWorkflow: updated context
end
end
HazelWorkflow-->>Context: final context / logs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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. ✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/package.json (1)
26-27: Remove unused React Query dependencies.
@tanstack/react-queryand@tanstack/react-query-devtoolsare no longer used in the codebase after migrating to frappe-react-sdk. No direct imports, QueryClient usage, or React Query hooks remain in the frontend source code. These dependencies should be removed from package.json.
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 47-50: Update the GitHub Actions step that currently references
actions/setup-node@v3 to use actions/setup-node@v4 by replacing the string
"actions/setup-node@v3" with "actions/setup-node@v4" (preserving existing inputs
like node-version: 24 and check-latest); ensure the step using
actions/setup-node is the one with the keys node-version and check-latest so the
workflow uses the newer setup-node action version.
In @frontend/src/components/common/param-field.tsx:
- Around line 33-39: DocTypeAutoComplete is not receiving the current value so
the selected DocType doesn't render; update the JSX where DocTypeAutoComplete is
used to pass the current value (e.g., value={param.value} or the appropriate
property on param that holds the selected doctype) and keep the existing
onChange={handleChange} to maintain controlled behavior; ensure you pass
undefined/null if no value to avoid uncontrolled-to-controlled warnings.
In @frontend/src/components/workflows/configPanel.tsx:
- Around line 177-195: actionFormState updates from handleActionFieldChange are
never persisted back to the editor store, so
editorStore.selectedNode.data.parameters remains stale when handleSaveWorkflow
calls nodesToHazelNodes(editorStore.nodes); either update the selected node on
each field change or merge before save: in handleActionFieldChange (or right
before handleSaveWorkflow runs) write the current actionFormState into
editorStore.selectedNode.data.parameters (or replace the selected node via
editorStore.updateNode/commit API) so nodesToHazelNodes reads the up-to-date
parameters from editorStore.nodes.
- Around line 57-69: The useEffect reads hazelWorkflow.trigger_config with
JSON.parse which can throw on malformed JSON; wrap the parse in a try-catch
(inside the same useEffect) to safely handle parse errors, e.g., try to parse
trigger_config into savedConfig and on failure fallback to an empty object and
optionally log the error; then continue initializing initState from
triggerDoc.params and call setTriggerFormState(initState) as before.
- Around line 38-44: The useFrappeGetDoc calls (useFrappeGetDoc) are currently
passed empty-string fallbacks, causing unnecessary fetches; update the two calls
that produce triggerDoc and actionDoc to use undefined instead of '' as the
fallback (use hazelWorkflow.trigger_type || undefined and
editorStore.selectedNode?.data.type || undefined) so SWR skips fetching when the
docname is missing.
In @frontend/src/components/workflows/editor.tsx:
- Around line 61-79: The onConnect callback captures editorStore.edges causing
stale closures; change the call to editorStore.setEdges to use a functional
updater so it receives the latest edges (i.e., pass a function that takes
prevEdges and returns addEdge(newEdge, prevEdges)) instead of referencing
editorStore.edges directly, and if editorStore.setEdges does not currently
accept a callback, update the store implementation to support a functional
update signature; refer to onConnect, editorStore.setEdges, addEdge, and
editorStore.edges when making the change.
In @frontend/src/stores/editor.ts:
- Around line 120-129: The removeNode function can leave selectedNode pointing
to a removed node; update removeNode (in frontend/src/stores/editor.ts) to check
get().selectedNode (or get().selectedNode?.id) against nodeId and, if it
matches, include selectedNode: null in the set() call alongside flowNodes and
flowEdges so the selection is cleared when the node is deleted.
In @frontend/src/utils/editor.ts:
- Around line 49-71: The node position logic uses || which treats 0 as falsy and
overrides valid coordinates; update the coordinate defaults in the loop that
processes hazelWorkflow.nodes to use nullish coalescing (e.g., node.position_x
?? DEFAULT_X and node.position_y ?? currentY) so zero positions are preserved,
and ensure currentY is still updated via currentY = posY + STEP_Y after pushing
into processedNodes (references: hazelWorkflow.nodes, node.position_x,
node.position_y, DEFAULT_X, currentY, processedNodes.push, getNodeTypeForData).
- Around line 139-148: The edgesToHazelConnections function is currently casting
edge.sourceHandle which can hide invalid runtime values; replace the cast with
runtime sanitization in edgesToHazelConnections by checking edge.sourceHandle
against the allowed set ('default', 'true', 'false') and using 'default' as the
fallback (handle undefined or any other string), then assign that sanitized
value to source_handle (keep source_node_id mapping from edge.source and
target_node_id from edge.target).
In @hazelnode/fixtures/hazel_node_type.json:
- Around line 101-149: The Condition node handler (ConditionNode.execute) must
validate operands: detect when operator is binary (e.g., equals, not_equals,
greater_than, less_than, greater_than_or_equal, less_than_or_equal, contains,
not_contains, starts_with, ends_with) and if so, ensure right_operand is
provided (not null/empty); if missing, throw/raise a clear error before
performing the comparison instead of defaulting to an empty string and letting
the later comparison/exception occur. Locate the execute method in
hazelnode.nodes.actions.condition_node.ConditionNode and add a pre-check for
these operators that raises an informative exception mentioning the missing
right_operand and the operator.
In @hazelnode/nodes/actions/condition_node.py:
- Around line 60-69: The return value is merging the input context into
top-level keys which can overwrite 'branch', 'condition_result', or 'evaluated';
stop spreading context directly and instead include it under its own key (e.g.,
'context': context) or explicitly filter out reserved keys before merging.
Locate the return in condition_node.py where the dict with 'branch',
'condition_result', and 'evaluated' is constructed and replace the "**context"
spread with a nested 'context' field (or build a new_context that excludes
'branch'/'condition_result'/'evaluated' and merge that) so reserved keys cannot
be clobbered; remember this will require updating any callers that expect flat
context.
In @hazelnode/nodes/actions/create_document_node.py:
- Around line 1-3: The import block in create_document_node.py is not
ordered/formatted per project lint rules; reorder and format imports (e.g.,
ensure standard library, third-party, then local imports) and run the project's
linter/formatter (eg. isort/black or the repo's preconfigured tool) to fix the
imports for frappe, hazelnode.nodes.Node, and
hazelnode.nodes.utils.parse_json_field/ensure_context so the import section is
sorted and CI linting passes.
- Line 27: The code calls doc.insert(ignore_permissions=True) which bypasses
Frappe's permission checks; remove the ignore_permissions=True flag or, if
bypass is not acceptable, add an explicit permission check before insertion
using frappe.has_permission(doctype, 'create') and raise an error (frappe.throw)
if the caller lacks create rights; if a system-level bypass is required,
document the justification and surrounding safeguards where
doc.insert(ignore_permissions=True) is used.
In @hazelnode/nodes/actions/log_node.py:
- Around line 1-3: Imports in log_node.py are unsorted causing linter failure;
reorder them to follow import grouping and alphabetical order (standard library,
third-party, local) and sort within groups—specifically ensure "import frappe"
appears in the third-party group and the two local imports "from hazelnode.nodes
import Node" and "from hazelnode.nodes.utils import ensure_context,
render_template_field" are grouped and alphabetized so the linter passes.
In @hazelnode/nodes/actions/set_variable_node.py:
- Around line 1-2: Reorder and group imports in set_variable_node.py so standard
library and third-party imports (if any) come before local package imports;
specifically ensure the local imports from hazelnode (symbols Node,
render_template_field, ensure_context) are grouped together and placed after
external imports with a blank line separating groups, and alphabetize within
each group to satisfy the linter.
In @hazelnode/nodes/actions/update_document_node.py:
- Around line 1-7: Imports are unsorted causing a linter failure; reorder them
into proper groups and alphabetical order: keep the third-party import "frappe"
first, then the local package imports grouped together with "from
hazelnode.nodes import Node" followed by "from hazelnode.nodes.utils import
(parse_json_field, render_template_field, ensure_context)" (ensure the names
inside the utils import are alphabetized as needed) so the file adheres to
sorted-import conventions.
In @hazelnode/nodes/utils.py:
- Around line 3-4: Reorder the imports so the standard library import appears
before third-party imports and add a separating blank line; specifically ensure
"import json" comes first, then a blank line, then "import frappe" in
hazelnode/nodes/utils.py so the import block is sorted and separated per linter
(ruff I001) expectations.
🧹 Nitpick comments (23)
frontend/src/components/nodes/condition-node.tsx (3)
16-25: Optimize node selection logic.The current implementation uses a loop to find the matching node, which is inefficient when only one node can match the
id. Consider simplifying the logic:♻️ Proposed optimization
useOnSelectionChange({ onChange: ({ nodes }) => { - for (const node of nodes) { - if (node.id === id) { - editorStore.setSelectedNode(node); - break; - } - } + const matchingNode = nodes.find((node) => node.id === id); + if (matchingNode) { + editorStore.setSelectedNode(matchingNode); + } }, });
53-68: Consider extracting handle configuration.The handle positioning uses a mix of Tailwind classes (
!-translate-x-8) and inline styles (left: '30%'), which may be harder to maintain. Consider centralizing handle configuration in a constant or using consistent positioning.♻️ Example refactor using constants
+const HANDLE_CONFIG = { + true: { left: '30%', className: '!bg-green-500 !w-3 !h-3 !-translate-x-8' }, + false: { left: '70%', className: '!bg-red-500 !w-3 !h-3 !translate-x-8' }, +} as const; + {/* True output (left) */} <Handle type="source" position={Position.Bottom} id="true" - className="!bg-green-500 !w-3 !h-3 !-translate-x-8" - style={{ left: '30%' }} + className={HANDLE_CONFIG.true.className} + style={{ left: HANDLE_CONFIG.true.left }} /> {/* False output (right) */} <Handle type="source" position={Position.Bottom} id="false" - className="!bg-red-500 !w-3 !h-3 !translate-x-8" - style={{ left: '70%' }} + className={HANDLE_CONFIG.false.className} + style={{ left: HANDLE_CONFIG.false.left }} />
46-68: Add accessibility labels to handles.The handles lack ARIA labels, which could make the workflow editor less accessible for users relying on assistive technologies.
♻️ Add aria-label attributes
<Handle type="target" position={Position.Top} className="!bg-gray-400 !w-3 !h-3" + aria-label="Input connection" /> {/* True output (left) */} <Handle type="source" position={Position.Bottom} id="true" className="!bg-green-500 !w-3 !h-3 !-translate-x-8" style={{ left: '30%' }} + aria-label="True condition output" /> {/* False output (right) */} <Handle type="source" position={Position.Bottom} id="false" className="!bg-red-500 !w-3 !h-3 !translate-x-8" style={{ left: '70%' }} + aria-label="False condition output" />frontend/src/components/common/doctype-autocomplete.tsx (1)
29-34: Add loading and error state handling.The component doesn't handle loading or error states from
useFrappeGetDocList. Users won't see feedback while data is being fetched or if the request fails.♻️ Add loading and error handling
- const { data: documents } = useFrappeGetDocList<DocTypeRecord>( + const { data: documents, isLoading, error } = useFrappeGetDocList<DocTypeRecord>( doctype, { fields: ['name'], } ); + + if (error) { + return ( + <Button outline={true} disabled className="w-[200px]"> + Error loading {doctype} + </Button> + ); + }And update the button to show loading state:
<Button outline={true} role="combobox" aria-expanded={open} className="w-[200px] justify-between" + disabled={isLoading} > {value ? documents?.find((doc) => doc.name === value)?.name - : `Select ${doctype}...`} + : isLoading ? 'Loading...' : `Select ${doctype}...`} <CaretSortIcon className="ml-2 h-4 w-4 shrink-0 opacity-50" /> </Button>frontend/src/components/common/param-field.tsx (2)
50-56: Filter empty options and use stable keys.The
split('\n')may produce empty strings from trailing newlines, resulting in invalid options. Additionally, using the option text as the React key could cause issues if options are duplicated.♻️ Proposed fix
<SelectContent> - {param.options?.split('\n').map((option) => ( - <SelectItem key={option} value={option}> + {param.options?.split('\n').filter(opt => opt.trim()).map((option, idx) => ( + <SelectItem key={`${param.fieldname}-${idx}`} value={option}> {option} </SelectItem> ))} </SelectContent>
33-69: Add fallback handling for unsupported field types.The component silently renders nothing (except the label) for unsupported field types. This could make debugging difficult when new field types are added. Consider logging a warning or rendering a placeholder message.
💡 Suggested improvement
Add a fallback case after the existing conditionals:
{!['Link', 'Select', 'Data', 'Int'].includes(param.fieldtype) && ( <div className="text-sm text-muted-foreground"> Unsupported field type: {param.fieldtype} </div> )}Or add a console warning:
// At the end of ParamField, before return if (!['Link', 'Select', 'Data', 'Int'].includes(param.fieldtype)) { console.warn(`Unsupported field type: ${param.fieldtype} for field ${param.fieldname}`); }frontend/proxyOptions.ts (1)
15-17: Consider guarding against missinghostheader.If
req.headers.hostis undefined, calling.split(':')will throw. While unlikely in practice, a defensive check would prevent runtime errors in edge cases.♻️ Suggested improvement
- router: function (req: { headers: { host: string } }) { - const site_name = req.headers.host.split(':')[0]; + router: function (req: { headers: { host?: string } }) { + const site_name = req.headers.host?.split(':')[0] ?? '127.0.0.1'; return `http://${site_name}:${webserver_port}`; },frontend/src/components/workflows/create-dialog.tsx (1)
50-52: Consider logging the error for debugging.The catch block swallows the error details. Logging would help diagnose failed creations.
♻️ Suggested improvement
- } catch { + } catch (error) { + console.error('Failed to create workflow:', error); toast.error('Failed to create workflow'); }hazelnode/nodes/actions/log_node.py (1)
19-23: Consider supporting additional log levels.Currently only 'Error' gets special treatment; 'Warning' and 'Debug' are logged as info. If users expect different behavior for these levels, consider expanding the conditional.
♻️ Optional enhancement
# Log to console if log_level == 'Error': frappe.log_error(message, 'Workflow Log') + elif log_level == 'Warning': + frappe.logger().warning(f'[Workflow] {message}') + elif log_level == 'Debug': + frappe.logger().debug(f'[Workflow] {message}') else: frappe.logger().info(f'[Workflow] {message}')hazelnode/nodes/actions/update_document_node.py (1)
35-38: Consider handling non-existent document gracefully.If the document doesn't exist,
frappe.get_doc()will raiseDoesNotExistError. Adding a try/catch would provide a clearer error message for workflow debugging.♻️ Suggested improvement
+ try: + doc = frappe.get_doc(doctype, docname) + except frappe.DoesNotExistError: + frappe.throw(f'Document {doctype}/{docname} does not exist') - doc = frappe.get_doc(doctype, docname) for field, value in field_values.items(): setattr(doc, field, value) doc.save(ignore_permissions=True)frontend/src/components/workflows/node-palette.tsx (2)
13-19: Consider handling fetch errors.Currently, if the fetch fails, the loading skeletons will display indefinitely since only
isLoadingis checked. Adding error handling would improve UX.♻️ Suggested improvement
- const { data: nodeTypes, isLoading } = useFrappeGetDocList<HazelNodeType>( + const { data: nodeTypes, isLoading, error } = useFrappeGetDocList<HazelNodeType>( 'Hazel Node Type', { fields: ['name', 'description', 'kind'], filters: [['kind', '=', 'Action']], } );Then add after the loading check:
if (error) { return <p className="p-3 text-sm text-red-500">Failed to load nodes</p>; }
97-107: Consider narrowing the type definitions.The
kindprop includes'Trigger'but only'Action'nodes are ever passed. Thecolorprop includes'lime'which is unused. Tightening these would improve type safety.♻️ Suggested refinement
interface PaletteNodeProps { name: string; description?: string; - kind: 'Action' | 'Trigger'; - color: 'amber' | 'zinc' | 'lime'; + kind: 'Action'; + color: 'amber' | 'zinc'; onDragStart: (frontend/src/components/nodes/node.tsx (2)
6-12: Consider clarifying node_id vs id distinction.Both
node_idinEditorNodeDataand theidprop inWorkflowNodeappear to serve as node identifiers. The selection logic (lines 26, 29) uses theidprop rather thandata.node_id.Consider documenting when to use each identifier, or verify if
node_idis still needed given theidprop usage throughout the component.
57-67: Handle styling uses !important flags.The Handle components use
!importantin Tailwind classes (!bg-gray-400 !w-3 !h-3) to override default styles. While this works, consider whether CSS specificity can be managed through more specific selectors or by adjusting the component hierarchy to avoid!important.The
id="default"on the source handle is correctly used for connection management in the graph-based workflow.frontend/src/components/workflows/list.tsx (1)
32-53: Consider logging error details for debugging.The error handling correctly reverts the optimistic update and shows a user-friendly message. However, the catch block doesn't log or expose error details, which could make debugging more difficult.
Consider logging the error for troubleshooting purposes:
📝 Proposed enhancement for error handling
- } catch { + } catch (error) { + console.error('Failed to update workflow:', error); mutate(); toast.error('Failed to update workflow'); }hazelnode/nodes/actions/condition_node.py (1)
49-50: Silent fallback for unknown operator may hide configuration errors.If an invalid operator string is provided, the code silently falls back to
operator.eq. Consider logging a warning or raising a validation error to surface misconfigurations.Optional: Add warning for unknown operator
+ import frappe + # Get the operator function - op_func = self.OPERATORS.get(op, operator.eq) + op_func = self.OPERATORS.get(op) + if op_func is None: + frappe.log_error(f'Unknown operator "{op}", defaulting to equals') + op_func = operator.eqfrontend/src/components/workflows/editor.tsx (1)
55-59: Consider addinghazelWorkflowto the dependency array or memoizing.The
useEffectdepends onhazelWorkflow.nodesandhazelWorkflow.connections, but also callseditorStore.setNodesandeditorStore.setEdgeswhich aren't in the dependency array. This is likely intentional, but the linter may flag missing dependencies.hazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.py (2)
146-170: N+1 query:frappe.db.get_valuecalled inside the execution loop.
_get_next_node_idcallsfrappe.db.get_valuefor each node to fetch the node type's kind. For workflows with many nodes, this creates N+1 query performance issues.Consider caching node type kinds
Pre-fetch all node type kinds before execution:
def execute(self, context=None, raise_exception=False): # Cache node type kinds at the start node_types = {n.type for n in self.nodes} self._node_kind_cache = { nt: frappe.db.get_value('Hazel Node Type', nt, 'kind') for nt in node_types } # ... rest of execute methodThen use
self._node_kind_cache.get(current_node.type)in_get_next_node_id.
148-152: Unused database fetch:node_typevariable is assigned but not used.The
node_typeis fetched from the database but isn't used in the subsequent logic. The branching decision relies solely onresult.get('branch'). Either remove this fetch or use it for explicit type validation.Remove unused fetch
def _get_next_node_id(self, current_node, result): """Get the next node ID based on the current node type and result.""" - # Check if this is a condition node - node_type = frappe.db.get_value( - 'Hazel Node Type', current_node.type, 'kind' - ) - # For condition nodes, check the branch in result if isinstance(result, dict) and 'branch' in result:frontend/src/constants/editor.ts (2)
44-67: Tighten handle typing to avoid accidental/invalid handle strings.You already export
EdgeHandle; consider using it ingetEdgeStyleForHandle(and downstream) so only known handles are passed around.Proposed diff
-export function getEdgeStyleForHandle(sourceHandle: string | null | undefined): { +export function getEdgeStyleForHandle(sourceHandle: EdgeHandle | null | undefined): { stroke: string; label?: string; labelStyle?: { fill: string; fontWeight: number }; } {
72-78: Consider makingcreateEdgeIddelimiter-safe to reduce collision risk.If node ids can contain
-(e.g.set-trigger), simple concatenation can theoretically collide. Using a less ambiguous delimiter (or encoding) makes this more robust.Proposed diff
export function createEdgeId( source: string, sourceHandle: string | null | undefined, target: string ): string { - return `${source}-${sourceHandle || EDGE_HANDLES.DEFAULT}-${target}`; + const h = sourceHandle || EDGE_HANDLES.DEFAULT; + return `e::${encodeURIComponent(source)}::${encodeURIComponent(h)}::${encodeURIComponent(target)}`; }frontend/src/stores/editor.ts (1)
77-81: Prefercrypto.randomUUID()(or similar) over counter+Date.now for node ids.This avoids rare collision cases (especially after
resetFlows) and removes timing dependence.Proposed diff
generateNodeId() { const counter = get().nodeCounter + 1; set({ nodeCounter: counter }); - return `node_${counter}_${Date.now()}`; + const uuid = + typeof crypto !== 'undefined' && 'randomUUID' in crypto + ? crypto.randomUUID() + : `${Date.now()}_${Math.random().toString(16).slice(2)}`; + return `node_${counter}_${uuid}`; },frontend/src/components/workflows/set-trigger-dialog.tsx (1)
30-58: AddworkflowNameguard before callingupdateDocto prevent silent failures if the component is refactored or reused.While the dialog is currently only rendered within the
/workflow/$idroute context, adding a guard is defensive and prevents potential issues if the component is moved or reused. IfworkflowNameis undefined,updateDocwill silently fail without clear feedback.Proposed diff
const { updateDoc } = useFrappeUpdateDoc<HazelWorkflow>(); async function setTrigger(trigger: HazelNodeType) { + if (!workflowName) { + toast.error('Missing workflow id in route'); + return; + } try { await updateDoc('Hazel Workflow', workflowName, { trigger_type: trigger.name, }); editorStore.resetFlows(); editorStore.appendNode({ name: trigger.name, type: trigger.name, kind: 'Trigger' }); onClose(false); toast.success('Trigger set successfully!'); - } catch { - toast.error('Failed to set trigger'); + } catch (e) { + toast.error('Failed to set trigger'); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (42)
.github/workflows/ci.ymlfrontend/package.jsonfrontend/proxyOptions.tsfrontend/src/components/common/doctype-autocomplete.tsxfrontend/src/components/common/param-field.tsxfrontend/src/components/nodes/condition-node.tsxfrontend/src/components/nodes/node.tsxfrontend/src/components/workflows/configPanel.tsxfrontend/src/components/workflows/create-dialog.tsxfrontend/src/components/workflows/details.tsxfrontend/src/components/workflows/editor.tsxfrontend/src/components/workflows/list.tsxfrontend/src/components/workflows/node-palette.tsxfrontend/src/components/workflows/set-trigger-dialog.tsxfrontend/src/constants/editor.tsfrontend/src/global.d.tsfrontend/src/lib/request.tsfrontend/src/main.tsxfrontend/src/queries/frappe.tsfrontend/src/queries/user.tsfrontend/src/routes/__root.tsxfrontend/src/routes/workflow.$id.tsxfrontend/src/stores/editor.tsfrontend/src/types/Hazelnode/HazelNode.tsfrontend/src/types/Hazelnode/HazelNodeConnection.tsfrontend/src/types/Hazelnode/HazelWorkflow.tsfrontend/src/utils/editor.tshazelnode/fixtures/hazel_node_type.jsonhazelnode/hazelnode/doctype/hazel_node/hazel_node.jsonhazelnode/hazelnode/doctype/hazel_node/hazel_node.pyhazelnode/hazelnode/doctype/hazel_node_connection/__init__.pyhazelnode/hazelnode/doctype/hazel_node_connection/hazel_node_connection.jsonhazelnode/hazelnode/doctype/hazel_node_connection/hazel_node_connection.pyhazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.jsonhazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.pyhazelnode/nodes/actions/condition_node.pyhazelnode/nodes/actions/create_document_node.pyhazelnode/nodes/actions/delay_node.pyhazelnode/nodes/actions/log_node.pyhazelnode/nodes/actions/set_variable_node.pyhazelnode/nodes/actions/update_document_node.pyhazelnode/nodes/utils.py
💤 Files with no reviewable changes (4)
- frontend/src/lib/request.ts
- frontend/src/routes/__root.tsx
- frontend/src/routes/workflow.$id.tsx
- frontend/src/queries/frappe.ts
🧰 Additional context used
🧬 Code graph analysis (14)
frontend/src/components/nodes/condition-node.tsx (4)
frontend/src/components/nodes/node.tsx (1)
EditorNodeData(6-12)frontend/src/stores/editor.ts (1)
useEditorStore(72-172)frontend/src/components/ui/card.tsx (3)
Card(77-77)CardHeader(78-78)CardTitle(80-80)frontend/src/components/ui/badge.tsx (1)
Badge(36-51)
hazelnode/nodes/actions/update_document_node.py (6)
hazelnode/nodes/__init__.py (1)
Node(4-7)hazelnode/nodes/utils.py (3)
parse_json_field(7-35)render_template_field(38-55)ensure_context(58-60)hazelnode/nodes/actions/create_document_node.py (1)
execute(9-32)hazelnode/nodes/actions/log_node.py (1)
execute(12-33)hazelnode/nodes/actions/set_variable_node.py (1)
execute(11-21)hazelnode/nodes/actions/delay_node.py (1)
execute(11-27)
frontend/src/types/Hazelnode/HazelNodeConnection.ts (1)
hazelnode/hazelnode/doctype/hazel_node_connection/hazel_node_connection.py (1)
HazelNodeConnection(7-24)
frontend/src/global.d.ts (2)
frontend/src/types/Hazelnode/HazelNodeConnection.ts (1)
HazelNodeConnection(1-18)hazelnode/hazelnode/doctype/hazel_node_connection/hazel_node_connection.py (1)
HazelNodeConnection(7-24)
frontend/src/components/workflows/create-dialog.tsx (1)
frontend/src/components/ui/button.tsx (1)
Button(191-222)
frontend/src/components/workflows/editor.tsx (2)
frontend/src/constants/editor.ts (5)
NODE_TYPES(26-30)getEdgeStyleForHandle(44-67)createEdgeId(72-78)DRAG_DATA_TYPE(97-97)DragData(92-95)frontend/src/components/nodes/add-trigger-node.tsx (1)
AddTriggerNode(6-24)
hazelnode/hazelnode/doctype/hazel_node_connection/hazel_node_connection.py (1)
frontend/src/types/Hazelnode/HazelNodeConnection.ts (1)
HazelNodeConnection(1-18)
frontend/src/components/workflows/configPanel.tsx (3)
frontend/src/hooks/confirm.ts (1)
useConfirm(4-6)frontend/src/utils/editor.ts (2)
nodesToHazelNodes(125-137)edgesToHazelConnections(139-148)frontend/src/components/common/param-field.tsx (1)
ParamForm(80-99)
hazelnode/nodes/actions/condition_node.py (1)
hazelnode/nodes/__init__.py (1)
Node(4-7)
frontend/src/types/Hazelnode/HazelWorkflow.ts (1)
frontend/src/types/Hazelnode/HazelNodeConnection.ts (1)
HazelNodeConnection(1-18)
frontend/src/components/workflows/set-trigger-dialog.tsx (2)
frontend/src/types/Hazelnode/HazelNodeType.ts (1)
HazelNodeType(3-26)hazelnode/hazelnode/doctype/hazel_node_type/hazel_node_type.py (1)
HazelNodeType(8-29)
frontend/src/stores/editor.ts (2)
frontend/src/components/nodes/node.tsx (1)
EditorNodeData(6-12)frontend/src/constants/editor.ts (3)
getNodeTypeForData(83-87)createEdgeId(72-78)EDGE_HANDLES(4-8)
hazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.py (4)
hazelnode/hazelnode/doctype/hazel_node_connection/hazel_node_connection.py (1)
HazelNodeConnection(7-24)hazelnode/hazelnode/doctype/hazel_node/hazel_node.py (1)
execute(31-39)hazelnode/nodes/http_request_node.py (1)
execute(8-20)hazelnode/nodes/__init__.py (1)
execute(6-7)
hazelnode/nodes/actions/set_variable_node.py (2)
hazelnode/nodes/__init__.py (1)
Node(4-7)hazelnode/nodes/utils.py (2)
render_template_field(38-55)ensure_context(58-60)
🪛 actionlint (1.7.10)
.github/workflows/ci.yml
47-47: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 GitHub Actions: Linter
hazelnode/nodes/actions/update_document_node.py
[error] 1-1: ruff I001 Import block is un-sorted or un-formatted. Organize imports.
hazelnode/nodes/utils.py
[error] 1-1: ruff I001 Import block is un-sorted or un-formatted. Organize imports.
hazelnode/nodes/actions/delay_node.py
[error] 1-1: ruff I001 Import block is un-sorted or un-formatted. Organize imports.
hazelnode/nodes/actions/create_document_node.py
[error] 1-1: ruff I001 Import block is un-sorted or un-formatted. Organize imports.
hazelnode/nodes/actions/set_variable_node.py
[error] 1-1: ruff I001 Import block is un-sorted or un-formatted. Organize imports.
hazelnode/nodes/actions/log_node.py
[error] 1-1: ruff I001 Import block is un-sorted or un-formatted. Organize imports.
🔇 Additional comments (48)
frontend/src/components/common/doctype-autocomplete.tsx (1)
1-84: LGTM! Clean migration to frappe-react-sdk.The migration from the custom hook to
useFrappeGetDocListsimplifies the data-fetching logic and aligns with the PR's objective to standardize on frappe-react-sdk. The component correctly handles the new data structure..github/workflows/ci.yml (2)
44-44: No action required. Python 3.14 is the latest stable version as of January 2026 (3.14.2 released December 5, 2025). This configuration is correct.Likely an incorrect or invalid review comment.
49-49: Node.js 24 is the current stable LTS version and this configuration is correct.Node.js 24 was released on May 6, 2025, and entered Active LTS in October 2025. As of January 2026, version 24.x is the current Node.js LTS (latest 24.12.0), with security support guaranteed until April 30, 2028. This configuration will not cause CI failures.
Likely an incorrect or invalid review comment.
frontend/src/queries/user.ts (1)
9-11: Backend endpoint exists and response type annotation is correct.The endpoint
hazelnode.api.get_current_user_infoexists and the type signature{ message: UserInfo }is correct—Frappe's REST API automatically wraps responses in a{ message }envelope.However, no consumers of
useUserInfo()were found in the codebase. Verify that this hook is actually being called somewhere, or if this is new code, ensure consumers are updated to accessdata?.messagewhen this hook is integrated.frontend/src/types/Hazelnode/HazelNode.ts (1)
12-21: LGTM! Graph positioning fields added correctly.The addition of
node_id,position_x, andposition_yproperly supports the new graph-based workflow system. The mandatorynode_idis consistently populated across all code paths—new nodes generated in the editor receive unique IDs viagenerateNodeId(), and existing nodes loaded from workflows use either the storednode_idor fall back tonode.namefor backward compatibility. The optional positional fields maintain backward compatibility while enabling graph positioning.frontend/src/components/common/param-field.tsx (1)
80-99: LGTM!The
ParamFormimplementation correctly iterates over parameters and maintains stable keys usingparam.fieldname.hazelnode/hazelnode/doctype/hazel_node/hazel_node.py (1)
21-21: LGTM!The type hint additions for
node_id,position_x, andposition_yare properly scoped within theTYPE_CHECKINGblock and follow the existing pattern. These changes support the graph-based workflow positioning without affecting runtime behavior.Also applies to: 26-27
hazelnode/fixtures/hazel_node_type.json (4)
150-186: Thevalueparameter being optional is intentional and well-defined.When
valueis not provided, the handler defaults to an empty string (params.get('value', '')) which is then set in the workflow context. This allows users to optionally set variables to empty strings or use template rendering with empty defaults. The behavior is explicit and consistent.
187-223: JSON parsing forfield_valuesis already safely implemented.The handler uses the
parse_json_field()utility which properly validates JSON input, renders template variables, and throws descriptive errors for malformed JSON. No changes required.
224-272: JSON parsing forfield_valuesis already safely handled.The handler uses the
parse_json_field()utility function which properly validates JSON with try-except error handling, safe defaults for empty values, and descriptive error messages on parse failure. No changes needed.
310-334: Thedelay_secondsparameter is already validated in the handler.The DelayNode implementation caps the delay at 60 seconds (line 22:
min(delay_seconds, 60)) and prevents negative delays from executing (line 24:if delay_seconds > 0). Type conversion also handles invalid inputs gracefully. The handler validation addresses the concerns about negative delays and excessively long pauses.Likely an incorrect or invalid review comment.
frontend/package.json (1)
32-32: Version ^1.13.0 is current and has no known security vulnerabilities.The specified version is the latest stable release (October 2025). Snyk scans show no direct vulnerabilities in frappe-react-sdk. Security concerns within the Frappe ecosystem are server-side framework issues, not SDK issues.
frontend/src/types/Hazelnode/HazelWorkflow.ts (1)
2-2: LGTM! Proper type integration for graph connections.The import and field addition follow the existing pattern and correctly enable the new graph-based workflow connections feature.
Also applies to: 25-26
frontend/src/main.tsx (1)
3-3: LGTM! Clean migration to frappe-react-sdk.The FrappeProvider is correctly imported and wraps the application at the appropriate level, replacing the previous React Query setup as intended.
Also applies to: 37-41
hazelnode/nodes/actions/delay_node.py (2)
11-27: LGTM! Reasonable delay implementation with appropriate safeguards.The execution logic correctly caps delays at 60 seconds to prevent abuse, handles invalid input gracefully, and includes documentation about using scheduled jobs for longer delays.
1-2: The imports in this file are already correctly sorted according to the project's isort configuration. Standard library imports (import time) come before local imports (from hazelnode.nodes import Node), which follows the configured rules. No changes are needed.Likely an incorrect or invalid review comment.
frontend/src/global.d.ts (1)
5-5: LGTM! Consistent type exposure pattern.The HazelNodeConnection type is correctly imported and exposed globally, following the established pattern for other types in this file.
Also applies to: 14-14
hazelnode/nodes/actions/create_document_node.py (1)
9-26: LGTM! Sound document creation logic with proper validation.The implementation correctly validates required parameters, uses template rendering for field values, and stores the created document in context for downstream nodes. The overall structure and error handling are appropriate.
Also applies to: 29-32
frontend/proxyOptions.ts (1)
1-9: LGTM! Graceful fallback for config loading.The try/catch pattern handles building outside a Frappe bench environment appropriately.
hazelnode/nodes/actions/set_variable_node.py (1)
11-21: LGTM! Clean implementation following established patterns.The execute method correctly normalizes context, renders template values, and conditionally stores the variable.
frontend/src/components/workflows/create-dialog.tsx (1)
29-53: LGTM! Clean migration to frappe-react-sdk with proper error handling.The async flow with try/catch, loading states, and toast notifications follows good UX patterns.
hazelnode/nodes/actions/log_node.py (1)
12-33: LGTM! Good pattern for workflow observability.Storing logs in context enables debugging workflow executions after the fact.
frontend/src/components/workflows/details.tsx (3)
17-20: LGTM! Clean migration to frappe-react-sdk data fetching.The hook usage with proper TypeScript generic and parameter handling is correct.
22-33: Good loading and error state handling.The skeleton placeholders provide appropriate visual feedback during loading.
35-55: Well-structured resizable panel layout.The three-panel layout with NodePalette, Editor, and ConfigPanel provides good UX for workflow editing. Panel size constraints (minSize/maxSize) on the palette prevent it from dominating the view.
hazelnode/nodes/actions/update_document_node.py (1)
38-38: Verifyignore_permissions=Trueis intentional.This bypasses Frappe's permission system, allowing workflows to update any document regardless of user permissions. Ensure this aligns with your security model for workflow automation.
frontend/src/components/workflows/node-palette.tsx (2)
21-29: LGTM! Clean drag-and-drop implementation.The drag data serialization with proper typing follows the established pattern from
editor.tsconstants.
109-133: LGTM! Well-structured PaletteNode component.Good use of hover/active states for drag feedback and proper conditional rendering for the description.
hazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.json (2)
7-18: LGTM! Schema structure is well-organized.The addition of the connections section with proper labeling for the nodes section improves the schema clarity and maintains logical field grouping.
Also applies to: 34-37
44-54: LGTM! Table field properly configured.The connections field correctly references the Hazel Node Connection DocType and follows Frappe's table field conventions.
frontend/src/components/nodes/node.tsx (2)
23-35: LGTM! Selection logic correctly updated.The id-based node selection logic properly handles both Action and Trigger node types, with appropriate state management for each case.
37-56: LGTM! Styling refinements improve consistency.The reduced minWidth, updated padding, and strict equality check for Badge color are all improvements that enhance the component's visual design and code quality.
hazelnode/hazelnode/doctype/hazel_node_connection/hazel_node_connection.py (1)
1-24: LGTM! Standard Frappe DocType implementation.The HazelNodeConnection class correctly follows Frappe's DocType pattern with auto-generated type hints that align with the frontend interface. The
source_handleLiteral values ('default', 'true', 'false') properly support conditional branching in the workflow graph.frontend/src/components/workflows/list.tsx (3)
22-30: LGTM! Data fetching properly configured.The useFrappeGetDocList hook is correctly configured with appropriate fields and ordering. The separation of read and write operations through distinct hooks follows best practices.
55-62: LGTM! Loading state properly handled.The loading state uses appropriate skeleton UI for a good user experience during data fetching.
64-118: LGTM! UI rendering and error state are well-implemented.The component correctly handles error states, renders the workflow list with proper null-safety (workflows?.map), and integrates the toggle functionality cleanly.
hazelnode/hazelnode/doctype/hazel_node/hazel_node.json (2)
39-55: LGTM! Position fields properly defined.The position fields with Section Break labeling and default values enable visual workflow layout. The Int fieldtype is appropriate for node coordinates.
Note: The default values are specified as strings ("0"), which is typical in Frappe JSON schemas and will be correctly handled during document creation.
6-17: LGTM! Schema structure is well-organized.The field ordering is logical (ID, type, position, parameters), and the section labels improve UI clarity. The schema properly supports the graph-based workflow functionality.
Also applies to: 57-60
frontend/src/types/Hazelnode/HazelNodeConnection.ts (1)
1-18: LGTM!The interface correctly mirrors the backend
HazelNodeConnectionDocType structure. The field types align with the Python definition inhazel_node_connection.py, and thesource_handleliteral union matches the Select options defined in the JSON schema.hazelnode/nodes/actions/condition_node.py (1)
71-94: LGTM!The variable resolution logic is well-implemented. The
{{variable}}syntax parsing and nested dot-notation access handle common use cases effectively.hazelnode/hazelnode/doctype/hazel_node_connection/hazel_node_connection.json (1)
1-53: LGTM!The DocType definition is well-structured as a child table (
istable: 1). The field definitions correctly align with both the TypeScript interface and the Python class. Thesource_handlehaving a default value of "default" appropriately handles the common case of non-conditional edges.hazelnode/nodes/utils.py (2)
48-55: LGTM with a minor note.The template rendering logic is clean. Note that falsy non-string values like
0orFalsewill be converted to an empty string, which is likely acceptable for template fields but worth being aware of.
7-36: LGTM!The
parse_json_fieldfunction handles all input cases appropriately: empty values, pre-parsed dicts, and JSON strings with template rendering. The error handling withfrappe.throwprovides clear feedback on invalid JSON.frontend/src/components/workflows/configPanel.tsx (1)
115-130: LGTM!The save workflow logic correctly aggregates trigger config, nodes, and connections. The use of
nodesToHazelNodesandedgesToHazelConnectionsutility functions maintains separation of concerns, and error handling is properly implemented.frontend/src/components/workflows/editor.tsx (1)
86-117: LGTM!The drag-and-drop implementation is well-structured. The coordinate translation via
screenToFlowPositioncorrectly handles the conversion from screen to flow coordinates. Error handling for malformed drag data is appropriate.hazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.py (2)
109-127: LGTM on cycle detection logic.The
visitedset appropriately prevents infinite loops in cyclic graphs. For a workflow execution engine, preventing any node from executing more than once per run is the expected behavior.
49-73: LGTM!The helper methods provide clean abstractions for graph traversal. The
'trigger'convention as the source node ID for finding the start node maintains backward compatibility with the linear fallback.frontend/src/stores/editor.ts (1)
131-161: No issue here—appendNodeis always called afterresetFlows().In
set-trigger-dialog.tsx,editorStore.resetFlows()clears all nodes beforeappendNode()is invoked. SinceresetFlows()empties theflowNodesarray, thecurrentNodes.length > 0check at line 146 ofappendNode()will always be false, preventing any edge creation. The set-trigger pseudo-node can never be the last node whenappendNode()runs.Likely an incorrect or invalid review comment.
- Update setup-node action to v4 in CI - Make DocTypeAutoComplete a controlled component with value prop - Add try-catch for JSON.parse in configPanel - Fix stale closure in editor onConnect using functional updater - Clear selectedNode when removed from store - Add updateNodeData action to persist action form changes - Use nullish coalescing for position coordinates (0 is valid) - Sanitize edge sourceHandle with type-safe validation - Validate binary operators require right_operand in ConditionNode - Fix context spread collision by nesting under 'context' key - Add permission checks to CreateDocument and UpdateDocument nodes
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @frontend/src/components/workflows/editor.tsx:
- Around line 101-104: The code incorrectly subtracts reactFlowBounds offsets
before calling reactFlowInstance.current.screenToFlowPosition, causing wrong
node positions; pass raw screen coordinates instead by calling
reactFlowInstance.current.screenToFlowPosition with event.clientX and
event.clientY (use the existing reactFlowInstance.current.screenToFlowPosition
symbol), removing the subtraction of reactFlowBounds.left and
reactFlowBounds.top so the method can perform its internal conversion correctly.
In @hazelnode/nodes/actions/condition_node.py:
- Around line 27-30: The string operator lambdas ('contains', 'not_contains',
'starts_with', 'ends_with') incorrectly coerce None to the literal "None";
update each operator to first handle None explicitly (e.g., treat None as
missing/empty and return False for containment/starts/ends checks or follow the
intended semantic) by checking if a is None or b is None before calling str()
and only then performing the comparison; modify the lambdas or replace them with
small functions named/located where 'contains', 'not_contains', 'starts_with',
and 'ends_with' are defined so they short-circuit on None instead of converting
it to "None".
- Around line 66-69: The except block around the evaluation of
op_func(left_value, right_value) silently swallows errors; change it to catch
Exception as e and log the exception before returning False so failures (type
errors, etc.) are visible. Use the module/class logger (or
logging.getLogger(__name__)) and call logger.exception or logger.error(...,
exc_info=True) with context including op_func.__name__ (or repr(op_func)),
left_value and right_value, then keep returning False to preserve fallback
behavior.
In @hazelnode/nodes/actions/update_document_node.py:
- Around line 1-7: Reorder the imports in update_document_node.py to satisfy the
linter: place third-party imports (frappe) first, then a blank line, then local
package imports from hazelnode (import Node from hazelnode.nodes and the
utilities parse_json_field, render_template_field, ensure_context from
hazelnode.nodes.utils); ensure each group is alphabetized and separated by a
blank line so the import block is standard-library, third-party, local-module
compliant.
🧹 Nitpick comments (12)
hazelnode/nodes/actions/update_document_node.py (2)
40-43: LGTM! Clean document update logic.The update flow is correct: fetch, modify fields, and save. Frappe will handle validation and raise appropriate errors if the document doesn't exist or fields are invalid.
💡 Optional: Add explicit error handling for missing document
For clearer error messages, you could wrap the
get_doccall:try: doc = frappe.get_doc(doctype, docname) except frappe.DoesNotExistError: frappe.throw(f'{doctype} {docname} not found')However, Frappe's default error handling is already clear, so this is purely optional.
45-47: LGTM! Proper context return.The updated document is correctly stored in the context for downstream workflow nodes.
💡 Optional: Add updated_doc_name for consistency
CreateDocumentNodestores bothcreated_docandcreated_doc_namein context. For consistency, consider also storing the document name:context['updated_doc'] = doc.as_dict() +context['updated_doc_name'] = doc.name return contextThis makes the pattern more predictable across action nodes, though the name is already available in
updated_doc.hazelnode/nodes/actions/condition_node.py (2)
63-63: Consider validating the operator instead of defaulting.Defaulting to
operator.eqwhen an unrecognized operator is provided could mask configuration errors or typos. Consider either raising an error or logging a warning when an invalid operator is encountered.✅ Proposed validation approach
# Get the operator function - op_func = self.OPERATORS.get(op, operator.eq) + op_func = self.OPERATORS.get(op) + if op_func is None: + return { + 'branch': 'false', + 'condition_result': False, + 'error': f"Invalid operator '{op}'", + 'context': context, + }
98-107: Consider supporting list indexing in nested paths.The current implementation handles nested dictionaries well but doesn't support array/list indexing via numeric keys (e.g.,
"items.0.name"to access the first item). This could be a useful enhancement for workflows that process arrays of data.🔧 Proposed enhancement for list indexing
def _get_nested_value(self, obj, path): """Get a nested value from a dict using dot notation.""" keys = path.split('.') current = obj for key in keys: if isinstance(current, dict): current = current.get(key) + elif isinstance(current, (list, tuple)): + try: + current = current[int(key)] + except (ValueError, IndexError, KeyError): + return None else: return None return currentfrontend/src/components/workflows/editor.tsx (2)
55-59: Missing dependencies in useEffect may cause stale closures or infinite loops.The effect calls
editorStore.setNodesandeditorStore.setEdgesbut they are not in the dependency array. Since zustand store selectors return stable references unless the selector changes, this works but violates exhaustive-deps. Additionally,hazelWorkflowis spread across dependencies buthazelWorkflow.trigger_typeand the workflow identity itself are not tracked.Consider using a stable workflow identifier:
useEffect(() => { const processedNodes = getProcessedNodes(hazelWorkflow); editorStore.setNodes(processedNodes); editorStore.setEdges(getProcessedEdges(hazelWorkflow, processedNodes)); - }, [hazelWorkflow.nodes, hazelWorkflow.connections]); + }, [hazelWorkflow]);Alternatively, if you intentionally want fine-grained control, also include
hazelWorkflow.trigger_typesincegetProcessedNodesdepends on it.
61-80: Non-null assertions on potentially null connection properties.
connection.sourceandconnection.targetcan benullaccording to the React FlowConnectiontype. The non-null assertions (!) will throw if these are ever null.Suggested defensive handling
const onConnect = useCallback( (connection: Connection) => { + if (!connection.source || !connection.target) { + return; + } const styleProps = getEdgeStyleForHandle(connection.sourceHandle); const newEdge = { ...connection, id: createEdgeId( - connection.source!, + connection.source, connection.sourceHandle, - connection.target! + connection.target ), markerEnd: { type: MarkerType.ArrowClosed }, label: styleProps.label, labelStyle: styleProps.labelStyle, style: { stroke: styleProps.stroke }, }; editorStore.setEdges((prevEdges) => addEdge(newEdge, prevEdges)); }, [editorStore.setEdges] );frontend/src/components/common/doctype-autocomplete.tsx (2)
41-46: Controlled component updates internal state unnecessarily.When
controlledValueis provided, the component is controlled and the parent manages the value. However,setValuestill updatesinternalValue, which is unused in controlled mode but could cause confusion or subtle bugs if the component switches between controlled and uncontrolled modes.Suggested improvement
const value = controlledValue ?? internalValue; const setValue = (newValue: string) => { - setInternalValue(newValue); + if (controlledValue === undefined) { + setInternalValue(newValue); + } onChange(newValue); };
48-53: Consider handling loading and error states fromuseFrappeGetDocList.The hook returns
isLoadinganderrorstates that are not being used. Users may see an empty dropdown while data is loading or if an error occurs.frontend/src/utils/editor.ts (1)
139-147: Type assertion insanitizeHandlecould be simplified.The function casts
handletoValidHandletwice. Consider a cleaner approach:Suggested simplification
function sanitizeHandle(handle: string | null | undefined): ValidHandle { - if (handle && VALID_HANDLES.includes(handle as ValidHandle)) { - return handle as ValidHandle; + if (handle && (VALID_HANDLES as readonly string[]).includes(handle)) { + return handle as ValidHandle; } return 'default'; }frontend/src/components/workflows/configPanel.tsx (2)
100-115: Action field changes trigger immediate store updates on every keystroke.Persisting to the editor store inside
setActionFormStatemeans every character typed updates the store synchronously. This could cause performance issues with frequent re-renders, especially with complex workflows.Consider debouncing the store update or only persisting on blur/save:
Suggested approach using onBlur
const handleActionFieldChange = (fieldname: string, value: string) => { - setActionFormState((prev) => { - const newState = { ...prev, [fieldname]: value }; - - // Persist to editor store immediately - if (editorStore.selectedNode) { - const parameters = Object.entries(newState).map(([fn, val]) => ({ - fieldname: fn, - value: val, - })); - editorStore.updateNodeData(editorStore.selectedNode.id, { parameters }); - } - - return newState; - }); + setActionFormState((prev) => ({ ...prev, [fieldname]: value })); }; + + const handleActionFieldBlur = () => { + if (editorStore.selectedNode) { + const parameters = Object.entries(actionFormState).map(([fn, val]) => ({ + fieldname: fn, + value: val, + })); + editorStore.updateNodeData(editorStore.selectedNode.id, { parameters }); + } + };
143-146: Double type assertion (as unknown as) suggests type mismatch.The
as unknown as HazelNode[]andas unknown as HazelNodeConnection[]casts bypass TypeScript's type checking entirely. This typically indicates thatnodesToHazelNodesandedgesToHazelConnectionsreturn types that don't match whatupdateDocexpects.Consider aligning the return types of these utility functions with the expected API types to maintain type safety.
frontend/src/stores/editor.ts (1)
80-84: Node ID generation could have edge-case collisions.While
Date.now()adds millisecond precision, rapid node additions (e.g., programmatic batch creation) within the same millisecond could theoretically produce duplicate IDs if the counter state update hasn't propagated. In practice, this is unlikely with user interactions.For robustness, consider using a more collision-resistant approach:
generateNodeId() { const counter = get().nodeCounter + 1; set({ nodeCounter: counter }); - return `node_${counter}_${Date.now()}`; + return `node_${counter}_${crypto.randomUUID().slice(0, 8)}`; },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ci.ymlfrontend/src/components/common/doctype-autocomplete.tsxfrontend/src/components/common/param-field.tsxfrontend/src/components/workflows/configPanel.tsxfrontend/src/components/workflows/editor.tsxfrontend/src/stores/editor.tsfrontend/src/utils/editor.tshazelnode/nodes/actions/condition_node.pyhazelnode/nodes/actions/create_document_node.pyhazelnode/nodes/actions/update_document_node.py
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/common/param-field.tsx
- hazelnode/nodes/actions/create_document_node.py
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/src/components/workflows/editor.tsx (4)
frontend/src/constants/editor.ts (5)
NODE_TYPES(26-30)getEdgeStyleForHandle(44-67)createEdgeId(72-78)DRAG_DATA_TYPE(97-97)DragData(92-95)frontend/src/components/nodes/node.tsx (1)
WorkflowNode(14-70)frontend/src/components/nodes/condition-node.tsx (1)
ConditionNode(7-77)frontend/src/utils/editor.ts (2)
getProcessedNodes(15-74)getProcessedEdges(76-123)
frontend/src/components/workflows/configPanel.tsx (4)
hazelnode/hazelnode/doctype/hazel_node_type/hazel_node_type.py (1)
HazelNodeType(8-29)frontend/src/types/Hazelnode/HazelNodeType.ts (1)
HazelNodeType(3-26)frontend/src/utils/editor.ts (2)
nodesToHazelNodes(125-137)edgesToHazelConnections(149-157)frontend/src/components/common/param-field.tsx (1)
ParamForm(81-100)
hazelnode/nodes/actions/update_document_node.py (3)
hazelnode/nodes/__init__.py (1)
Node(4-7)hazelnode/nodes/utils.py (3)
parse_json_field(7-35)render_template_field(38-55)ensure_context(58-60)hazelnode/nodes/actions/create_document_node.py (1)
execute(9-37)
frontend/src/stores/editor.ts (2)
frontend/src/components/nodes/node.tsx (1)
EditorNodeData(6-12)frontend/src/constants/editor.ts (3)
getNodeTypeForData(83-87)createEdgeId(72-78)EDGE_HANDLES(4-8)
🪛 GitHub Actions: Linter
hazelnode/nodes/actions/update_document_node.py
[error] 1-1: I001 Import block is un-sorted or un-formatted.
🔇 Additional comments (16)
.github/workflows/ci.yml (1)
47-49: Node.js 24 is available and in Active LTS status.The update to
actions/setup-node@v4and Node.js 24 is appropriate. Node.js 24 was released on May 6, 2025, and entered Active LTS status in October 2025, making it a solid choice for CI workflows.hazelnode/nodes/actions/update_document_node.py (3)
13-28: LGTM! Proper initialization and validation.The context initialization, parameter validation, and template rendering for
docnamefollow the established pattern fromCreateDocumentNode. This allows dynamic document references like{{created_doc_name}}from prior workflow steps.
29-32: LGTM! Appropriate permission check.Using
'write'permission for update operations is correct and aligns with Frappe's permission model.
34-38: LGTM! Consistent field parsing.The
parse_json_fieldusage matches the pattern inCreateDocumentNodeand properly handles template rendering for dynamic field values.hazelnode/nodes/actions/condition_node.py (2)
38-56: LGTM: Solid validation for binary operators.The validation correctly ensures binary operators have a right operand and returns a clear error message. The early return pattern prevents unnecessary processing.
84-96: LGTM: Clean variable resolution implementation.The
{{var}}syntax handling is clear and the delegation to_get_nested_valuefor dot-notation support is well-structured. The method correctly handles non-string values by passing them through unchanged.frontend/src/components/workflows/editor.tsx (1)
124-151: LGTM!The React Flow integration is well-structured with proper event handlers, node types mapping, and default edge options. The wrapper div with ref for drop position calculation and the
fitViewconfiguration are appropriate.frontend/src/components/common/doctype-autocomplete.tsx (1)
76-98: LGTM!The conditional rendering and CommandItem iteration are correctly implemented. The toggle selection behavior (re-selecting clears value) is a reasonable UX pattern for optional fields.
frontend/src/utils/editor.ts (3)
15-74: LGTM!The
getProcessedNodesfunction correctly handles the trigger-not-set case, positions nodes with fallback to defaults, and properly maps node data including the kind fallback to 'Action'.
76-123: LGTM!The edge processing correctly prioritizes backend connections when available and falls back to linear chaining. The SET_TRIGGER node filtering prevents invalid edges.
149-157: LGTM!The
edgesToHazelConnectionsfunction correctly maps React Flow edges to Hazel connection format with handle sanitization.frontend/src/components/workflows/configPanel.tsx (2)
154-218: LGTM!The JSX structure is well-organized with clear sections for trigger configuration, action buttons, and action settings. The conditional rendering based on
triggerDoc?.paramsandactionDoc?.paramscorrectly guards against undefined data.
31-45: This pattern is safe—useFrappeGetDocwith undefined docname is the standard SWR conditional-fetching approach.The hook is built on SWR, which prevents API requests when its key is falsy (undefined/null). No invalid request or error will occur. The code already has proper null checks (
if (triggerDoc)andif (actionDoc)) before accessing the data, which is correct defensive programming.Likely an incorrect or invalid review comment.
frontend/src/stores/editor.ts (3)
141-171:appendNodeassumes linear workflow topology.The function always connects to the last node in the array, which works for linear workflows but may not behave correctly with branching (condition nodes with true/false outputs). Consider if this is the intended behavior for all use cases.
183-208: LGTM!The
updateNodeDatafunction correctly updates both the node inflowNodesand synchronizesselectedNodeif it matches. The immutable update pattern is properly implemented.
55-73: LGTM!The
createFlowNodehelper properly constructs a React Flow node with all required fields and sensible defaults. The use ofgetNodeTypeForDataensures proper node type assignment for condition nodes.
| const position = reactFlowInstance.current.screenToFlowPosition({ | ||
| x: event.clientX - reactFlowBounds.left, | ||
| y: event.clientY - reactFlowBounds.top, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
React Flow screenToFlowPosition input coordinates documentation
💡 Result:
screenToFlowPosition expects a screen (pixel) position object { x:number, y:number } — e.g. pass event.clientX and event.clientY directly. It returns an XYPosition in flow coordinates. An optional options object { snapToGrid: boolean } can be passed. [1][2]
Sources:
[1] React Flow — ReactFlowInstance: screenToFlowPosition signature.
[2] React Flow changelog/example showing usage with event.clientX/event.clientY (replacement for the old project API).
Remove redundant offset subtraction; screenToFlowPosition expects raw screen coordinates.
screenToFlowPosition accepts screen coordinates directly (e.g., event.clientX, event.clientY). Subtracting reactFlowBounds.left and reactFlowBounds.top before passing them converts to container-relative coordinates, which screenToFlowPosition doesn't expect—the method handles offset conversion internally. This causes incorrect positioning.
Proposed fix
const position = reactFlowInstance.current.screenToFlowPosition({
- x: event.clientX - reactFlowBounds.left,
- y: event.clientY - reactFlowBounds.top,
+ x: event.clientX,
+ y: event.clientY,
});🤖 Prompt for AI Agents
In @frontend/src/components/workflows/editor.tsx around lines 101 - 104, The
code incorrectly subtracts reactFlowBounds offsets before calling
reactFlowInstance.current.screenToFlowPosition, causing wrong node positions;
pass raw screen coordinates instead by calling
reactFlowInstance.current.screenToFlowPosition with event.clientX and
event.clientY (use the existing reactFlowInstance.current.screenToFlowPosition
symbol), removing the subtraction of reactFlowBounds.left and
reactFlowBounds.top so the method can perform its internal conversion correctly.
| 'contains': lambda a, b: b in str(a), | ||
| 'not_contains': lambda a, b: b not in str(a), | ||
| 'starts_with': lambda a, b: str(a).startswith(str(b)), | ||
| 'ends_with': lambda a, b: str(a).endswith(str(b)), |
There was a problem hiding this comment.
Handle None values in string operators.
The string operators (contains, not_contains, starts_with, ends_with) use str() coercion which will convert None to the string "None". This could lead to unexpected behavior where None is treated as the literal string "None" rather than an empty or missing value.
Consider adding explicit None checks or documenting this behavior.
🛡️ Proposed fix with None handling
- 'contains': lambda a, b: b in str(a),
- 'not_contains': lambda a, b: b not in str(a),
- 'starts_with': lambda a, b: str(a).startswith(str(b)),
- 'ends_with': lambda a, b: str(a).endswith(str(b)),
+ 'contains': lambda a, b: str(b) in str(a) if a is not None and b is not None else False,
+ 'not_contains': lambda a, b: str(b) not in str(a) if a is not None and b is not None else True,
+ 'starts_with': lambda a, b: str(a).startswith(str(b)) if a is not None and b is not None else False,
+ 'ends_with': lambda a, b: str(a).endswith(str(b)) if a is not None and b is not None else False,🤖 Prompt for AI Agents
In @hazelnode/nodes/actions/condition_node.py around lines 27 - 30, The string
operator lambdas ('contains', 'not_contains', 'starts_with', 'ends_with')
incorrectly coerce None to the literal "None"; update each operator to first
handle None explicitly (e.g., treat None as missing/empty and return False for
containment/starts/ends checks or follow the intended semantic) by checking if a
is None or b is None before calling str() and only then performing the
comparison; modify the lambdas or replace them with small functions
named/located where 'contains', 'not_contains', 'starts_with', and 'ends_with'
are defined so they short-circuit on None instead of converting it to "None".
| try: | ||
| result = op_func(left_value, right_value) | ||
| except Exception: | ||
| result = False |
There was a problem hiding this comment.
Add error logging for failed evaluations.
The bare except Exception silently swallows all errors and returns False, making debugging difficult. At minimum, log the exception before returning a fallback value so operators can diagnose issues like type mismatches or unexpected values.
📝 Proposed fix to add logging
+import frappe
+
# Evaluate the condition
try:
result = op_func(left_value, right_value)
- except Exception:
+ except Exception as e:
+ frappe.log_error(
+ title=f"ConditionNode evaluation failed",
+ message=f"Operator: {op}, Left: {left_value}, Right: {right_value}, Error: {str(e)}"
+ )
result = False🤖 Prompt for AI Agents
In @hazelnode/nodes/actions/condition_node.py around lines 66 - 69, The except
block around the evaluation of op_func(left_value, right_value) silently
swallows errors; change it to catch Exception as e and log the exception before
returning False so failures (type errors, etc.) are visible. Use the
module/class logger (or logging.getLogger(__name__)) and call logger.exception
or logger.error(..., exc_info=True) with context including op_func.__name__ (or
repr(op_func)), left_value and right_value, then keep returning False to
preserve fallback behavior.
Both pip and yarn cache actions were using outdated versions (v2 and v3). Updated to v4 to match the reference CI configuration.
Changed from version-specific mariadb-client-10.6 to generic mariadb-client to avoid package availability issues on newer Ubuntu runners.
- Update pre-commit-config.yaml based on buzz repository - Upgrade pre-commit-hooks to v5.0.0 - Upgrade ruff to v0.8.1 - Add ruff import sorter and linter hooks - Exclude tsconfig files from JSON validation (JSONC format) - Remove prettier/eslint hooks (need project-specific config) - Fix deprecated ruff lint settings in pyproject.toml - Apply ruff formatting to all Python files
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
73-76: Consider potential MariaDB client/server version mismatch.The MariaDB client package has been changed from
mariadb-client-10.6to the genericmariadb-client. While this improves compatibility with newer Ubuntu runners, be aware that:
- The MariaDB service container still uses version 10.6 (line 30)
- The client version installed may differ from 10.6, potentially causing compatibility issues
- Most MariaDB client/server combinations are compatible, but edge cases exist
If issues arise, consider either:
- Testing to confirm the generic client works with the 10.6 server, or
- Upgrading the MariaDB service to match whatever client version Ubuntu provides
#!/bin/bash # Check what mariadb-client version would be installed on ubuntu-latest docker run --rm ubuntu:latest bash -c "apt update && apt-cache policy mariadb-client"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Server
🔇 Additional comments (3)
.github/workflows/ci.yml (3)
46-50: The code is correct. Node.js 24 was released on May 6, 2025, and is available by January 2026.actions/setup-node@v4has been available since October 23, 2023. No changes are required.Likely an incorrect or invalid review comment.
41-44: Python 3.14 is a released and valid version.Python 3.14 was released on October 7, 2025, and as of January 2026, Python 3.14.2 is the latest stable release. Specifying
python-version: "3.14"is valid and will resolve to the latest patch version in the 3.14 release line. This will not cause the workflow to fail.Likely an incorrect or invalid review comment.
52-59: actions/cache@v4 is available and current — the latest release is v4.3.0. No action required; the workflow update is valid.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @hazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.py:
- Around line 152-176: The method _get_next_node_id makes an unnecessary DB call
to frappe.db.get_value (assigning node_type) and contains a misleading comment
about "condition nodes"; remove the frappe.db.get_value call and the node_type
variable, update the comment to reflect that branch-based routing is determined
solely by the result dict (and not by node kind), and keep the existing
branch/default logic using get_outgoing_connections(current_node.node_id,
branch) and get_outgoing_connections(current_node.node_id, 'default') so there
is no DB lookup on each invocation.
In @hazelnode/nodes/actions/create_document_node.py:
- Around line 18-20: The ignore_permissions flag is taken directly from node
params and can let unprivileged triggers bypass checks; when reading
ignore_permissions in create_document_node.py, validate it and only honor True
if the triggering user or workflow owner is a System Manager (e.g., check
frappe.has_role(current_user, "System Manager") or verify workflow creator role)
otherwise force it to False and optionally log/audit the override; update the
code that reads the ignore_permissions param (the variable named
ignore_permissions and the subsequent permission check using
frappe.has_permission) to enforce this restriction and reject or ignore any True
value coming from non-System Manager contexts.
🧹 Nitpick comments (4)
pyproject.toml (1)
24-26: Configuration structure correctly updated to the current Ruff standard.The
selectsetting is correctly placed under[tool.ruff.lint]in pyproject.toml. This aligns with Ruff's configuration structure for modern versions.The current configuration only enables import sorting rules (
"I"). Consider expanding the ruleset to leverage Ruff's linting capabilities (e.g.,"E"for pycodestyle errors,"F"for Pyflakes,"B"for bugbear).hazelnode/nodes/actions/delay_node.py (2)
12-28: Consider wrapping execution in error handling.While the delay parsing has try-except protection, the overall execute method lacks error handling. If context manipulation or the sleep itself encounters issues, exceptions will propagate uncaught.
♻️ Optional: Add defensive error handling
def execute(self, event=None, params=None, context=None): + try: context = context or {} delay_seconds = params.get('delay_seconds', 0) try: delay_seconds = int(delay_seconds) except (ValueError, TypeError): delay_seconds = 0 # Limit delay to prevent abuse (max 60 seconds for synchronous execution) delay_seconds = min(delay_seconds, 60) if delay_seconds > 0: time.sleep(delay_seconds) return context + except Exception as e: + frappe.log_error(f'DelayNode execution failed: {str(e)}') + raise
22-23: 60-second cap may still be too long for synchronous execution.Even with the safety cap at 60 seconds, blocking a request thread for a full minute could cause timeouts or poor user experience. Consider reducing this limit (e.g., 5-10 seconds) or using the note in the docstring to enforce scheduled jobs for longer delays.
hazelnode/nodes/actions/create_document_node.py (1)
29-30: Add error handling for document creation failures.Document insertion can fail due to validation errors, unique constraints, or mandatory field violations. Consider wrapping this in try-except to provide clearer error messages or graceful degradation.
♻️ Add defensive error handling
- doc = frappe.get_doc({'doctype': doctype, **field_values}) - doc.insert(ignore_permissions=ignore_permissions) + try: + doc = frappe.get_doc({'doctype': doctype, **field_values}) + doc.insert(ignore_permissions=ignore_permissions) + except Exception as e: + frappe.throw( + f'Failed to create {doctype} document: {str(e)}' + )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.pre-commit-config.yamlhazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.pyhazelnode/nodes/__init__.pyhazelnode/nodes/actions/create_document_node.pyhazelnode/nodes/actions/delay_node.pyhazelnode/nodes/actions/log_node.pyhazelnode/nodes/actions/set_variable_node.pyhazelnode/nodes/actions/update_document_node.pyhazelnode/nodes/utils.pypyproject.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- hazelnode/nodes/actions/log_node.py
- hazelnode/nodes/actions/update_document_node.py
- hazelnode/nodes/utils.py
- hazelnode/nodes/actions/set_variable_node.py
🧰 Additional context used
🧬 Code graph analysis (2)
hazelnode/nodes/actions/create_document_node.py (3)
hazelnode/nodes/__init__.py (2)
Node(4-6)execute(6-6)hazelnode/nodes/utils.py (2)
ensure_context(63-65)parse_json_field(8-40)hazelnode/nodes/actions/update_document_node.py (1)
execute(14-50)
hazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.py (3)
hazelnode/hazelnode/doctype/hazel_node_connection/hazel_node_connection.py (1)
HazelNodeConnection(7-24)hazelnode/nodes/__init__.py (1)
execute(6-6)hazelnode/hazelnode/doctype/hazel_node/hazel_node.py (1)
execute(31-39)
🔇 Additional comments (12)
pyproject.toml (1)
31-31: LGTM!The
indent-style = "tab"setting is valid and properly configured in the Ruff format section.hazelnode/nodes/__init__.py (1)
6-6: LGTM! Idiomatic abstract method syntax.The single-line ellipsis is the standard Python convention for abstract method bodies.
.pre-commit-config.yaml (4)
1-3: LGTM!The global configuration is correct. Excluding
node_modulesand.gitis standard practice, andfail_fast: falseensures all hooks run to surface all issues in one pass.
34-37: LGTM!The CI configuration is appropriate. Weekly autoupdate schedule keeps hooks current without being too frequent, and the settings are correctly formatted for pre-commit.ci.
7-19: Ensure pre-commit version >= 3.2.0 is enforced.Version v5.0.0 introduces breaking changes:
- Requires pre-commit >= 3.2.0 (check that this dependency is declared in your environment setup or
.pre-commit-config.yaml).- No deprecated stage names are used in this configuration.
Verify the pre-commit version requirement is documented and enforced in the project dependencies.
21-32: The ruff configuration is valid. All hook IDs (ruffandruff-format) exist in v0.8.1, the args syntax is correct, and the three-stage approach (import sorting, linting, formatting) is sound.hazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.py (6)
20-24: LGTM!The type annotations for HazelNodeConnection and the connections field are correctly implemented following Frappe's type-checking patterns.
49-74: LGTM!The helper methods provide clean abstractions for graph navigation. The get_start_node method appropriately handles backward compatibility with a safe fallback to the first node in the array.
76-103: LGTM!The execute method correctly delegates to graph-based or linear execution paths while maintaining proper error handling, rollback on failure, and execution log management.
105-109: LGTM!The linear execution fallback correctly preserves backward compatibility for workflows without connections.
135-150: LGTM!The node execution logic correctly parses parameters, executes the node, logs the results comprehensively, and propagates the output as the new context.
111-133: Thevisitedset preventing node re-execution is the correct and intended behavior for a directed acyclic graph (DAG) workflow execution model. The design explicitly follows connections between nodes and does not support cyclic re-execution. If a node needs to execute multiple times, it should be represented as separate nodes in the workflow graph.
| def _get_next_node_id(self, current_node, result): | ||
| """Get the next node ID based on the current node type and result.""" | ||
| # Check if this is a condition node | ||
| node_type = frappe.db.get_value( | ||
| 'Hazel Node Type', current_node.type, 'kind' | ||
| ) | ||
|
|
||
| # For condition nodes, check the branch in result | ||
| if isinstance(result, dict) and 'branch' in result: | ||
| branch = result.get('branch') | ||
| if branch in ('true', 'false'): | ||
| connections = self.get_outgoing_connections( | ||
| current_node.node_id, branch | ||
| ) | ||
| if connections: | ||
| return connections[0].target_node_id | ||
|
|
||
| # Default: follow the 'default' connection | ||
| connections = self.get_outgoing_connections( | ||
| current_node.node_id, 'default' | ||
| ) | ||
| if connections: | ||
| return connections[0].target_node_id | ||
|
|
||
| return None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the exact file and method to verify the unused variable claim
cat -n hazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.py | sed -n '152,176p'Repository: BuildWithHussain/hazelnode
Length of output: 1024
🏁 Script executed:
# Also check the broader context to see if node_type is used anywhere after line 157
cat -n hazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.py | sed -n '150,180p'Repository: BuildWithHussain/hazelnode
Length of output: 1055
🏁 Script executed:
# Check if there are test cases or documentation about the expected behavior
fd -e py -e json | xargs rg -l "hazel_workflow|_get_next_node_id" | head -20Repository: BuildWithHussain/hazelnode
Length of output: 180
🏁 Script executed:
# Search for how connections are queried and if multiple connections with same handle exist
rg -n "source_handle|source_node_id" hazelnode/Repository: BuildWithHussain/hazelnode
Length of output: 952
🏁 Script executed:
# Find the get_outgoing_connections method definition
rg -n "def get_outgoing_connections" hazelnode/ -A 15Repository: BuildWithHussain/hazelnode
Length of output: 1728
🏁 Script executed:
# Check if there are multiple connections with the same source_node_id and source_handle
rg -n "source_handle" hazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.py -B 2 -A 2Repository: BuildWithHussain/hazelnode
Length of output: 260
🏁 Script executed:
# Check the full hazel_workflow.py file to understand the class structure
wc -l hazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.pyRepository: BuildWithHussain/hazelnode
Length of output: 134
🏁 Script executed:
# Check if there are tests for this method
rg -n "_get_next_node_id|execute_workflow" hazelnode/ -l | grep -i testRepository: BuildWithHussain/hazelnode
Length of output: 52
🏁 Script executed:
# Check the full context of the method and how it's called
rg -n "_get_next_node_id" hazelnode/ -B 2 -A 2Repository: BuildWithHussain/hazelnode
Length of output: 1015
🏁 Script executed:
# Look at the Hazel Node Type schema to understand what 'kind' values exist
fd -e json | xargs rg -l "Hazel Node Type" | head -5Repository: BuildWithHussain/hazelnode
Length of output: 369
🏁 Script executed:
# Check what the different node types and their kinds are
cat -n hazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.py | sed -n '1,60p'Repository: BuildWithHussain/hazelnode
Length of output: 2129
🏁 Script executed:
# Check what node kinds exist
cat hazelnode/fixtures/hazel_node_type.json | head -100Repository: BuildWithHussain/hazelnode
Length of output: 2356
🏁 Script executed:
# Check the Hazel Node Type schema
cat hazelnode/hazelnode/doctype/hazel_node_type/hazel_node_type.jsonRepository: BuildWithHussain/hazelnode
Length of output: 1970
🏁 Script executed:
# Look for any validation or logic that should restrict certain operations to condition nodes
rg -n "kind.*condition|condition.*kind" hazelnode/ -iRepository: BuildWithHussain/hazelnode
Length of output: 52
Remove unused database call.
Lines 155-157 fetch the node kind from the database, but the node_type variable is never used in the method. Additionally, no "Condition" node kind exists in the system—only "Trigger" and "Action" kinds are defined. The comment suggesting conditional routing applies only to condition nodes is misleading since the implementation allows branch-based routing for any node via the result dict check, without validating the node type.
Issue:
- Unnecessary database call creates overhead on every node execution
- Comment references non-existent "condition nodes"
♻️ Proposed fix
def _get_next_node_id(self, current_node, result):
"""Get the next node ID based on the current node type and result."""
- # Check if this is a condition node
- node_type = frappe.db.get_value(
- 'Hazel Node Type', current_node.type, 'kind'
- )
-
- # For condition nodes, check the branch in result
+ # Check the branch in result for branching logic
if isinstance(result, dict) and 'branch' in result:Verify: Are condition nodes planned for future implementation, or should branch-based routing be documented as available for all node types?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _get_next_node_id(self, current_node, result): | |
| """Get the next node ID based on the current node type and result.""" | |
| # Check if this is a condition node | |
| node_type = frappe.db.get_value( | |
| 'Hazel Node Type', current_node.type, 'kind' | |
| ) | |
| # For condition nodes, check the branch in result | |
| if isinstance(result, dict) and 'branch' in result: | |
| branch = result.get('branch') | |
| if branch in ('true', 'false'): | |
| connections = self.get_outgoing_connections( | |
| current_node.node_id, branch | |
| ) | |
| if connections: | |
| return connections[0].target_node_id | |
| # Default: follow the 'default' connection | |
| connections = self.get_outgoing_connections( | |
| current_node.node_id, 'default' | |
| ) | |
| if connections: | |
| return connections[0].target_node_id | |
| return None | |
| def _get_next_node_id(self, current_node, result): | |
| """Get the next node ID based on the current node type and result.""" | |
| # Check the branch in result for branching logic | |
| if isinstance(result, dict) and 'branch' in result: | |
| branch = result.get('branch') | |
| if branch in ('true', 'false'): | |
| connections = self.get_outgoing_connections( | |
| current_node.node_id, branch | |
| ) | |
| if connections: | |
| return connections[0].target_node_id | |
| # Default: follow the 'default' connection | |
| connections = self.get_outgoing_connections( | |
| current_node.node_id, 'default' | |
| ) | |
| if connections: | |
| return connections[0].target_node_id | |
| return None |
🤖 Prompt for AI Agents
In @hazelnode/hazelnode/doctype/hazel_workflow/hazel_workflow.py around lines
152 - 176, The method _get_next_node_id makes an unnecessary DB call to
frappe.db.get_value (assigning node_type) and contains a misleading comment
about "condition nodes"; remove the frappe.db.get_value call and the node_type
variable, update the comment to reflect that branch-based routing is determined
solely by the result dict (and not by node kind), and keep the existing
branch/default logic using get_outgoing_connections(current_node.node_id,
branch) and get_outgoing_connections(current_node.node_id, 'default') so there
is no DB lookup on each invocation.
| # Check permissions before creating document | ||
| ignore_permissions = params.get('ignore_permissions', False) | ||
| if not ignore_permissions and not frappe.has_permission( |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for CreateDocumentNode and ignore_permissions usage
rg -nP --type=py -C5 'CreateDocumentNode|ignore_permissions' -g '!**/test/**'Repository: BuildWithHussain/hazelnode
Length of output: 7540
🏁 Script executed:
#!/bin/bash
# Find where node.execute() is called and how params are passed
rg -nP --type=py 'node\.execute\(|\.execute\(event.*params' -C5
# Also search for where parameters come from in the workflow execution
rg -nP --type=py 'parameters|node_config' -C3 | head -100Repository: BuildWithHussain/hazelnode
Length of output: 3065
🏁 Script executed:
#!/bin/bash
# Check for Hazel Workflow doctype definition and permissions
find . -name "hazel_workflow.json" -o -name "hazel_node.json" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && cat {}'
# Search for any validation of ignore_permissions parameter
rg -nP --type=py 'ignore_permissions.*validate|validate.*ignore_permissions' -C3Repository: BuildWithHussain/hazelnode
Length of output: 4191
Validate or restrict the ignore_permissions parameter in workflows.
The ignore_permissions flag is extracted directly from node parameters without validation. While Hazel Workflow creation is restricted to System Managers, if a workflow configured with ignore_permissions=True is triggered by unprivileged users (via webhooks or other triggers), those users effectively bypass document creation permission checks. Either restrict this parameter to System Manager-only workflows via explicit validation or document this as a System Manager-controlled feature with appropriate warnings.
🤖 Prompt for AI Agents
In @hazelnode/nodes/actions/create_document_node.py around lines 18 - 20, The
ignore_permissions flag is taken directly from node params and can let
unprivileged triggers bypass checks; when reading ignore_permissions in
create_document_node.py, validate it and only honor True if the triggering user
or workflow owner is a System Manager (e.g., check frappe.has_role(current_user,
"System Manager") or verify workflow creator role) otherwise force it to False
and optionally log/audit the override; update the code that reads the
ignore_permissions param (the variable named ignore_permissions and the
subsequent permission check using frappe.has_permission) to enforce this
restriction and reject or ignore any True value coming from non-System Manager
contexts.
| context['created_doc'] = doc.as_dict() | ||
| context['created_doc_name'] = doc.name |
There was a problem hiding this comment.
Context key collision when multiple create nodes execute.
Storing the created document under fixed keys created_doc and created_doc_name means sequential create nodes in a workflow will overwrite each other's results. Consider using unique keys (e.g., based on doctype or node ID) or appending to a list.
♻️ Suggested fix: Use unique context keys
- context['created_doc'] = doc.as_dict()
- context['created_doc_name'] = doc.name
+ # Use doctype-specific keys to avoid collisions
+ context[f'created_{doctype.lower().replace(" ", "_")}'] = doc.as_dict()
+ context[f'created_{doctype.lower().replace(" ", "_")}_name'] = doc.name
+
+ # Also maintain a list of all created documents
+ if 'created_docs' not in context:
+ context['created_docs'] = []
+ context['created_docs'].append({
+ 'doctype': doctype,
+ 'name': doc.name,
+ 'doc': doc.as_dict()
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| context['created_doc'] = doc.as_dict() | |
| context['created_doc_name'] = doc.name | |
| # Use doctype-specific keys to avoid collisions | |
| context[f'created_{doctype.lower().replace(" ", "_")}'] = doc.as_dict() | |
| context[f'created_{doctype.lower().replace(" ", "_")}_name'] = doc.name | |
| # Also maintain a list of all created documents | |
| if 'created_docs' not in context: | |
| context['created_docs'] = [] | |
| context['created_docs'].append({ | |
| 'doctype': doctype, | |
| 'name': doc.name, | |
| 'doc': doc.as_dict() | |
| }) |
| if delay_seconds > 0: | ||
| time.sleep(delay_seconds) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's view the actual DelayNode implementation
cat -n hazelnode/nodes/actions/delay_node.pyRepository: BuildWithHussain/hazelnode
Length of output: 889
🏁 Script executed:
# Check for imports and see how Frappe context is set up
head -30 hazelnode/nodes/actions/delay_node.pyRepository: BuildWithHussain/hazelnode
Length of output: 693
🏁 Script executed:
# Search for usage of DelayNode in the codebase
rg "DelayNode|delay_node" --type py -B 2 -A 2Repository: BuildWithHussain/hazelnode
Length of output: 342
🌐 Web query:
Frappe best practices for implementing time delays without blocking request threads
💡 Result:
Short answer: never sleep in a web request — push work to background workers or the scheduler, and notify the client asynchronously. Key practices:
- Don’t block request threads with time.sleep or long loops. Convert slow/waiting work into background jobs. [1][2]
- Use frappe.enqueue / frappe.enqueue_doc to run tasks in worker queues (short/default/long). Choose queue and timeout appropriately and use enqueue_after_commit if you need DB commit first. [1][2]
- For "run this after N seconds" use scheduled jobs (scheduler_events / scheduler) or a background-worker job that schedules the real work (enqueue from a scheduler) instead of sleeping in the request. The scheduler already uses sensible ticks/jitter; prefer it for delayed execution. [3][1]
- If you need to schedule one-off delays, implement a tiny scheduled task or DB table of pending actions and a worker that enqueues when the timestamp arrives (avoid busy‑loops). This is more reliable and scalable than sleeping in-process. [3]
- Use realtime notifications / websockets (frappe.publish_realtime) or polling to update the client when the background job finishes rather than holding the HTTP response. [1]
- Tune worker configuration and timeouts (queue selection, job timeout) and monitor worker logs / scheduler logs for failures. Add email/alerting for failed jobs. [2][4]
References:
- Frappe docs — Running / Background Jobs (enqueue, queues, enqueue_after_commit). [1]
- Frappe background jobs API / timeouts and queues. [2]
- Frappe blog on scheduler behaviour, ticks and jitter — why scheduler-based delays are preferred. [3]
- Frappe Cloud / FAQ guidance on converting long requests to background jobs and timeouts. [4]
If you want, I can show code examples for: a) enqueueing a delayed job pattern (DB + scheduler worker), b) using enqueue_after_commit, and c) client realtime notification. Which one do you want?
🌐 Web query:
Frappe background jobs vs request thread handling synchronous delays
💡 Result:
Short answer
- Use Frappe background jobs (frappe.enqueue / RQ) for long-running, I/O‑heavy, or unpredictable tasks so web request threads aren’t blocked. Background workers run asynchronously and have configurable queues/timeouts (short, default, long). [1][2]
- Don’t perform long synchronous work inside the request handler: it ties up a web worker (reduces concurrency), risks request timeouts, and hurts latency and scale. Offload and return quickly; use realtime / job status to notify the client. [1][2][3]
Practical guidance
- When to enqueue: heavy CPU work, long I/O (API calls, file processing), bulk document operations, scheduled tasks, or anything > a few hundred ms. Use queue="long" or a custom queue and set timeout. [2]
- When synchronous is ok: cheap DB reads/writes, validation, small updates, or when immediate result is required (<100–300 ms typical).
- How to enqueue: frappe.enqueue(method_or_path, queue="long"/"default"/"short", timeout=…, enqueue_after_commit=True, at_front=True). Use frappe.enqueue_doc for DocType methods. [2][1]
- User feedback: return a response immediately and publish progress via frappe.publish_realtime or expose RQ Job status (RQ Job / RQ Worker doctypes). [1][3]
- Ops: run enough background workers, monitor worker/job metrics (logs, RQ Job, CPU usage), and tune web worker/gunicorn timeouts to your traffic pattern. Frappe Cloud and the framework docs show monitoring options. [3][4]
Risks and caveats
- Background failures must be handled (retries, logging, idempotency). Jobs consume separate CPU/ram so size workers accordingly. [2][3]
- enqueue_after_commit is useful to avoid running jobs on failed DB transactions. [2]
References
- Frappe docs — Running Background Jobs / examples. [1]
- Frappe docs — Background Jobs API, queues, timeouts, enqueue options. [2]
- Frappe docs — Profiling, monitoring and RQ Job/Worker doctypes. [3]
- Frappe Cloud — monitoring background jobs vs request CPU. [4]
If you want, I can: (a) show exact code examples for enqueue + realtime progress, (b) recommend queue/workers sizing for your workload, or (c) check your current site config for timeouts and queue setup. Which do you prefer?
Blocking sleep in request thread violates Frappe architecture and creates severe performance risk.
Using time.sleep() blocks the execution thread synchronously. In Frappe request handling, this ties up a web worker, reduces concurrency, and risks request timeouts—especially under load. The code's comment acknowledges this ("synchronous execution"), yet still implements the blocking pattern.
Use frappe.enqueue() to defer work to a background job queue, or implement as a scheduled task with scheduler_events. For user feedback, publish progress via frappe.publish_realtime() or expose job status asynchronously rather than holding the request.
2 participants
useFrappeCreateDoc, useFrappeUpdateDoc, and useFrappeDeleteDoc
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.