Feat/hitl#250
Conversation
- Add channel selector dropdown to Input node configuration - Add channel selector dropdown to Approval node configuration - Support Email and App (Web UI) channel options - Update TypeScript interfaces to include channel field
- Add loop_back_node_id and loop_back_condition fields to InputNode config - Update review mode to show input fields with required indicators - Add loop_back_node_id and loop_back_condition to HITLRequest interface - Update review mode UI to display configured input fields - Improve handleSubmit to handle loop back conditions
…nding requests pages
WalkthroughAdds Human-in-the-Loop (HITL) end-to-end: metadata parsing/removal utilities, in-chat HITL UI and runtime handling, pending-requests UI (list/detail), WorkflowService HITL API client, editor nodes for approval/input, SSO client, sidebar pending badge, and related editor/webhook tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatUI as Chat UI (Thread/HITLRequestChat)
participant Client as Client (WorkflowService)
participant API as Backend API
participant DB as Database
Note over API,DB: HITL request created by a workflow execution
User->>ChatUI: Opens assistant message containing HITL metadata
ChatUI->>ChatUI: parseHITLMetadata & removeHITLMetadata
alt Metadata parsed
ChatUI->>User: Render HITLRequestChat (approval/input)
end
User->>ChatUI: Submit response (response_data + comment)
ChatUI->>Client: submitHITLResponse(executionId,nodeId,iteration,response)
Client->>API: POST /workflows/hitl/submit
API->>DB: Persist response, update request status, enqueue/resume workflow
DB-->>API: OK
API-->>Client: Success payload
Client-->>ChatUI: Success result
ChatUI->>User: Show success state / update UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Areas to focus review on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 11
🧹 Nitpick comments (14)
app/(main)/layout.tsx (1)
45-48: Consider showing a loading indicator instead of returning null.While returning
nullprevents rendering during authentication checks, it results in a blank screen for users. Consider rendering a loading spinner or skeleton for better UX.Apply this diff to show a loading state:
// Show loading state while checking auth if (user == null) { - return null; + return ( + <div className="flex items-center justify-center h-screen"> + <div className="text-center"> + <div className="h-8 w-8 animate-spin rounded-full border-4 border-primary border-t-transparent mx-auto mb-2" /> + <p className="text-sm text-muted-foreground">Loading...</p> + </div> + </div> + ); }lib/utils/hitlMetadata.ts (1)
31-36: Consider extracting regex patterns to module-level constants.The regex patterns array is defined inside the function on every call. Since these patterns are static, consider extracting them to module-level constants for better performance and readability.
+/** + * Regex patterns for matching HITL metadata in various formats + */ +const HITL_METADATA_PATTERNS = [ + /<!--\s*HITL_METADATA:\s*({[\s\S]*?})\s*-->/g, // Standard format with any whitespace (non-greedy) + /<!--\s*HITL_METADATA:\s*({[\s\S]*})-->/g, // Greedy match for multi-line + /<!--HITL_METADATA:\s*({[\s\S]*?})-->/g, // No spaces around comment + /HITL_METADATA:\s*({[\s\S]*?})(?=\s*-->)/, // More flexible - match before --> +]; + export function parseHITLMetadata(messageText: string): HITLMetadata | null { if (!messageText) return null; - // Look for HITL_METADATA in HTML comment - // Try multiple regex patterns to handle different formatting - const patterns = [ - /<!--\s*HITL_METADATA:\s*({[\s\S]*?})\s*-->/g, - /<!--\s*HITL_METADATA:\s*({[\s\S]*})-->/g, - /<!--HITL_METADATA:\s*({[\s\S]*?})-->/g, - /HITL_METADATA:\s*({[\s\S]*?})(?=\s*-->)/, - ]; - - for (const regex of patterns) { + for (const regex of HITL_METADATA_PATTERNS) {lib/sso/unified-auth.ts (1)
57-65: Extract common authorization header logic to reduce duplication.The Authorization header pattern
{ headers: { Authorization: \Bearer ${firebaseToken}` } }` is repeated across four methods. Consider extracting this to a private helper method to follow DRY principles.export class UnifiedAuthClient { private baseURL: string; constructor(baseURL = API_URL) { this.baseURL = baseURL; } + /** + * Get authorization headers for authenticated requests + */ + private getAuthHeaders(firebaseToken: string) { + return { + headers: { Authorization: `Bearer ${firebaseToken}` }, + }; + } /** * Get user's connected providers */ async getMyProviders(firebaseToken: string): Promise<{ providers: AuthProvider[]; primary_provider?: AuthProvider }> { const response = await axios.get( `${this.baseURL}/api/v1/providers/me`, - { - headers: { Authorization: `Bearer ${firebaseToken}` }, - } + this.getAuthHeaders(firebaseToken) ); return response.data; } /** * Set a provider as primary */ async setPrimaryProvider( firebaseToken: string, providerType: string ): Promise<void> { await axios.post( `${this.baseURL}/api/v1/providers/set-primary`, { provider_type: providerType }, - { - headers: { Authorization: `Bearer ${firebaseToken}` }, - } + this.getAuthHeaders(firebaseToken) ); } // Apply similar changes to unlinkProvider and getAccountAlso applies to: 70-81, 86-97, 102-110
app/(main)/chat/[chatId]/components/HITLRequestChat.tsx (1)
253-259: Consider adding visual loading feedback for better UX.The submit button shows "Submitting..." text when
isSubmittingis true, but adding a loading spinner icon would provide better visual feedback to users.+import { Loader2 } from "lucide-react"; <Button onClick={() => handleSubmit()} disabled={isSubmitting} className="w-full" > - {isSubmitting ? "Submitting..." : "Submit Response"} + {isSubmitting ? ( + <> + <Loader2 className="h-4 w-4 mr-2 animate-spin" /> + Submitting... + </> + ) : ( + "Submit Response" + )} </Button>app/(main)/workflows/components/editor/nodes/manual-steps/input-node.tsx (2)
40-42: State synchronization issue withfields.The
fieldsstate is initialized fromconfig.input_fieldsonly once during mount. If the parent component updatesconfig.input_fieldsexternally (e.g., when loading saved workflow data), this local state won't reflect those changes.Consider synchronizing state with props:
+import { FC, useState, useEffect } from "react"; -import { FC, useState } from "react"; // Inside component: const [fields, setFields] = useState<InputField[]>( config.input_fields || [{ name: "", type: "text", required: false }] ); + +useEffect(() => { + setFields(config.input_fields || [{ name: "", type: "text", required: false }]); +}, [config.input_fields]);
186-196: Add radix parameter toparseIntand validate timeout bounds.The
parseIntcall on line 189 should include a radix parameter for clarity. Additionally, consider enforcing the min/max bounds (1-168) programmatically to prevent invalid values.onChange={(e) => - handleChange("timeout_hours", parseInt(e.target.value) || 24) + handleChange("timeout_hours", Math.min(168, Math.max(1, parseInt(e.target.value, 10) || 24))) }app/(main)/workflows/pending-requests/page.tsx (2)
72-77: Missing dependency inuseEffectmay cause stale closures.The
loadRequestsfunction is called in the effect and interval but is not included in the dependency array. WhilepageandpageSizeare listed,loadRequestsreferences other state setters that could cause issues.Consider wrapping
loadRequestswithuseCallbackor moving the fetch logic inside the effect:+import { useEffect, useState, useCallback } from "react"; -import { useEffect, useState } from "react"; // Then wrap loadRequests: +const loadRequests = useCallback(async () => { -const loadRequests = async () => { // ... existing implementation -}; +}, [page, pageSize]); // Update effect deps: useEffect(() => { loadRequests(); const interval = setInterval(loadRequests, 30000); return () => clearInterval(interval); -}, [page, pageSize]); +}, [loadRequests]);
735-818: Significant code duplication in field rendering.The input field rendering logic (text, number, select, multi_select, file) is duplicated between the review mode section (lines 735-818) and standard input mode section (lines 858-950). This makes maintenance harder and increases the chance of inconsistencies.
Extract a reusable
FieldRenderercomponent:const FieldRenderer = ({ field, value, onChange, disabled }: { field: HITLRequest['fields'][0]; value: any; onChange: (value: any) => void; disabled: boolean; }) => { switch (field.type) { case "text": return <Textarea value={value || ""} onChange={(e) => onChange(e.target.value)} disabled={disabled} />; case "number": return <Input type="number" value={value || ""} onChange={(e) => onChange(parseFloat(e.target.value) || 0)} disabled={disabled} />; // ... other cases } };Then use it in both sections to reduce duplication.
Also applies to: 858-950
app/(main)/workflows/pending-requests/[requestId]/page.tsx (4)
40-44: Missing dependency in useEffect.The
loadRequestfunction is called inside the effect but not included in the dependency array. This can lead to stale closures. Consider either moving the fetch logic inside the effect or wrappingloadRequestwithuseCallback.+import { useEffect, useState, useCallback } from "react"; ... - const loadRequest = async () => { + const loadRequest = useCallback(async () => { try { setLoading(true); ... } - }; + }, [executionId, nodeId, iteration]); useEffect(() => { if (executionId && nodeId) { loadRequest(); } - }, [executionId, nodeId, iteration]); + }, [executionId, nodeId, loadRequest]);
470-550: Extract duplicate field rendering to a reusable component.The field rendering logic here (lines 470-550) is nearly identical to the review mode block (lines 344-427). Extracting this to a shared
FieldRenderercomponent would reduce duplication and improve maintainability.// Example extraction: const FieldRenderer: FC<{ field: { name: string; type: string; required?: boolean; options?: string[] }; value: any; onChange: (value: any) => void; disabled: boolean; }> = ({ field, value, onChange, disabled }) => { // Render based on field.type... };
486-494: Number input cannot be cleared.Using
parseFloat(e.target.value) || 0means users cannot clear the field—it will always reset to0. Consider allowing empty values for optional number fields:-onChange={(e) => handleFieldChange(field.name, parseFloat(e.target.value) || 0)} +onChange={(e) => handleFieldChange(field.name, e.target.value === "" ? "" : parseFloat(e.target.value))}
116-157: Inconsistent indentation makes control flow unclear.The nested
if-elseblocks have misaligned indentation, making it difficult to follow the control flow. Theelseat line 152 pairs with theifat line 116, but the indentation suggests otherwise.Consider reformatting for clarity:
- if (request.fields && request.fields.length > 0) { - // Validate required fields + if (request.fields && request.fields.length > 0) { + // Validate required fields for (const field of request.fields) {app/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx (2)
18-23: Unusedworkflowprop and weak typing.The
workflowprop is typed asanyand is not used in the component. Consider removing it if unnecessary, or providing a proper type if it's intended for future use.interface ApprovalConfigProps { config: ApprovalNodeData; onConfigChange: (config: ApprovalNodeData) => void; readOnly?: boolean; - workflow?: any; } export const ApprovalConfigComponent: FC<ApprovalConfigProps> = ({ config, onConfigChange, readOnly = false, - workflow, }) => {
93-104: Consider using a UI library Select component for consistency.Native
<select>elements are used here while other inputs use UI library components (Input,Textarea). For visual consistency with the design system, consider using aSelectcomponent from the UI library if available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/(main)/chat/[chatId]/components/HITLRequestChat.tsx(1 hunks)app/(main)/chat/[chatId]/components/Thread.tsx(6 hunks)app/(main)/layout.tsx(2 hunks)app/(main)/workflows/[workflowId]/executions/[executionId]/page.tsx(4 hunks)app/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx(1 hunks)app/(main)/workflows/components/editor/nodes/manual-steps/input-node.tsx(1 hunks)app/(main)/workflows/components/editor/nodes/node-registry.ts(3 hunks)app/(main)/workflows/components/editor/nodes/node.tsx(2 hunks)app/(main)/workflows/pending-requests/[requestId]/page.tsx(1 hunks)app/(main)/workflows/pending-requests/page.tsx(1 hunks)components/ui/command.tsx(7 hunks)lib/sso/unified-auth.ts(1 hunks)lib/utils.ts(1 hunks)lib/utils/hitlMetadata.ts(1 hunks)services/WorkflowService.ts(2 hunks)types/auth.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use PascalCase for component names and camelCase for utility names
Order imports: React/Next imports first, then components, then utilities
Use Next.js error boundaries, optional chaining, and nullish coalescing for error handling
Files:
lib/utils/hitlMetadata.tsapp/(main)/workflows/components/editor/nodes/node-registry.tsapp/(main)/workflows/components/editor/nodes/node.tsxtypes/auth.tscomponents/ui/command.tsxapp/(main)/layout.tsxapp/(main)/workflows/pending-requests/page.tsxlib/utils.tsapp/(main)/workflows/components/editor/nodes/manual-steps/input-node.tsxapp/(main)/workflows/pending-requests/[requestId]/page.tsxapp/(main)/chat/[chatId]/components/HITLRequestChat.tsxapp/(main)/chat/[chatId]/components/Thread.tsxservices/WorkflowService.tsapp/(main)/workflows/[workflowId]/executions/[executionId]/page.tsxapp/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsxlib/sso/unified-auth.ts
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.tsx: Use Tailwind CSS for styling and thecnutility for conditional classes
Use functional components with TypeScript FC types
Prefer destructuring props at the component level
Use the "use client" directive for client components
Files:
app/(main)/workflows/components/editor/nodes/node.tsxcomponents/ui/command.tsxapp/(main)/layout.tsxapp/(main)/workflows/pending-requests/page.tsxapp/(main)/workflows/components/editor/nodes/manual-steps/input-node.tsxapp/(main)/workflows/pending-requests/[requestId]/page.tsxapp/(main)/chat/[chatId]/components/HITLRequestChat.tsxapp/(main)/chat/[chatId]/components/Thread.tsxapp/(main)/workflows/[workflowId]/executions/[executionId]/page.tsxapp/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx
🧠 Learnings (2)
📚 Learning: 2025-07-22T09:20:21.427Z
Learnt from: CR
Repo: potpie-ai/potpie-ui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T09:20:21.427Z
Learning: Applies to **/*.{ts,tsx} : Use Next.js error boundaries, optional chaining, and nullish coalescing for error handling
Applied to files:
components/ui/command.tsx
📚 Learning: 2025-07-22T09:20:21.427Z
Learnt from: CR
Repo: potpie-ai/potpie-ui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T09:20:21.427Z
Learning: Use Redux Toolkit with Redux Persist for global state management, and React hooks for local state
Applied to files:
app/(main)/layout.tsx
🧬 Code graph analysis (7)
app/(main)/workflows/components/editor/nodes/node-registry.ts (2)
app/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx (1)
approvalNodeMetadata(130-138)app/(main)/workflows/components/editor/nodes/manual-steps/input-node.tsx (1)
inputNodeMetadata(266-274)
app/(main)/workflows/pending-requests/page.tsx (4)
app/utils/queryClient.ts (1)
queryClient(3-10)contexts/AuthContext.tsx (1)
useAuthContext(14-14)services/WorkflowService.ts (3)
HITLRequest(69-95)WorkflowService(249-940)HITLResponseRequest(97-100)lib/utils.ts (2)
formatLocalTime(152-159)formatRelativeTime(166-170)
app/(main)/workflows/components/editor/nodes/manual-steps/input-node.tsx (6)
components/ui/input.tsx (1)
Input(25-25)components/ui/button.tsx (1)
Button(56-56)app/(main)/workflows/components/editor/nodes/node-registry.ts (1)
inputNodeMetadata(90-90)services/WorkflowService.ts (1)
WorkflowNode(133-140)app/(main)/workflows/components/editor/nodes/color_utils.ts (1)
getNodeColors(14-53)app/(main)/workflows/components/editor/handles.tsx (2)
TargetHandle(38-67)SourceHandle(7-36)
app/(main)/workflows/pending-requests/[requestId]/page.tsx (4)
app/utils/queryClient.ts (1)
queryClient(3-10)contexts/AuthContext.tsx (1)
useAuthContext(14-14)services/WorkflowService.ts (3)
HITLRequest(69-95)WorkflowService(249-940)HITLResponseRequest(97-100)lib/utils.ts (1)
formatRelativeTime(166-170)
app/(main)/chat/[chatId]/components/HITLRequestChat.tsx (2)
lib/utils/hitlMetadata.ts (1)
HITLMetadata(5-20)services/WorkflowService.ts (2)
HITLResponseRequest(97-100)WorkflowService(249-940)
app/(main)/workflows/[workflowId]/executions/[executionId]/page.tsx (1)
services/WorkflowService.ts (2)
HITLRequest(69-95)WorkflowService(249-940)
lib/sso/unified-auth.ts (1)
types/auth.ts (3)
SSOLoginResponse(22-31)AuthProvider(11-20)UserAccount(33-43)
🪛 Biome (2.1.2)
app/(main)/workflows/pending-requests/page.tsx
[error] 112-113: Expected a statement but instead found '<<<<<<< HEAD'.
Expected a statement here.
(parse)
[error] 117-118: Expected a statement but instead found '>>>>>>> f393226'.
Expected a statement here.
(parse)
app/(main)/workflows/pending-requests/[requestId]/page.tsx
[error] 182-183: Expected a statement but instead found '<<<<<<< HEAD'.
Expected a statement here.
(parse)
🔇 Additional comments (16)
lib/utils.ts (1)
214-227: LGTM! Good defensive programming.The changes add proper guards for missing
fieldNameornodeIdand handle non-stringfieldNamevalues gracefully by converting them to strings. This prevents runtime errors and improves error message robustness.types/auth.ts (1)
1-44: LGTM! Clean and well-structured type definitions.The type definitions are clear, use appropriate string literal unions for type safety, and properly mark optional fields. The structure aligns well with the SSO authentication flow described in the PR objectives.
components/ui/command.tsx (1)
16-16: Reassess whether the null-to-undefined ref conversion is necessary.The conversion from
ref={ref}toref={ref === null ? undefined : ref}appears to be defensive coding without documented justification. cmdk forwards props including ref to appropriate elements with no specific requirement for this pattern. React's standard forwardRef usage passes refs directly without such conversion, and React handles both null and undefined equivalently. Consider reverting to the standardref={ref}pattern across all affected lines (16, 47, 64, 77, 90, 106, 118) unless there is a specific runtime issue this conversion resolves.app/(main)/workflows/components/editor/nodes/node.tsx (1)
7-8: LGTM!The new imports and switch case for
manual_stepcategory follow the established pattern used for triggers and agents. The fallback toApprovalNodefor unknown manual step types is a sensible default.Also applies to: 33-42
app/(main)/workflows/components/editor/nodes/node-registry.ts (1)
37-38: LGTM!The new
approvalNodeMetadataandinputNodeMetadataare correctly imported, registered inavailableNodes, and re-exported following the established pattern for other node types.Also applies to: 72-73, 89-90
app/(main)/workflows/components/editor/nodes/manual-steps/input-node.tsx (1)
266-311: LGTM!The
inputNodeMetadataexport andInputNodecomponent follow the established patterns from other node implementations. The node correctly usesgetNodeColors,TargetHandle, andSourceHandlefor consistent visual styling.services/WorkflowService.ts (2)
68-100: LGTM!The
HITLRequestandHITLResponseRequestinterfaces are well-defined with appropriate optional fields for the HITL workflow requirements.
835-939: LGTM with a minor note.The HITL API methods (
listHITLRequests,getHITLRequest,submitHITLResponse,deleteHITLRequest) follow the established patterns in this service. TheerrorMessagevariable is assigned but unused in catch blocks - this is consistent with other methods in the file (likely intended for future logging/toast integration).app/(main)/workflows/pending-requests/page.tsx (1)
802-815: File input only captures filename, not actual file content.The file input handler only stores the filename (
file.name) rather than the actual file data. This likely won't work for actual file uploads.Verify if this is intentional. If actual file upload is needed, you'll need to handle file reading (e.g., base64 encoding) or use FormData for multipart uploads:
// Example: Read file as base64 const reader = new FileReader(); reader.onload = () => handleFieldChange(field.name, reader.result); reader.readAsDataURL(file);Also applies to: 926-939
app/(main)/workflows/pending-requests/[requestId]/page.tsx (3)
411-424: File input only captures filename, not file content.The file input handler stores only
file.name, not the actual file data. If file uploads are required, the file content needs to be read (e.g., viaFileReader) or uploaded separately. If this is intentional (e.g., filename is sufficient for the backend), please verify this aligns with the API expectations.
207-221: LGTM!The loading skeleton provides good UX feedback with appropriate animation.
244-272: LGTM!The status determination and badge rendering logic handles edge cases well, including both explicit expired status and calculated expiration from
time_remaining_seconds.app/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx (4)
31-44: LGTM!The change handlers correctly guard against modifications in read-only mode, and the approvers parsing properly trims whitespace and filters empty entries.
76-89: LGTM!Timeout input has appropriate browser-level validation with
min/maxattributes and a sensible fallback default.
140-181: LGTM!The
ApprovalNodecomponent is well-structured with proper conditional rendering for optional fields and correct handle placement for workflow wiring.
130-138: LGTM!The node metadata is properly structured for registry integration with all required fields.
| const hitlMetadata = useMemo(() => { | ||
| // Try parsing from current text state | ||
| const parsedFromText = parseHITLMetadata(text); | ||
| if (parsedFromText) { | ||
| console.log("✅ [HITL] Parsed metadata from text state:", parsedFromText); | ||
| return parsedFromText; | ||
| } | ||
| // Fallback: check initial message content if runtime text doesn't have it | ||
| const parsedFromInitial = parseHITLMetadata(initialText); | ||
| if (parsedFromInitial) { | ||
| console.log("✅ [HITL] Parsed metadata from initial text:", parsedFromInitial); | ||
| return parsedFromInitial; | ||
| } | ||
| // Debug: log if we see HITL keywords but couldn't parse | ||
| if ((text || initialText).includes("HITL_METADATA") || (text || initialText).includes("hitl_request_id")) { | ||
| console.warn("⚠️ [HITL] HITL keywords found but metadata not parsed. Text length:", (text || initialText).length); | ||
| } | ||
| return null; | ||
| }, [text, initialText]); | ||
|
|
||
| const displayText = useMemo(() => { | ||
| const textToUse = text || initialText; | ||
| if (hitlMetadata) { | ||
| const cleaned = removeHITLMetadata(textToUse); | ||
| console.log("🧹 [HITL] Removed metadata from display text. Original length:", textToUse.length, "Cleaned length:", cleaned.length); | ||
| return cleaned; | ||
| } | ||
| return textToUse; | ||
| }, [text, initialText, hitlMetadata]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove or conditionalize console logs in production code.
Similar to the issue in lib/utils/hitlMetadata.ts, this component contains multiple console.log statements (lines 467, 473, 478, 487) that will run in production. Consider removing them or placing them behind a debug flag.
+const DEBUG_HITL = process.env.NEXT_PUBLIC_DEBUG_HITL === 'true';
+
const hitlMetadata = useMemo(() => {
// Try parsing from current text state
const parsedFromText = parseHITLMetadata(text);
if (parsedFromText) {
- console.log("✅ [HITL] Parsed metadata from text state:", parsedFromText);
+ if (DEBUG_HITL) console.log("✅ [HITL] Parsed metadata from text state:", parsedFromText);
return parsedFromText;
}
// Fallback: check initial message content if runtime text doesn't have it
const parsedFromInitial = parseHITLMetadata(initialText);
if (parsedFromInitial) {
- console.log("✅ [HITL] Parsed metadata from initial text:", parsedFromInitial);
+ if (DEBUG_HITL) console.log("✅ [HITL] Parsed metadata from initial text:", parsedFromInitial);
return parsedFromInitial;
}
// Debug: log if we see HITL keywords but couldn't parse
if ((text || initialText).includes("HITL_METADATA") || (text || initialText).includes("hitl_request_id")) {
- console.warn("⚠️ [HITL] HITL keywords found but metadata not parsed. Text length:", (text || initialText).length);
+ if (DEBUG_HITL) console.warn("⚠️ [HITL] HITL keywords found but metadata not parsed. Text length:", (text || initialText).length);
}
return null;
}, [text, initialText]);
const displayText = useMemo(() => {
const textToUse = text || initialText;
if (hitlMetadata) {
const cleaned = removeHITLMetadata(textToUse);
- console.log("🧹 [HITL] Removed metadata from display text. Original length:", textToUse.length, "Cleaned length:", cleaned.length);
+ if (DEBUG_HITL) console.log("🧹 [HITL] Removed metadata from display text. Original length:", textToUse.length, "Cleaned length:", cleaned.length);
return cleaned;
}
return textToUse;
}, [text, initialText, hitlMetadata]);Apply similar changes to lines 516-520 and other console statements.
📝 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.
| const hitlMetadata = useMemo(() => { | |
| // Try parsing from current text state | |
| const parsedFromText = parseHITLMetadata(text); | |
| if (parsedFromText) { | |
| console.log("✅ [HITL] Parsed metadata from text state:", parsedFromText); | |
| return parsedFromText; | |
| } | |
| // Fallback: check initial message content if runtime text doesn't have it | |
| const parsedFromInitial = parseHITLMetadata(initialText); | |
| if (parsedFromInitial) { | |
| console.log("✅ [HITL] Parsed metadata from initial text:", parsedFromInitial); | |
| return parsedFromInitial; | |
| } | |
| // Debug: log if we see HITL keywords but couldn't parse | |
| if ((text || initialText).includes("HITL_METADATA") || (text || initialText).includes("hitl_request_id")) { | |
| console.warn("⚠️ [HITL] HITL keywords found but metadata not parsed. Text length:", (text || initialText).length); | |
| } | |
| return null; | |
| }, [text, initialText]); | |
| const displayText = useMemo(() => { | |
| const textToUse = text || initialText; | |
| if (hitlMetadata) { | |
| const cleaned = removeHITLMetadata(textToUse); | |
| console.log("🧹 [HITL] Removed metadata from display text. Original length:", textToUse.length, "Cleaned length:", cleaned.length); | |
| return cleaned; | |
| } | |
| return textToUse; | |
| }, [text, initialText, hitlMetadata]); | |
| const DEBUG_HITL = process.env.NEXT_PUBLIC_DEBUG_HITL === 'true'; | |
| const hitlMetadata = useMemo(() => { | |
| // Try parsing from current text state | |
| const parsedFromText = parseHITLMetadata(text); | |
| if (parsedFromText) { | |
| if (DEBUG_HITL) console.log("✅ [HITL] Parsed metadata from text state:", parsedFromText); | |
| return parsedFromText; | |
| } | |
| // Fallback: check initial message content if runtime text doesn't have it | |
| const parsedFromInitial = parseHITLMetadata(initialText); | |
| if (parsedFromInitial) { | |
| if (DEBUG_HITL) console.log("✅ [HITL] Parsed metadata from initial text:", parsedFromInitial); | |
| return parsedFromInitial; | |
| } | |
| // Debug: log if we see HITL keywords but couldn't parse | |
| if ((text || initialText).includes("HITL_METADATA") || (text || initialText).includes("hitl_request_id")) { | |
| if (DEBUG_HITL) console.warn("⚠️ [HITL] HITL keywords found but metadata not parsed. Text length:", (text || initialText).length); | |
| } | |
| return null; | |
| }, [text, initialText]); | |
| const displayText = useMemo(() => { | |
| const textToUse = text || initialText; | |
| if (hitlMetadata) { | |
| const cleaned = removeHITLMetadata(textToUse); | |
| if (DEBUG_HITL) console.log("🧹 [HITL] Removed metadata from display text. Original length:", textToUse.length, "Cleaned length:", cleaned.length); | |
| return cleaned; | |
| } | |
| return textToUse; | |
| }, [text, initialText, hitlMetadata]); |
🤖 Prompt for AI Agents
In app/(main)/chat/[chatId]/components/Thread.tsx around lines 463-491 (and also
address console statements around 516-520), there are multiple
console.log/console.warn calls that will execute in production; remove these
console statements or wrap them behind a runtime debug flag (e.g., check
process.env.NODE_ENV !== 'production' or a dedicated isDebug/isDev boolean), or
replace them with the project's centralized logger that respects log levels;
ensure the conditional prevents any logging in production and keep the existing
messages intact for dev-only usage.
| // Handle redirect to sign-in in useEffect to avoid setState during render | ||
| useEffect(() => { | ||
| if (user == null) { | ||
| // Preserve all query parameters when redirecting to sign-in | ||
| const queryString = searchParams.toString(); | ||
| const redirectPath = queryString ? `${pathname}?${queryString}` : pathname; | ||
|
|
||
| if (repo && branch) { | ||
| dispatch(setRepoName(repo)); | ||
| dispatch(setBranchName(branch)); | ||
| } | ||
|
|
||
| router.push(`/sign-in?redirect=${encodeURIComponent(redirectPath)}`); | ||
| } | ||
|
|
||
| router.push(`/sign-in?redirect=${encodeURIComponent(redirectPath)}`); | ||
| }, [user, pathname, searchParams, router, dispatch, repo, branch]); |
There was a problem hiding this comment.
Reconsider dispatching Redux actions before redirect when user is null.
The current logic dispatches setRepoName and setBranchName within the redirect effect when user == null. This means repo/branch state is being set during an unauthenticated flow, which may not align with the intended state management strategy. Consider whether these actions should only be dispatched after successful authentication or if they should be deferred until the user returns from sign-in.
Additionally, ensure that the redirect doesn't create a loop if AuthContext takes time to initialize or if the sign-in page also uses this layout.
Apply this diff to conditionally dispatch only when both user exists and repo/branch are present:
// Handle redirect to sign-in in useEffect to avoid setState during render
useEffect(() => {
+ // Set repo/branch state if user is authenticated
+ if (user && repo && branch) {
+ dispatch(setRepoName(repo));
+ dispatch(setBranchName(branch));
+ }
+
if (user == null) {
// Preserve all query parameters when redirecting to sign-in
const queryString = searchParams.toString();
const redirectPath = queryString ? `${pathname}?${queryString}` : pathname;
-
- if (repo && branch) {
- dispatch(setRepoName(repo));
- dispatch(setBranchName(branch));
- }
-
router.push(`/sign-in?redirect=${encodeURIComponent(redirectPath)}`);
}
}, [user, pathname, searchParams, router, dispatch, repo, branch]);🤖 Prompt for AI Agents
In app/(main)/layout.tsx around lines 29 to 43, stop dispatching setRepoName and
setBranchName when user is null and only dispatch those actions when a valid
authenticated user exists and both repo and branch are present; keep the
redirect for unauthenticated users but guard it to avoid loops by skipping
redirect if the current pathname is the sign-in page or if an auth-initializing
flag is true; update the effect to conditionally branch: (1) if user is truthy
and repo && branch then dispatch the two actions, (2) else if user is null and
not on /sign-in and auth is initialized then perform the router.push redirect
preserving query params; ensure the effect dependencies include the
auth-initializing flag and remove unnecessary dispatches during unauthenticated
flow.
| async ssoLogin( | ||
| email: string, | ||
| ssoProvider: 'google' | 'azure', | ||
| idToken: string, | ||
| providerData?: Record<string, any> | ||
| ): Promise<SSOLoginResponse> { | ||
| const response = await axios.post<SSOLoginResponse>( | ||
| `${this.baseURL}/api/v1/sso/login`, | ||
| { | ||
| email, | ||
| sso_provider: ssoProvider, | ||
| id_token: idToken, | ||
| provider_data: providerData, | ||
| } | ||
| ); | ||
| return response.data; | ||
| } |
There was a problem hiding this comment.
Add error handling and request timeout configuration.
The ssoLogin method (and all other methods in this class) lack error handling and timeout configuration. Network requests can fail or hang, and these methods will throw unhandled axios errors. Consider adding:
- Error handling with user-friendly error messages
- Request timeouts to prevent hanging requests
- Consistent error response format
Apply this diff to add error handling and timeout:
+import { parseApiError } from '@/lib/utils';
+
const API_URL = process.env.NEXT_PUBLIC_API_URL || 'http://localhost:8000';
export class UnifiedAuthClient {
private baseURL: string;
+ private timeout: number = 10000; // 10 seconds
constructor(baseURL = API_URL) {
this.baseURL = baseURL;
}
/**
* Authenticate via SSO provider
*/
async ssoLogin(
email: string,
ssoProvider: 'google' | 'azure',
idToken: string,
providerData?: Record<string, any>
): Promise<SSOLoginResponse> {
- const response = await axios.post<SSOLoginResponse>(
- `${this.baseURL}/api/v1/sso/login`,
- {
- email,
- sso_provider: ssoProvider,
- id_token: idToken,
- provider_data: providerData,
- }
- );
- return response.data;
+ try {
+ const response = await axios.post<SSOLoginResponse>(
+ `${this.baseURL}/api/v1/sso/login`,
+ {
+ email,
+ sso_provider: ssoProvider,
+ id_token: idToken,
+ provider_data: providerData,
+ },
+ { timeout: this.timeout }
+ );
+ return response.data;
+ } catch (error) {
+ console.error('SSO login failed:', error);
+ throw new Error(parseApiError(error));
+ }
}Apply similar patterns to all other methods in this class.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/sso/unified-auth.ts around lines 16 to 32, the ssoLogin method lacks
error handling and a request timeout; wrap the axios.post call in a try/catch,
pass a timeout in the request options (e.g. { timeout: 5000 }), and normalize
errors in the catch by throwing a consistent Error (or a custom error object)
that includes a friendly message and any available axios error details (response
status/body or message) so callers receive a predictable error shape; apply the
same try/catch + timeout + normalized error pattern to all other methods in this
class.
| export function parseHITLMetadata(messageText: string): HITLMetadata | null { | ||
| if (!messageText) return null; | ||
|
|
||
| // Look for HITL_METADATA in HTML comment | ||
| // Try multiple regex patterns to handle different formatting | ||
| const patterns = [ | ||
| /<!--\s*HITL_METADATA:\s*({[\s\S]*?})\s*-->/g, // Standard format with any whitespace (non-greedy) | ||
| /<!--\s*HITL_METADATA:\s*({[\s\S]*})-->/g, // Greedy match for multi-line | ||
| /<!--HITL_METADATA:\s*({[\s\S]*?})-->/g, // No spaces around comment | ||
| /HITL_METADATA:\s*({[\s\S]*?})(?=\s*-->)/, // More flexible - match before --> | ||
| ]; | ||
|
|
||
| for (const regex of patterns) { | ||
| // Reset regex lastIndex for global patterns | ||
| if (regex.global) { | ||
| regex.lastIndex = 0; | ||
| } | ||
| const match = messageText.match(regex); | ||
| if (match && match[1]) { | ||
| try { | ||
| // Clean up the JSON string (remove any trailing whitespace or characters) | ||
| let jsonStr = match[1].trim(); | ||
| // Ensure it's valid JSON by finding the matching closing brace | ||
| const openBraces = (jsonStr.match(/{/g) || []).length; | ||
| const closeBraces = (jsonStr.match(/}/g) || []).length; | ||
| if (openBraces > closeBraces) { | ||
| // Find the last closing brace | ||
| const lastBraceIndex = jsonStr.lastIndexOf('}'); | ||
| if (lastBraceIndex > 0) { | ||
| jsonStr = jsonStr.substring(0, lastBraceIndex + 1); | ||
| } | ||
| } | ||
| const metadata = JSON.parse(jsonStr); | ||
| console.log("✅ [HITL] Parsed metadata successfully:", metadata); | ||
| return metadata as HITLMetadata; | ||
| } catch (error) { | ||
| console.warn("⚠️ [HITL] Error parsing JSON from match:", error, "Match preview:", match[1]?.substring(0, 150)); | ||
| // Continue to next pattern | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Debug: log if we're looking at a message that might contain HITL metadata | ||
| if (messageText.includes("HITL_METADATA") || messageText.includes("hitl_request_id")) { | ||
| const hitlIndex = messageText.indexOf("HITL_METADATA"); | ||
| const preview = messageText.substring( | ||
| Math.max(0, hitlIndex - 50), | ||
| Math.min(messageText.length, hitlIndex + 600) | ||
| ); | ||
| console.warn("⚠️ [HITL] HITL metadata found in text but regex didn't match. Preview:", preview); | ||
| // Try to manually extract JSON | ||
| const jsonStart = preview.indexOf('{'); | ||
| const jsonEnd = preview.lastIndexOf('}'); | ||
| if (jsonStart > 0 && jsonEnd > jsonStart) { | ||
| try { | ||
| const manualJson = preview.substring(jsonStart, jsonEnd + 1); | ||
| const metadata = JSON.parse(manualJson); | ||
| console.log("✅ [HITL] Manually extracted and parsed metadata:", metadata); | ||
| return metadata as HITLMetadata; | ||
| } catch (e) { | ||
| console.warn("⚠️ [HITL] Manual extraction also failed:", e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider removing or conditionalizing console logs in production code.
The parseHITLMetadata function contains multiple console.log and console.warn statements (lines 59, 62, 75, 83, 86) that will run in production. While helpful for debugging during development, these logs may expose internal implementation details and clutter the console in production environments.
Consider one of the following approaches:
- Remove the console logs entirely
- Use a debug flag to conditionally log
- Use a proper logging library that supports log levels
Option 1: Add a debug flag
+const DEBUG_HITL = process.env.NEXT_PUBLIC_DEBUG_HITL === 'true';
+
export function parseHITLMetadata(messageText: string): HITLMetadata | null {
if (!messageText) return null;
// Look for HITL_METADATA in HTML comment
// Try multiple regex patterns to handle different formatting
const patterns = [
/<!--\s*HITL_METADATA:\s*({[\s\S]*?})\s*-->/g,
/<!--\s*HITL_METADATA:\s*({[\s\S]*})-->/g,
/<!--HITL_METADATA:\s*({[\s\S]*?})-->/g,
/HITL_METADATA:\s*({[\s\S]*?})(?=\s*-->)/,
];
for (const regex of patterns) {
if (regex.global) {
regex.lastIndex = 0;
}
const match = messageText.match(regex);
if (match && match[1]) {
try {
let jsonStr = match[1].trim();
const openBraces = (jsonStr.match(/{/g) || []).length;
const closeBraces = (jsonStr.match(/}/g) || []).length;
if (openBraces > closeBraces) {
const lastBraceIndex = jsonStr.lastIndexOf('}');
if (lastBraceIndex > 0) {
jsonStr = jsonStr.substring(0, lastBraceIndex + 1);
}
}
const metadata = JSON.parse(jsonStr);
- console.log("✅ [HITL] Parsed metadata successfully:", metadata);
+ if (DEBUG_HITL) console.log("✅ [HITL] Parsed metadata successfully:", metadata);
return metadata as HITLMetadata;
} catch (error) {
- console.warn("⚠️ [HITL] Error parsing JSON from match:", error, "Match preview:", match[1]?.substring(0, 150));
+ if (DEBUG_HITL) console.warn("⚠️ [HITL] Error parsing JSON from match:", error, "Match preview:", match[1]?.substring(0, 150));
}
}
}
if (messageText.includes("HITL_METADATA") || messageText.includes("hitl_request_id")) {
const hitlIndex = messageText.indexOf("HITL_METADATA");
const preview = messageText.substring(
Math.max(0, hitlIndex - 50),
Math.min(messageText.length, hitlIndex + 600)
);
- console.warn("⚠️ [HITL] HITL metadata found in text but regex didn't match. Preview:", preview);
+ if (DEBUG_HITL) console.warn("⚠️ [HITL] HITL metadata found in text but regex didn't match. Preview:", preview);
const jsonStart = preview.indexOf('{');
const jsonEnd = preview.lastIndexOf('}');
if (jsonStart > 0 && jsonEnd > jsonStart) {
try {
const manualJson = preview.substring(jsonStart, jsonEnd + 1);
const metadata = JSON.parse(manualJson);
- console.log("✅ [HITL] Manually extracted and parsed metadata:", metadata);
+ if (DEBUG_HITL) console.log("✅ [HITL] Manually extracted and parsed metadata:", metadata);
return metadata as HITLMetadata;
} catch (e) {
- console.warn("⚠️ [HITL] Manual extraction also failed:", e);
+ if (DEBUG_HITL) console.warn("⚠️ [HITL] Manual extraction also failed:", e);
}
}
}
return null;
}📝 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.
| export function parseHITLMetadata(messageText: string): HITLMetadata | null { | |
| if (!messageText) return null; | |
| // Look for HITL_METADATA in HTML comment | |
| // Try multiple regex patterns to handle different formatting | |
| const patterns = [ | |
| /<!--\s*HITL_METADATA:\s*({[\s\S]*?})\s*-->/g, // Standard format with any whitespace (non-greedy) | |
| /<!--\s*HITL_METADATA:\s*({[\s\S]*})-->/g, // Greedy match for multi-line | |
| /<!--HITL_METADATA:\s*({[\s\S]*?})-->/g, // No spaces around comment | |
| /HITL_METADATA:\s*({[\s\S]*?})(?=\s*-->)/, // More flexible - match before --> | |
| ]; | |
| for (const regex of patterns) { | |
| // Reset regex lastIndex for global patterns | |
| if (regex.global) { | |
| regex.lastIndex = 0; | |
| } | |
| const match = messageText.match(regex); | |
| if (match && match[1]) { | |
| try { | |
| // Clean up the JSON string (remove any trailing whitespace or characters) | |
| let jsonStr = match[1].trim(); | |
| // Ensure it's valid JSON by finding the matching closing brace | |
| const openBraces = (jsonStr.match(/{/g) || []).length; | |
| const closeBraces = (jsonStr.match(/}/g) || []).length; | |
| if (openBraces > closeBraces) { | |
| // Find the last closing brace | |
| const lastBraceIndex = jsonStr.lastIndexOf('}'); | |
| if (lastBraceIndex > 0) { | |
| jsonStr = jsonStr.substring(0, lastBraceIndex + 1); | |
| } | |
| } | |
| const metadata = JSON.parse(jsonStr); | |
| console.log("✅ [HITL] Parsed metadata successfully:", metadata); | |
| return metadata as HITLMetadata; | |
| } catch (error) { | |
| console.warn("⚠️ [HITL] Error parsing JSON from match:", error, "Match preview:", match[1]?.substring(0, 150)); | |
| // Continue to next pattern | |
| } | |
| } | |
| } | |
| // Debug: log if we're looking at a message that might contain HITL metadata | |
| if (messageText.includes("HITL_METADATA") || messageText.includes("hitl_request_id")) { | |
| const hitlIndex = messageText.indexOf("HITL_METADATA"); | |
| const preview = messageText.substring( | |
| Math.max(0, hitlIndex - 50), | |
| Math.min(messageText.length, hitlIndex + 600) | |
| ); | |
| console.warn("⚠️ [HITL] HITL metadata found in text but regex didn't match. Preview:", preview); | |
| // Try to manually extract JSON | |
| const jsonStart = preview.indexOf('{'); | |
| const jsonEnd = preview.lastIndexOf('}'); | |
| if (jsonStart > 0 && jsonEnd > jsonStart) { | |
| try { | |
| const manualJson = preview.substring(jsonStart, jsonEnd + 1); | |
| const metadata = JSON.parse(manualJson); | |
| console.log("✅ [HITL] Manually extracted and parsed metadata:", metadata); | |
| return metadata as HITLMetadata; | |
| } catch (e) { | |
| console.warn("⚠️ [HITL] Manual extraction also failed:", e); | |
| } | |
| } | |
| } | |
| return null; | |
| } | |
| const DEBUG_HITL = process.env.NEXT_PUBLIC_DEBUG_HITL === 'true'; | |
| export function parseHITLMetadata(messageText: string): HITLMetadata | null { | |
| if (!messageText) return null; | |
| // Look for HITL_METADATA in HTML comment | |
| // Try multiple regex patterns to handle different formatting | |
| const patterns = [ | |
| /<!--\s*HITL_METADATA:\s*({[\s\S]*?})\s*-->/g, // Standard format with any whitespace (non-greedy) | |
| /<!--\s*HITL_METADATA:\s*({[\s\S]*})-->/g, // Greedy match for multi-line | |
| /<!--HITL_METADATA:\s*({[\s\S]*?})-->/g, // No spaces around comment | |
| /HITL_METADATA:\s*({[\s\S]*?})(?=\s*-->)/, // More flexible - match before --> | |
| ]; | |
| for (const regex of patterns) { | |
| // Reset regex lastIndex for global patterns | |
| if (regex.global) { | |
| regex.lastIndex = 0; | |
| } | |
| const match = messageText.match(regex); | |
| if (match && match[1]) { | |
| try { | |
| // Clean up the JSON string (remove any trailing whitespace or characters) | |
| let jsonStr = match[1].trim(); | |
| // Ensure it's valid JSON by finding the matching closing brace | |
| const openBraces = (jsonStr.match(/{/g) || []).length; | |
| const closeBraces = (jsonStr.match(/}/g) || []).length; | |
| if (openBraces > closeBraces) { | |
| // Find the last closing brace | |
| const lastBraceIndex = jsonStr.lastIndexOf('}'); | |
| if (lastBraceIndex > 0) { | |
| jsonStr = jsonStr.substring(0, lastBraceIndex + 1); | |
| } | |
| } | |
| const metadata = JSON.parse(jsonStr); | |
| if (DEBUG_HITL) console.log("✅ [HITL] Parsed metadata successfully:", metadata); | |
| return metadata as HITLMetadata; | |
| } catch (error) { | |
| if (DEBUG_HITL) console.warn("⚠️ [HITL] Error parsing JSON from match:", error, "Match preview:", match[1]?.substring(0, 150)); | |
| // Continue to next pattern | |
| } | |
| } | |
| } | |
| // Debug: log if we're looking at a message that might contain HITL metadata | |
| if (messageText.includes("HITL_METADATA") || messageText.includes("hitl_request_id")) { | |
| const hitlIndex = messageText.indexOf("HITL_METADATA"); | |
| const preview = messageText.substring( | |
| Math.max(0, hitlIndex - 50), | |
| Math.min(messageText.length, hitlIndex + 600) | |
| ); | |
| if (DEBUG_HITL) console.warn("⚠️ [HITL] HITL metadata found in text but regex didn't match. Preview:", preview); | |
| // Try to manually extract JSON | |
| const jsonStart = preview.indexOf('{'); | |
| const jsonEnd = preview.lastIndexOf('}'); | |
| if (jsonStart > 0 && jsonEnd > jsonStart) { | |
| try { | |
| const manualJson = preview.substring(jsonStart, jsonEnd + 1); | |
| const metadata = JSON.parse(manualJson); | |
| if (DEBUG_HITL) console.log("✅ [HITL] Manually extracted and parsed metadata:", metadata); | |
| return metadata as HITLMetadata; | |
| } catch (e) { | |
| if (DEBUG_HITL) console.warn("⚠️ [HITL] Manual extraction also failed:", e); | |
| } | |
| } | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In lib/utils/hitlMetadata.ts around lines 26 to 92, remove or conditionalize the
console.log/console.warn calls (lines ~59, 62, 75, 83, 86) so they don't run in
production; replace them with either (a) calls to the project's logger if one
exists, or (b) guarded logs behind a debug flag (e.g., add an optional
parseHITLMetadata(messageText: string, opts?: { debug?: boolean }) parameter or
read a DEBUG env var) and only call console.* when debug is truthy, and ensure
types are updated and tests still pass.
- Add 'Pending Requests' link to Workflows section in sidebar - Fetch pending requests count using React Query with 30s auto-refresh - Display count badge when there are pending requests (> 0) - Invalidate query cache when requests are handled/deleted for immediate UI update - Fix merge conflict markers in pending-requests pages
- Remove merge conflict markers from pending-requests/page.tsx - Add missing Link import in [executionId]/page.tsx - Fix type error: setHitlRequests to use requests array from response object - All build errors resolved
…isplay - Add approve/reject buttons for approval nodes matching pending requests UI - Add 'Approve and Continue' / 'Request Changes' buttons for input nodes with loop_back - Hide HITL component after submission and show response in chat - Display formatted response text after submission - Support all field types (text, textarea, number, select, multi_select, boolean, file) - Update HITLMetadata interface to include loop_back_node_id and loop_back_condition - Fix 404 error handling for active-session endpoint (workflow conversations) - Improve HITL rendering in both UserMessage and AssistantMessage components
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
app/(main)/chat/[chatId]/components/Thread.tsx (2)
358-398: Console logs should be behind a debug flag.Multiple console.log statements (lines 358, 362, 381, 385, 398) will execute in production. This was already flagged in a previous review with a suggested
DEBUG_HITLenvironment variable pattern.
583-635: Console logging in useMemo will run on every render evaluation.The extensive console.log statements in the
hitlMetadatauseMemo (lines 585, 590, 596, 601, 621, 625-627, 630) run during every memo evaluation. While dependencies limit re-runs, this still generates noise. This was flagged in a previous review.app/(main)/workflows/pending-requests/[requestId]/page.tsx (1)
183-187: Merge conflict resolved.The unresolved Git merge conflict markers flagged in the previous review have been successfully removed. The code now properly invalidates the sidebar query and navigates back to the pending requests list.
app/(main)/workflows/pending-requests/page.tsx (1)
112-115: Merge conflict resolved.The unresolved Git merge conflict markers flagged in the previous review have been successfully removed. The query invalidation logic is now clean and functional.
🧹 Nitpick comments (14)
app/(main)/workflows/components/editor/components/NodeConfigPanel.tsx (1)
55-58: Consider removing debug console.log before merging to production.Debug logging statements are typically removed before production deployment to reduce noise in production logs and avoid performance overhead.
Apply this diff to remove the debug log:
const handleConfigChange = (newConfig: any) => { - console.log( - `🔧 [NodeConfigPanel] Config changed for node ${selectedNode.id}:`, - newConfig - ); setConfig(newConfig); onConfigChange(newConfig); };app/(main)/workflows/components/editor/components/NodePaletteContainer.tsx (1)
170-173: Consider removing debug console.log before merging to production.Debug logging statements are typically removed before production deployment to reduce noise in production logs and avoid performance overhead.
Apply this diff to remove the debug log:
const handleConfigChange = (config: any) => { if (selectedNode && onNodeConfigChange) { - console.log( - `🔧 [NodePaletteContainer] Calling onNodeConfigChange for node ${selectedNode.id} with config:`, - config - ); onNodeConfigChange(selectedNode.id, config); } };services/ChatService.ts (1)
56-63: Simplify error handling by removing unreachable 404 check.The
validateStatusconfiguration on lines 46-50 prevents axios from throwing on 404 responses, so the 404 check in the catch block (lines 58-60) is unreachable. Any error reaching this catch block will be a network error or a non-200/non-404 HTTP status.Apply this diff to simplify the error handling:
} catch (error: any) { - // 404 is expected for workflow conversations - return null silently - if (error.response?.status === 404) { - return null; - } - // Only log non-404 errors - console.warn("Error detecting active session (non-404):", error); + // Log unexpected errors (network errors or non-200/404 status codes) + console.warn("Error detecting active session:", error); throw error; }app/(main)/workflows/components/editor/nodes/triggers/webhook/webhook-trigger.tsx (2)
71-94: Potential stale closure with suppressed exhaustive-deps warning.
onConfigChangeis called inside the effect but excluded from the dependency array. If the parent component re-createsonConfigChangebetween renders, this could use a stale callback. Consider either:
- Memoizing
onConfigChangein the parent withuseCallback- Using a ref to track the latest callback:
+const onConfigChangeRef = useRef(onConfigChange); +useEffect(() => { + onConfigChangeRef.current = onConfigChange; +}, [onConfigChange]); + useEffect(() => { const url = config.webhook_url || (config.hash ? `${process.env.NEXT_PUBLIC_WORKFLOWS_WEBHOOK_URL || process.env.NEXT_PUBLIC_WORKFLOWS_URL}/api/v1/webhook/${config.hash}` : ""); setLocalWebhookUrl(url); if (!config.hash && config.webhook_url && !readOnly) { const match = config.webhook_url.match(/\/webhook\/([a-f0-9\-]+)/); if (match && match[1]) { const extractedHash = match[1]; - onConfigChange({ ...config, hash: extractedHash }); + onConfigChangeRef.current({ ...config, hash: extractedHash }); } } - // eslint-disable-next-line react-hooks/exhaustive-deps }, [config.hash, config.webhook_url, readOnly]);
96-112: Consider removing debug console.log statements.The
console.logat lines 99-102 and throughout the file appear to be development debugging artifacts. Consider removing these or gating them behind a debug flag to keep production console output clean.app/(main)/workflows/components/editor/utils/workflowUtils.ts (1)
154-179: Consider extracting duplicate hash extraction logic to a shared utility.The regex pattern
/\/webhook\/([a-f0-9\-]+)/and extraction logic is duplicated inwebhook-trigger.tsx(lines 82 and 160). Consider extracting to a shared utility:// In a shared utils file, e.g., lib/utils/webhookUtils.ts export const extractHashFromWebhookUrl = (webhookUrl: string): string | null => { const match = webhookUrl.match(/\/webhook\/([a-f0-9-]+)/); return match?.[1] ?? null; };Then import and use in both locations to ensure consistent behavior and reduce maintenance burden.
app/(main)/workflows/[workflowId]/executions/[executionId]/page.tsx (1)
78-87: Consider periodic refresh of HITL requests for running executions.Currently, HITL requests are fetched once on mount. For running executions, requests might be resolved by other users or the status might change. Consider adding periodic refresh similar to the execution tree pattern, especially since the PR mentions real-time updates for pending requests.
You could add HITL requests to the
refreshExecutionTreefunction or create a separate polling mechanism whenexecution.status === "running"andhitlRequests.length > 0.Also applies to: 103-133
app/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx (2)
22-22: Type theworkflowparameter.The
anytype reduces type safety and makes the component harder to maintain. Consider defining a proper interface or using the appropriate workflow type fromWorkflowService.Apply this diff to improve type safety:
+import { Workflow } from "@/services/WorkflowService"; // Assuming Workflow type exists + interface ApprovalConfigProps { config: ApprovalNodeData; onConfigChange: (config: ApprovalNodeData) => void; readOnly?: boolean; - workflow?: any; + workflow?: Workflow; }If
Workflowtype doesn't exist, define an appropriate interface for the workflow parameter.
82-84: Add programmatic bounds checking for timeout_hours.While the HTML
minandmaxattributes provide client-side validation, theparseIntlogic doesn't enforce the 1-168 hour constraint programmatically. This could allow out-of-range values if HTML validation is bypassed or doesn't trigger.Apply this diff to add defensive bounds checking:
+const clampTimeout = (value: number) => Math.min(Math.max(value, 1), 168); + onChange={(e) => - handleChange("timeout_hours", parseInt(e.target.value) || 24) + handleChange("timeout_hours", clampTimeout(parseInt(e.target.value) || 24)) }components/Layouts/Sidebar.tsx (1)
74-82: Consider error handling for the pending requests query.The query lacks error handling. If the API fails,
pendingRequestsData?.totalwill be undefined, defaulting to 0. This is acceptable, but consider addingretry: falseor error handling to prevent repeated failed requests from impacting performance.const { data: pendingRequestsData } = useQuery({ queryKey: ["pendingHITLRequests", userId], queryFn: () => WorkflowService.listHITLRequests(undefined, 1, 1), enabled: !!userId, refetchInterval: 30000, // Refetch every 30 seconds + retry: 1, // Limit retries on failure + staleTime: 10000, // Consider data fresh for 10s to reduce redundant fetches });app/(main)/chat/[chatId]/page.tsx (1)
138-142: Variable shadowing:parsingStatusshadows the state variable.Line 139 declares
const parsingStatuswhich shadows the state variableparsingStatusfrom line 42. While this works, it can cause confusion. The code correctly callssetParsingStatus(parsingStatus)on line 142, but the shadowing makes it less clear.// Only check parsing status for conversations with projects (not workflow conversations) try { - const parsingStatus = await BranchAndRepositoryService.getParsingStatus( + const status = await BranchAndRepositoryService.getParsingStatus( info.project_ids[0] ); - setParsingStatus(parsingStatus); + setParsingStatus(status); } catch (error) {app/(main)/chat/[chatId]/components/Thread.tsx (2)
754-846: Consider extracting the HITL render logic into a dedicated component.The IIFE spanning 90+ lines combines parsing logic, retry attempts, and conditional rendering. This is difficult to test and maintain. Consider extracting this into a
<HITLMessageRenderer>component that receivestext,initialText, and callbacks.// Extract to separate component const HITLMessageRenderer: FC<{ text: string; initialText: string; hitlMetadata: HITLMetadata | null; onResponseSubmitted: (data: Record<string, any>, action?: string) => void; }> = ({ text, initialText, hitlMetadata, onResponseSubmitted }) => { const hasHitlKeywords = (text || initialText).includes("HITL_METADATA") || (text || initialText).includes("hitl_request_id"); const metadataToUse = hitlMetadata || (hasHitlKeywords ? parseHITLMetadata(text || initialText) : null); if (metadataToUse) { return ( <div className="mb-4 w-full"> <HITLRequestChat metadata={metadataToUse} onResponseSubmitted={onResponseSubmitted} /> </div> ); } if (hasHitlKeywords) { // Fallback warning UI return <HITLParseFailedWarning text={text || initialText} />; } return null; };
809-811: UseuseQueryClienthook instead of accessing window global.Accessing
(window as any).queryClientis fragile and bypasses React Query's context system. Use theuseQueryClienthook for proper invalidation.+import { useQueryClient } from "@tanstack/react-query"; // In component: +const queryClient = useQueryClient(); // In callback: -if (typeof window !== 'undefined' && (window as any).queryClient) { - (window as any).queryClient.invalidateQueries({ queryKey: ['pendingHITLRequests'] }); -} +queryClient.invalidateQueries({ queryKey: ['pendingHITLRequests'] });app/(main)/workflows/pending-requests/[requestId]/page.tsx (1)
75-197: Consider extracting validation and response data assembly logic.The
handleSubmitfunction (123 lines) handles multiple node types and modes with deeply nested conditionals. While functional, extracting the validation and response assembly logic would improve readability and testability.Consider extracting helper functions such as:
function validateInputFields( fields: HITLRequest['fields'], responseData: Record<string, any> ): { valid: boolean; error?: string } { // Lines 118-124 logic } function filterResponseData( fields: HITLRequest['fields'], responseData: Record<string, any> ): Record<string, any> { // Lines 126-140 logic }This would reduce the cognitive load of the main
handleSubmitfunction and make the validation logic reusable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
app/(main)/chat/[chatId]/components/HITLRequestChat.tsx(1 hunks)app/(main)/chat/[chatId]/components/Thread.tsx(9 hunks)app/(main)/chat/[chatId]/page.tsx(2 hunks)app/(main)/chat/[chatId]/runtime.ts(3 hunks)app/(main)/workflows/[workflowId]/executions/[executionId]/page.tsx(4 hunks)app/(main)/workflows/components/editor/components/NodeConfigPanel.tsx(1 hunks)app/(main)/workflows/components/editor/components/NodePaletteContainer.tsx(1 hunks)app/(main)/workflows/components/editor/hooks/useWorkflowEditor.ts(3 hunks)app/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx(1 hunks)app/(main)/workflows/components/editor/nodes/manual-steps/input-node.tsx(1 hunks)app/(main)/workflows/components/editor/nodes/triggers/webhook/webhook-trigger.tsx(6 hunks)app/(main)/workflows/components/editor/utils/workflowUtils.ts(1 hunks)app/(main)/workflows/pending-requests/[requestId]/page.tsx(1 hunks)app/(main)/workflows/pending-requests/page.tsx(1 hunks)components/Layouts/Sidebar.tsx(4 hunks)lib/Constants.tsx(1 hunks)lib/utils/hitlMetadata.ts(1 hunks)services/ChatService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/utils/hitlMetadata.ts
- app/(main)/workflows/components/editor/nodes/manual-steps/input-node.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use PascalCase for component names and camelCase for utility names
Order imports: React/Next imports first, then components, then utilities
Use Next.js error boundaries, optional chaining, and nullish coalescing for error handling
Files:
app/(main)/workflows/components/editor/utils/workflowUtils.tsapp/(main)/workflows/components/editor/hooks/useWorkflowEditor.tsapp/(main)/chat/[chatId]/page.tsxapp/(main)/workflows/components/editor/components/NodePaletteContainer.tsxapp/(main)/chat/[chatId]/runtime.tscomponents/Layouts/Sidebar.tsxservices/ChatService.tsapp/(main)/chat/[chatId]/components/Thread.tsxapp/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsxapp/(main)/workflows/[workflowId]/executions/[executionId]/page.tsxlib/Constants.tsxapp/(main)/workflows/pending-requests/[requestId]/page.tsxapp/(main)/chat/[chatId]/components/HITLRequestChat.tsxapp/(main)/workflows/components/editor/components/NodeConfigPanel.tsxapp/(main)/workflows/pending-requests/page.tsxapp/(main)/workflows/components/editor/nodes/triggers/webhook/webhook-trigger.tsx
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.tsx: Use Tailwind CSS for styling and thecnutility for conditional classes
Use functional components with TypeScript FC types
Prefer destructuring props at the component level
Use the "use client" directive for client components
Files:
app/(main)/chat/[chatId]/page.tsxapp/(main)/workflows/components/editor/components/NodePaletteContainer.tsxcomponents/Layouts/Sidebar.tsxapp/(main)/chat/[chatId]/components/Thread.tsxapp/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsxapp/(main)/workflows/[workflowId]/executions/[executionId]/page.tsxlib/Constants.tsxapp/(main)/workflows/pending-requests/[requestId]/page.tsxapp/(main)/chat/[chatId]/components/HITLRequestChat.tsxapp/(main)/workflows/components/editor/components/NodeConfigPanel.tsxapp/(main)/workflows/pending-requests/page.tsxapp/(main)/workflows/components/editor/nodes/triggers/webhook/webhook-trigger.tsx
🧠 Learnings (1)
📚 Learning: 2025-07-22T09:20:21.427Z
Learnt from: CR
Repo: potpie-ai/potpie-ui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T09:20:21.427Z
Learning: Applies to **/*.{ts,tsx} : Use Next.js error boundaries, optional chaining, and nullish coalescing for error handling
Applied to files:
app/(main)/workflows/[workflowId]/executions/[executionId]/page.tsx
🧬 Code graph analysis (8)
app/(main)/workflows/components/editor/utils/workflowUtils.ts (3)
app/(main)/workflows/components/editor/nodes/node-registry.ts (2)
NodeType(2-17)NodeGroup(19-24)services/WorkflowService.ts (2)
NodeType(113-128)NodeGroup(130-130)app/(main)/workflows/components/editor/nodes/color_utils.ts (1)
NodeGroup(1-6)
app/(main)/chat/[chatId]/page.tsx (1)
services/BranchAndRepositoryService.ts (1)
BranchAndRepositoryService(11-199)
app/(main)/chat/[chatId]/runtime.ts (3)
services/ChatService.ts (2)
ChatService(7-699)resumeActiveSession(86-171)lib/utils/hitlMetadata.ts (1)
parseHITLMetadata(29-121)services/WorkflowService.ts (1)
WorkflowService(249-940)
app/(main)/chat/[chatId]/components/Thread.tsx (4)
lib/utils/hitlMetadata.ts (3)
parseHITLMetadata(29-121)HITLMetadata(5-23)removeHITLMetadata(126-132)app/(main)/chat/[chatId]/components/HITLRequestChat.tsx (1)
HITLRequestChat(27-405)lib/utils.ts (1)
isMultimodalEnabled(74-77)app/layout.tsx (1)
metadata(9-12)
app/(main)/workflows/[workflowId]/executions/[executionId]/page.tsx (1)
services/WorkflowService.ts (2)
HITLRequest(69-95)WorkflowService(249-940)
app/(main)/workflows/pending-requests/[requestId]/page.tsx (4)
app/utils/queryClient.ts (1)
queryClient(3-10)contexts/AuthContext.tsx (1)
useAuthContext(14-14)services/WorkflowService.ts (3)
HITLRequest(69-95)WorkflowService(249-940)HITLResponseRequest(97-100)lib/utils.ts (1)
formatRelativeTime(166-170)
app/(main)/workflows/pending-requests/page.tsx (1)
services/WorkflowService.ts (3)
HITLRequest(69-95)WorkflowService(249-940)HITLResponseRequest(97-100)
app/(main)/workflows/components/editor/nodes/triggers/webhook/webhook-trigger.tsx (1)
services/WorkflowService.ts (2)
WorkflowService(249-940)WorkflowNode(133-140)
🪛 Biome (2.1.2)
app/(main)/chat/[chatId]/components/Thread.tsx
[error] 356-356: This hook is being called from a nested function, but all hooks must be called unconditionally from the top-level component.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 394-394: This hook is being called from a nested function, but all hooks must be called unconditionally from the top-level component.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (29)
services/ChatService.ts (1)
41-55: LGTM: Clean handling of 404 as a valid state.The use of
validateStatusto treat 404 as a non-error response is the correct approach for this HITL workflow scenario. The explicit null return for missing sessions aligns well with the PR's goal to reduce noise for workflow conversations without active sessions.app/(main)/workflows/components/editor/nodes/triggers/webhook/webhook-trigger.tsx (5)
37-37: LGTM!Import updated correctly to include
useEffectfor the new synchronization logic.
50-56: LGTM!Interface extension is backward compatible and aligns with the
WorkflowService.refreshTriggerHashreturn type.
114-145: LGTM!The webhook generation flow correctly stores both
trigger_hashandwebhook_urlfrom the API response, ensuring the backend's canonical URL is used.
164-167: LGTM!The priority chain ensures immediate UI feedback via
localWebhookUrlwhile falling back to config values.
324-332: LGTM!The
isWebhookGeneratedflag correctly handles bothhashandwebhook_urlfor backward compatibility, and the destructuring pattern is clean.app/(main)/workflows/components/editor/utils/workflowUtils.ts (1)
170-188: LGTM!The augmented
nodeDatais correctly assigned, ensuring the extracted hash is persisted when saving the workflow.app/(main)/workflows/components/editor/hooks/useWorkflowEditor.ts (2)
655-676: LGTM! Reasonable workaround for React Flow's change detection.The
_lastUpdatedtimestamp forces React Flow to detect configuration changes by updating the data reference. This is a well-known pattern for React Flow, and the implementation is correct:
- Applied consistently to both
editingNodesandselectedNode- Does not pollute persisted workflow data (the actual
configis saved separately at line 688)- Well-commented explaining the intent
This ensures that HITL node configuration updates are immediately visible in the workflow editor.
980-991: LGTM! Helpful debug logging for webhook trigger nodes.The debug logging provides visibility into webhook trigger node metadata during save operations. The implementation is appropriate:
- Properly gated behind the
debugModeflag- Clear logging format with emoji prefix for easy identification
- Useful for debugging webhook-related issues during HITL workflows
app/(main)/workflows/[workflowId]/executions/[executionId]/page.tsx (4)
7-7: Previous issues resolved: imports are now complete.The
HITLRequesttype andLinkcomponent imports have been added, fixing the previous critical runtime error.Also applies to: 10-10
42-42: LGTM: State declaration is properly typed.
78-87: Previous type mismatch resolved correctly.The code now properly extracts the
requestsarray from the paginated response with appropriate fallback handling.
259-298: HITL requests UI implemented correctly.The pending requests section displays clearly with appropriate styling and user actions. The Link component is now properly imported, resolving the previous runtime error.
app/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx (3)
36-46: LGTM!The approvers parsing logic correctly handles comma-separated input with trimming and filtering. The bidirectional conversion between string and array is well implemented.
146-187: LGTM!The
ApprovalNodecomponent is well-structured with proper conditional rendering, dynamic styling, and correct handle placement for workflow diagram connections. The metadata export follows the expected pattern for node registration.
169-171: No action required. The concern about color format is unfounded.getNodeColorsconsistently returns 6-digit hex strings (e.g.,"#4b5563","#e5e7eb"). Concatenating"20"or"40"produces valid 8-digit hex colors with alpha transparency (e.g.,"#e5e7eb20"for ~12.5% opacity), which is the intended behavior.app/(main)/chat/[chatId]/runtime.ts (2)
367-374: LGTM - Good fallback handling on HITL submission failure.The success case adds a confirmation message and exits early, while failures gracefully fall through to normal chat flow. This ensures the user isn't blocked if HITL submission encounters issues.
74-94: LGTM - Graceful handling of 404 for workflow conversations.The try/catch properly distinguishes between expected 404s (workflow conversations without active sessions) and unexpected errors, allowing normal message loading to continue in both cases.
components/Layouts/Sidebar.tsx (1)
184-193: LGTM - Badge rendering logic is clean.The conditional rendering correctly prioritizes the pending count badge over the description badge, and the styling is consistent with existing UI patterns.
lib/Constants.tsx (1)
131-136: LGTM - New navigation item follows existing patterns.The Pending Requests link is properly structured with icon, title, href, and disabled flag consistent with other sidebar items.
app/(main)/chat/[chatId]/page.tsx (1)
170-176: LGTM - Guard prevents dialog for workflow conversations.The early return when
projectIdis empty ensures workflow conversations don't trigger parsing dialogs, which aligns with the expected behavior described in the AI summary.app/(main)/chat/[chatId]/components/Thread.tsx (2)
356-391: LGTM - useMemo is correctly at component top level.The static analysis hint about hooks in nested functions is a false positive. The
useMemocalls are at the top level of theUserMessagefunctional component, not inside a nested function. The component is defined via a factory pattern (UserMessageWithURL), butUserMessageitself is a proper FC.
479-508: LGTM - Helper function for formatting HITL responses.The
formatResponseTexthelper provides clean formatting for different HITL response types (approval, requested_changes, input fields) and is properly typed.app/(main)/workflows/pending-requests/[requestId]/page.tsx (2)
414-422: Verify file handling implementation with backend requirements.The file input handling currently only captures and stores the filename (
file.name) inresponseData, not the actual file content or a file reference. Ensure this aligns with the backend's expectations for file fields in HITL responses.If the backend requires actual file uploads, you'll need to implement file upload handling (e.g., FormData, multipart/form-data) and update the
HITLResponseRequesttype and submission logic accordingly.Also applies to: 538-546
1-44: Well-structured component with proper Next.js patterns.The component demonstrates good practices:
- Proper "use client" directive for client components
- Correct use of Next.js hooks (useParams, useSearchParams, useRouter)
- React Query integration for cache invalidation
- Loading skeletons and error states for better UX
- Toast notifications for user feedback
Also applies to: 206-241
app/(main)/workflows/pending-requests/page.tsx (2)
513-595: Well-implemented pagination with edge case handling.The pagination implementation includes:
- Dynamic page size selection (10, 20, 50, 100)
- Sliding window for page numbers (shows 5 pages max)
- Proper edge case handling for start, middle, and end positions
- Clear page info display ("Showing X to Y of Z requests")
- Disabled state management for Previous/Next buttons
72-77: Auto-refresh with proper cleanup.The 30-second auto-refresh interval for pending requests is well-implemented with proper cleanup in the useEffect return function. The dependency array correctly includes
pageandpageSizeto reload when pagination changes.app/(main)/chat/[chatId]/components/HITLRequestChat.tsx (2)
408-537: Field rendering properly refactored into helper function.The previous review's concern about duplicate field type conditionals has been addressed. The code now uses a clean
renderFieldInputhelper function with a switch statement that properly handles each field type without duplication.
22-30: Well-designed component with clear state management.The component demonstrates good React patterns:
- Proper TypeScript interface definition with optional callback prop
- Post-submission state management to show feedback without re-rendering the form
- Clear separation between expired, submitted, and active states
- Helper functions for formatting and rendering
Also applies to: 190-233
| // Check if this is a review mode (has loop_back_node_id) | ||
| const isReviewMode = !!(metadata as any).hitl_loop_back_node_id; | ||
| const loopBackCondition = (metadata as any).hitl_loop_back_condition || "needs_changes"; |
There was a problem hiding this comment.
Avoid unsafe type casting for loop-back properties.
The type assertions (metadata as any) bypass TypeScript's type safety when accessing hitl_loop_back_node_id and hitl_loop_back_condition. This could mask errors if property names change or if the properties are missing.
Update the HITLMetadata interface in lib/utils/hitlMetadata.ts to include these optional properties:
export interface HITLMetadata {
hitl_request_id: string;
hitl_execution_id: string;
hitl_node_id: string;
hitl_iteration: number;
hitl_node_type: "approval" | "input";
hitl_timeout_at: string;
hitl_fields?: Array<{
name: string;
type: string;
required?: boolean;
options?: string[];
}>;
+ hitl_loop_back_node_id?: string;
+ hitl_loop_back_condition?: string;
}Then remove the type assertions:
- const isReviewMode = !!(metadata as any).hitl_loop_back_node_id;
- const loopBackCondition = (metadata as any).hitl_loop_back_condition || "needs_changes";
+ const isReviewMode = !!metadata.hitl_loop_back_node_id;
+ const loopBackCondition = metadata.hitl_loop_back_condition || "needs_changes";Committable suggestion skipped: line range outside the PR's diff.
| if (textToCheck.includes("HITL_METADATA") || textToCheck.includes("hitl_request_id")) { | ||
| console.warn("⚠️ [HITL] Regex parsing failed, trying manual extraction. Text length:", textToCheck.length); | ||
| const hitlIndex = textToCheck.indexOf("HITL_METADATA"); | ||
| if (hitlIndex > -1) { | ||
| // Extract a larger snippet to ensure we get the full JSON | ||
| const snippet = textToCheck.substring( | ||
| Math.max(0, hitlIndex - 20), | ||
| Math.min(textToCheck.length, hitlIndex + 3000) | ||
| ); | ||
|
|
||
| // Find the JSON object boundaries | ||
| const jsonStart = snippet.indexOf('{'); | ||
| const jsonEnd = snippet.lastIndexOf('}'); | ||
| if (jsonStart > 0 && jsonEnd > jsonStart) { | ||
| try { | ||
| // Extract and parse JSON | ||
| let jsonStr = snippet.substring(jsonStart, jsonEnd + 1); | ||
| // Clean up any trailing characters | ||
| jsonStr = jsonStr.trim(); | ||
| const extracted = JSON.parse(jsonStr); | ||
| if (extracted.hitl_request_id && extracted.hitl_execution_id) { | ||
| console.log("✅ [HITL] Manually extracted metadata in useMemo:", extracted); | ||
| return extracted as HITLMetadata; | ||
| } | ||
| } catch (e) { | ||
| console.error("❌ [HITL] Manual extraction in useMemo failed:", e); | ||
| const jsonPreview = snippet.substring(jsonStart, Math.min(jsonStart + 500, jsonEnd + 1)); | ||
| console.error("❌ [HITL] JSON string preview:", jsonPreview); | ||
| } | ||
| } else { | ||
| console.warn("⚠️ [HITL] Could not find JSON boundaries. HitlIndex:", hitlIndex, "Snippet length:", snippet.length); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor: Extract duplicate manual HITL parsing logic to utility function.
The manual JSON extraction logic is duplicated in:
UserMessageuseMemo (lines 366-390)AssistantMessageuseMemo (lines 600-632)- Render IIFE (lines 770-792)
This should be consolidated into parseHITLMetadata in lib/utils/hitlMetadata.ts to ensure consistent behavior and reduce maintenance burden.
// In lib/utils/hitlMetadata.ts, enhance parseHITLMetadata to include manual extraction:
export function parseHITLMetadata(messageText: string): HITLMetadata | null {
// ... existing regex patterns ...
// If regex failed, try manual extraction as fallback
if (messageText.includes("HITL_METADATA") || messageText.includes("hitl_request_id")) {
const hitlIndex = messageText.indexOf("HITL_METADATA");
if (hitlIndex > -1) {
const snippet = messageText.substring(
Math.max(0, hitlIndex - 20),
Math.min(messageText.length, hitlIndex + 3000)
);
const jsonStart = snippet.indexOf('{');
const jsonEnd = snippet.lastIndexOf('}');
if (jsonStart > 0 && jsonEnd > jsonStart) {
try {
const jsonStr = snippet.substring(jsonStart, jsonEnd + 1).trim();
const extracted = JSON.parse(jsonStr);
if (extracted.hitl_request_id && extracted.hitl_execution_id) {
return extracted as HITLMetadata;
}
} catch { /* ignore */ }
}
}
}
return null;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/(main)/chat/[chatId]/components/Thread.tsx around lines 600-632, the
manual HITL JSON extraction is duplicated; extract this logic into
lib/utils/hitlMetadata.ts as a single exported function parseHITLMetadata(text:
string): HITLMetadata | null. Implement parsing to: check for "HITL_METADATA" or
"hitl_request_id", locate the hitl marker index, extract a safe snippet (e.g.,
±3000 chars but clamped), find the first '{' and last '}', trim and JSON.parse
the substring, validate it contains hitl_request_id and hitl_execution_id, and
return the parsed object or null on failure (log errors inside the util if
desired). Then replace the duplicated blocks in UserMessage useMemo,
AssistantMessage useMemo, and the Render IIFE with imports and calls to
parseHITLMetadata(textToCheck) and handle its null/returned value accordingly.
| if (lowerText.includes("approve") || lowerText === "yes" || lowerText === "y") { | ||
| responseData = { approved: true }; | ||
| } else if (lowerText.includes("reject") || lowerText === "no" || lowerText === "n") { | ||
| responseData = { approved: false }; | ||
| } else { | ||
| // Default to approve if unclear | ||
| responseData = { approved: true }; | ||
| } |
There was a problem hiding this comment.
Defaulting to approval on ambiguous input may cause unintended workflow progression.
When user input doesn't clearly indicate "approve" or "reject," the code defaults to approved: true (line 313). This could lead to accidental approvals if the user's response is misinterpreted.
Consider requiring explicit approval language or prompting the user for clarification instead of silently approving.
- } else {
- // Default to approve if unclear
- responseData = { approved: true };
- }
+ } else {
+ // Don't default - require explicit approval
+ console.warn("⚠️ [HITL] Ambiguous approval response, asking user to clarify");
+ // Continue to normal chat flow to let the user know their response wasn't clear
+ const clarificationMessage: ThreadMessageLike = {
+ role: "assistant",
+ content: [{ type: "text", text: "I couldn't determine if you want to approve or reject. Please respond with 'approve' or 'reject'." }],
+ };
+ setMessages((currentMessages) => [...currentMessages, clarificationMessage]);
+ return;
+ }Committable suggestion skipped: line range outside the PR's diff.
| {request.fields && request.fields.length > 0 && ( | ||
| <div className="space-y-4"> | ||
| <Label className="text-sm font-semibold">Provide Input</Label> | ||
| {request.fields.map((field) => ( | ||
| <div key={field.name}> | ||
| <Label htmlFor={field.name} className="mb-2 block"> | ||
| {field.name} | ||
| {field.required && <span className="text-red-500 ml-1">*</span>} | ||
| </Label> | ||
| {field.type === "text" && ( | ||
| <Textarea | ||
| id={field.name} | ||
| value={responseData[field.name] || ""} | ||
| onChange={(e) => handleFieldChange(field.name, e.target.value)} | ||
| disabled={submitting} | ||
| required={field.required} | ||
| /> | ||
| )} | ||
| {field.type === "number" && ( | ||
| <Input | ||
| id={field.name} | ||
| type="number" | ||
| value={responseData[field.name] || ""} | ||
| onChange={(e) => handleFieldChange(field.name, parseFloat(e.target.value) || 0)} | ||
| disabled={submitting} | ||
| required={field.required} | ||
| /> | ||
| )} | ||
| {field.type === "select" && ( | ||
| <select | ||
| id={field.name} | ||
| value={responseData[field.name] || ""} | ||
| onChange={(e) => handleFieldChange(field.name, e.target.value)} | ||
| disabled={submitting} | ||
| required={field.required} | ||
| className="w-full px-3 py-2 border border-gray-300 rounded-md" | ||
| > | ||
| <option value="">Select an option</option> | ||
| {field.options?.map((option) => ( | ||
| <option key={option} value={option}> | ||
| {option} | ||
| </option> | ||
| ))} | ||
| </select> | ||
| )} | ||
| {field.type === "multi_select" && ( | ||
| <div className="space-y-2"> | ||
| {field.options?.map((option) => ( | ||
| <label key={option} className="flex items-center"> | ||
| <input | ||
| type="checkbox" | ||
| checked={(responseData[field.name] || []).includes(option)} | ||
| onChange={(e) => { | ||
| const current = responseData[field.name] || []; | ||
| const updated = e.target.checked | ||
| ? [...current, option] | ||
| : current.filter((v: string) => v !== option); | ||
| handleFieldChange(field.name, updated); | ||
| }} | ||
| disabled={submitting} | ||
| className="mr-2" | ||
| /> | ||
| {option} | ||
| </label> | ||
| ))} | ||
| </div> | ||
| )} | ||
| {field.type === "file" && ( | ||
| <Input | ||
| id={field.name} | ||
| type="file" | ||
| onChange={(e) => { | ||
| const file = e.target.files?.[0]; | ||
| if (file) { | ||
| handleFieldChange(field.name, file.name); | ||
| } | ||
| }} | ||
| disabled={submitting} | ||
| required={field.required} | ||
| /> | ||
| )} | ||
| </div> | ||
| ))} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicate field rendering logic into a reusable helper function.
The field rendering logic for input nodes is duplicated between review mode (lines 343-426) and standard mode (lines 469-548). Both blocks contain identical conditionals for handling text, number, select, multi_select, and file field types.
Extract the field rendering logic into a helper function similar to the approach in HITLRequestChat.tsx (lines 408-537):
+function renderFieldInput(
+ field: { name: string; type: string; options?: string[]; required?: boolean },
+ value: any,
+ onChange: (fieldName: string, value: any) => void,
+ disabled: boolean
+) {
+ switch (field.type) {
+ case "text":
+ return (
+ <Textarea
+ id={field.name}
+ value={value || ""}
+ onChange={(e) => onChange(field.name, e.target.value)}
+ disabled={disabled}
+ required={field.required}
+ />
+ );
+ case "number":
+ return (
+ <Input
+ id={field.name}
+ type="number"
+ value={value || ""}
+ onChange={(e) => onChange(field.name, parseFloat(e.target.value) || 0)}
+ disabled={disabled}
+ required={field.required}
+ />
+ );
+ // ... other field types
+ }
+}Then replace both occurrences with:
{request.fields.map((field) => (
<div key={field.name}>
<Label htmlFor={field.name} className="mb-2 block">
{field.name}
{field.required && <span className="text-red-500 ml-1">*</span>}
</Label>
- {field.type === "text" && (
- <Textarea ... />
- )}
- {field.type === "number" && (
- <Input type="number" ... />
- )}
- // ... more conditionals
+ {renderFieldInput(field, responseData[field.name] || "", handleFieldChange, submitting)}
</div>
))}Also applies to: 469-548
| {selectedRequest.fields.map((field) => ( | ||
| <div key={field.name}> | ||
| <Label htmlFor={field.name} className="mb-2 block"> | ||
| {field.name} | ||
| {field.required && <span className="text-red-500 ml-1">*</span>} | ||
| </Label> | ||
| {field.type === "text" && ( | ||
| <Textarea | ||
| id={field.name} | ||
| value={responseData[field.name] || ""} | ||
| onChange={(e) => handleFieldChange(field.name, e.target.value)} | ||
| disabled={submitting} | ||
| required={field.required} | ||
| /> | ||
| )} | ||
| {field.type === "number" && ( | ||
| <Input | ||
| id={field.name} | ||
| type="number" | ||
| value={responseData[field.name] || ""} | ||
| onChange={(e) => handleFieldChange(field.name, parseFloat(e.target.value) || 0)} | ||
| disabled={submitting} | ||
| required={field.required} | ||
| /> | ||
| )} | ||
| {field.type === "select" && ( | ||
| <select | ||
| id={field.name} | ||
| value={responseData[field.name] || ""} | ||
| onChange={(e) => handleFieldChange(field.name, e.target.value)} | ||
| disabled={submitting} | ||
| required={field.required} | ||
| className="w-full px-3 py-2 border border-input rounded-md bg-background" | ||
| > | ||
| <option value="">Select an option</option> | ||
| {field.options?.map((option) => ( | ||
| <option key={option} value={option}> | ||
| {option} | ||
| </option> | ||
| ))} | ||
| </select> | ||
| )} | ||
| {field.type === "multi_select" && ( | ||
| <div className="space-y-2"> | ||
| {field.options?.map((option) => ( | ||
| <label key={option} className="flex items-center"> | ||
| <input | ||
| type="checkbox" | ||
| checked={(responseData[field.name] || []).includes(option)} | ||
| onChange={(e) => { | ||
| const current = responseData[field.name] || []; | ||
| const updated = e.target.checked | ||
| ? [...current, option] | ||
| : current.filter((v: string) => v !== option); | ||
| handleFieldChange(field.name, updated); | ||
| }} | ||
| disabled={submitting} | ||
| className="mr-2" | ||
| /> | ||
| {option} | ||
| </label> | ||
| ))} | ||
| </div> | ||
| )} | ||
| {field.type === "file" && ( | ||
| <Input | ||
| id={field.name} | ||
| type="file" | ||
| onChange={(e) => { | ||
| const file = e.target.files?.[0]; | ||
| if (file) { | ||
| handleFieldChange(field.name, file.name); | ||
| } | ||
| }} | ||
| disabled={submitting} | ||
| required={field.required} | ||
| /> | ||
| )} | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicate field rendering logic into a shared component.
The drawer contains the same field rendering duplication issue as the detail page: identical logic appears for review mode (lines 736-815) and standard mode (lines 860-937). Additionally, this logic is duplicated from the detail page ([requestId]/page.tsx).
Consider creating a shared HITLFieldInput component in a separate file:
// components/workflows/HITLFieldInput.tsx
interface HITLFieldInputProps {
field: {
name: string;
type: string;
options?: string[];
required?: boolean;
};
value: any;
onChange: (fieldName: string, value: any) => void;
disabled?: boolean;
}
export function HITLFieldInput({ field, value, onChange, disabled }: HITLFieldInputProps) {
// Consolidate field rendering logic here
}Then import and use it in both files:
{request.fields.map((field) => (
<div key={field.name}>
<Label htmlFor={field.name} className="mb-2 block">
{field.name}
{field.required && <span className="text-red-500 ml-1">*</span>}
</Label>
- {field.type === "text" && (
- <Textarea ... />
- )}
- // ... more conditionals
+ <HITLFieldInput
+ field={field}
+ value={responseData[field.name] || ""}
+ onChange={handleFieldChange}
+ disabled={submitting}
+ />
</div>
))}Also applies to: 860-937
🤖 Prompt for AI Agents
In app/(main)/workflows/pending-requests/page.tsx around lines 736-815 (and also
the duplicated block at 860-937 and the duplicate in [requestId]/page.tsx), the
per-field rendering logic is duplicated across review and standard modes and
across pages; extract this into a shared component. Create
components/workflows/HITLFieldInput.tsx that accepts props { field, value,
onChange, disabled, required } (with field having name, type, options[]),
implement the switch/conditional rendering for types text, number, select,
multi_select, file, include the Label and required marker, ensure number
parsing, multi-select array handling, file -> send file.name, and preserve
classNames and disabled/required behavior; export the component and replace each
duplicated block in both files with a single HITLFieldInput usage passing
responseData[field.name], handleFieldChange, submitting, and field.required.
Ensure proper imports and TypeScript typings for the component props and usage.
- Remove HITL code from execution page (not part of SSO PR) - Fix missing Link import in execution page - Fix unescaped quotes in input-node.tsx - Add HITL stub types/methods to fix pre-existing build errors
- Add HITLRequestChat component for chat integration - Update Thread component for HITL metadata parsing - Add HITL metadata utilities - Update approval and input node components - Add pending requests pages for HITL management
- Resolved conflicts by keeping local HITL UI components - Maintains HITLRequestChat, Thread updates, and HITL metadata utilities - Keeps approval and input node components with HITL integration - Resolved conflicts in execution page and WorkflowService to use full HITL implementation
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
lib/utils/hitlMetadata.ts (2)
26-118: Conditionalize or remove console logs in production code.The
parseHITLMetadatafunction contains multipleconsole.logandconsole.warnstatements (lines 65, 68, 82, 106, 110-111) that will execute in production, potentially exposing implementation details and cluttering the console.As noted in previous reviews, consider:
- Adding a debug flag:
const DEBUG_HITL = process.env.NEXT_PUBLIC_DEBUG_HITL === 'true'- Wrapping all console statements:
if (DEBUG_HITL) console.log(...)- Or removing the logs entirely if not needed for production diagnostics
123-172: Conditionalize or remove console logs in production code.The
removeHITLMetadatafunction contains console.log statements (lines 150, 157, 163) that will execute in production.Apply the same debug flag approach as recommended for
parseHITLMetadatato prevent production console pollution.app/(main)/chat/[chatId]/components/Thread.tsx (3)
463-491: Conditionalize console logs for production.Multiple console.log and console.warn statements (lines 467, 473, 478, 487) will execute in production, potentially cluttering the console and exposing implementation details.
As noted in previous reviews, wrap these logs behind a debug flag:
const DEBUG_HITL = process.env.NEXT_PUBLIC_DEBUG_HITL === 'true'; // Then: if (DEBUG_HITL) console.log(...)
510-522: Conditionalize initialization logs.Console logs at lines 517 and 519 will run for every message initialization in production.
Apply the same debug flag approach to prevent production console noise.
591-636: Remove duplicate HITL error handling blocks.Two nearly identical conditional blocks (lines 602-621 and 623-636) both check for HITL keywords when metadata parsing fails, resulting in duplicate error messages.
Remove the second block (lines 623-636) as noted in previous reviews to avoid showing duplicate warnings to users.
lib/sso/unified-auth.ts (1)
16-110: Add error handling and timeout configuration to all HTTP methods.All methods in
UnifiedAuthClientlack error handling and timeout configuration. Network requests can fail or hang indefinitely, and these methods will throw raw axios errors without user-friendly messages.As noted in previous reviews, consider:
- Wrapping axios calls in try-catch blocks
- Adding timeout configuration (e.g., 10 seconds)
- Using the
parseApiErrorutility from@/lib/utilsfor consistent error messages- Throwing normalized errors with friendly messages
Apply similar error handling patterns to all methods:
ssoLogin,confirmLinking,cancelLinking,getMyProviders,setPrimaryProvider,unlinkProvider, andgetAccount.app/(main)/chat/[chatId]/components/HITLRequestChat.tsx (1)
201-218: Fix duplicate field type conditional preventing textarea rendering.Line 210 checks
field.type === "textarea" || field.type === "text", but"text"was already handled at line 201. This means textarea fields will never render asTextareacomponents because the text condition matches first.🔎 Apply this fix:
) : field.type === "text" || field.type === "string" ? ( <Input id={field.name} type="text" value={responseData[field.name] || ""} onChange={(e) => handleFieldChange(field.name, e.target.value)} required={field.required} className="mt-1" /> -) : field.type === "textarea" || field.type === "text" ? ( +) : field.type === "textarea" ? ( <Textarea id={field.name} value={responseData[field.name] || ""} onChange={(e) => handleFieldChange(field.name, e.target.value)} required={field.required} className="mt-1" rows={4} />
🧹 Nitpick comments (2)
app/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx (2)
18-23: Type theworkflowprop properly.The
workflowprop is typed asany, which defeats TypeScript's type safety. If this prop is not currently used, consider removing it. Otherwise, import and use the properWorkflowtype from your service layer.🔎 Suggested fix:
If the prop is needed:
+import { WorkflowNode, Workflow } from "@/services/WorkflowService"; + interface ApprovalConfigProps { config: ApprovalNodeData; onConfigChange: (config: ApprovalNodeData) => void; readOnly?: boolean; - workflow?: any; + workflow?: Workflow; }If the prop is unused, remove it:
interface ApprovalConfigProps { config: ApprovalNodeData; onConfigChange: (config: ApprovalNodeData) => void; readOnly?: boolean; - workflow?: any; }
74-90: Consider validation feedback for timeout input.The
parseInt(e.target.value) || 24fallback (line 83) silently resets invalid input to the default without user feedback. While the HTML5 number input provides basic browser validation, explicitly handling and displaying validation errors would improve UX.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/(main)/chat/[chatId]/components/HITLRequestChat.tsx(1 hunks)app/(main)/chat/[chatId]/components/Thread.tsx(6 hunks)app/(main)/workflows/[workflowId]/executions/[executionId]/page.tsx(2 hunks)app/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx(1 hunks)app/(main)/workflows/components/editor/nodes/manual-steps/input-node.tsx(1 hunks)app/(main)/workflows/pending-requests/[requestId]/page.tsx(1 hunks)lib/sso/unified-auth.ts(1 hunks)lib/utils/hitlMetadata.ts(1 hunks)services/WorkflowService.ts(3 hunks)types/auth.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/(main)/workflows/pending-requests/[requestId]/page.tsx
- app/(main)/workflows/components/editor/nodes/manual-steps/input-node.tsx
- types/auth.ts
- app/(main)/workflows/[workflowId]/executions/[executionId]/page.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use PascalCase for component names and camelCase for utility names
Order imports: React/Next imports first, then components, then utilities
Use Next.js error boundaries, optional chaining, and nullish coalescing for error handling
Files:
app/(main)/chat/[chatId]/components/Thread.tsxlib/utils/hitlMetadata.tslib/sso/unified-auth.tsapp/(main)/chat/[chatId]/components/HITLRequestChat.tsxapp/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsxservices/WorkflowService.ts
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.tsx: Use Tailwind CSS for styling and thecnutility for conditional classes
Use functional components with TypeScript FC types
Prefer destructuring props at the component level
Use the "use client" directive for client components
Files:
app/(main)/chat/[chatId]/components/Thread.tsxapp/(main)/chat/[chatId]/components/HITLRequestChat.tsxapp/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx
🧬 Code graph analysis (4)
app/(main)/chat/[chatId]/components/Thread.tsx (2)
lib/utils/hitlMetadata.ts (2)
parseHITLMetadata(26-118)removeHITLMetadata(123-172)app/(main)/chat/[chatId]/components/HITLRequestChat.tsx (1)
HITLRequestChat(24-265)
lib/sso/unified-auth.ts (1)
types/auth.ts (3)
SSOLoginResponse(21-30)AuthProvider(10-19)UserAccount(32-42)
app/(main)/chat/[chatId]/components/HITLRequestChat.tsx (2)
lib/utils/hitlMetadata.ts (1)
HITLMetadata(5-20)services/WorkflowService.ts (1)
WorkflowService(279-970)
services/WorkflowService.ts (1)
lib/utils.ts (1)
parseApiError(195-302)
🔇 Additional comments (2)
services/WorkflowService.ts (1)
865-969: HITL methods properly use error handling.All four HITL methods (
listHITLRequests,getHITLRequest,submitHITLResponse,deleteHITLRequest) appropriately useparseApiErrorfor error handling and follow the existing service pattern.app/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx (1)
168-172: No issues found. The color concatenation in lines 170-171 correctly assumes hex format, which is always returned bygetNodeColors. The 8-digit hex notation (#RRGGBBAA) produces valid CSS with alpha transparency and is not fragile.
| import { FC } from "react"; | ||
| import { Label } from "@/components/ui/label"; | ||
| import { Textarea } from "@/components/ui/textarea"; | ||
| import { Input } from "@/components/ui/input"; | ||
| import { WorkflowNode } from "@/services/WorkflowService"; | ||
| import { getNodeColors } from "../color_utils"; | ||
| import { SourceHandle, TargetHandle } from "../../handles"; | ||
| import { CheckCircle } from "lucide-react"; |
There was a problem hiding this comment.
Add "use client" directive.
This component contains interactive elements (Input, Textarea with onChange handlers), which require client-side execution in Next.js. Add the "use client" directive at the top of the file.
As per coding guidelines, use the "use client" directive for client components.
🔎 Apply this diff to add the directive:
+"use client";
+
import { FC } from "react";
import { Label } from "@/components/ui/label";
import { Textarea } from "@/components/ui/textarea";📝 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.
| import { FC } from "react"; | |
| import { Label } from "@/components/ui/label"; | |
| import { Textarea } from "@/components/ui/textarea"; | |
| import { Input } from "@/components/ui/input"; | |
| import { WorkflowNode } from "@/services/WorkflowService"; | |
| import { getNodeColors } from "../color_utils"; | |
| import { SourceHandle, TargetHandle } from "../../handles"; | |
| import { CheckCircle } from "lucide-react"; | |
| "use client"; | |
| import { FC } from "react"; | |
| import { Label } from "@/components/ui/label"; | |
| import { Textarea } from "@/components/ui/textarea"; | |
| import { Input } from "@/components/ui/input"; | |
| import { WorkflowNode } from "@/services/WorkflowService"; | |
| import { getNodeColors } from "../color_utils"; | |
| import { SourceHandle, TargetHandle } from "../../handles"; | |
| import { CheckCircle } from "lucide-react"; |
🤖 Prompt for AI Agents
In app/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx
around lines 1 to 8, this component uses interactive client-side elements
(Input, Textarea with onChange) but lacks the Next.js "use client" directive;
add the literal "use client" directive as the very first line of the file so the
component runs on the client, preserving existing imports and component code.
| <div> | ||
| <Label htmlFor="approvers">Approvers (comma-separated user IDs)</Label> | ||
| <Input | ||
| id="approvers" | ||
| value={approversString} | ||
| onChange={(e) => handleApproversChange(e.target.value)} | ||
| placeholder="user1@example.com, user2@example.com" | ||
| disabled={readOnly} | ||
| /> | ||
| <p className="text-xs text-gray-500 mt-1"> | ||
| Enter user IDs or emails separated by commas | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
Clarify approvers field terminology.
The label refers to "user IDs" (line 51) while the placeholder and hint text (lines 56, 60) reference "emails". This inconsistency may confuse users about what format to enter.
🔎 Suggested fix for consistency:
- <Label htmlFor="approvers">Approvers (comma-separated user IDs)</Label>
+ <Label htmlFor="approvers">Approvers (comma-separated emails)</Label>
<Input
id="approvers"
value={approversString}
onChange={(e) => handleApproversChange(e.target.value)}
placeholder="user1@example.com, user2@example.com"
disabled={readOnly}
/>
<p className="text-xs text-gray-500 mt-1">
- Enter user IDs or emails separated by commas
+ Enter user emails separated by commas
</p>📝 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.
| <div> | |
| <Label htmlFor="approvers">Approvers (comma-separated user IDs)</Label> | |
| <Input | |
| id="approvers" | |
| value={approversString} | |
| onChange={(e) => handleApproversChange(e.target.value)} | |
| placeholder="user1@example.com, user2@example.com" | |
| disabled={readOnly} | |
| /> | |
| <p className="text-xs text-gray-500 mt-1"> | |
| Enter user IDs or emails separated by commas | |
| </p> | |
| </div> | |
| <div> | |
| <Label htmlFor="approvers">Approvers (comma-separated emails)</Label> | |
| <Input | |
| id="approvers" | |
| value={approversString} | |
| onChange={(e) => handleApproversChange(e.target.value)} | |
| placeholder="user1@example.com, user2@example.com" | |
| disabled={readOnly} | |
| /> | |
| <p className="text-xs text-gray-500 mt-1"> | |
| Enter user emails separated by commas | |
| </p> | |
| </div> |
🤖 Prompt for AI Agents
In app/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx
around lines 50 to 62, the approvers field uses mixed terminology ("user IDs" in
the Label vs "emails" in the placeholder and hint), which is confusing; pick one
canonical term (either "user IDs" or "emails") and update the Label, Input
placeholder, and hint text to use that same term consistently, and if both
formats are accepted, change the Label to something generic like "Approvers
(comma-separated user IDs or emails)" and adjust the placeholder/hint to match
that wording so the UI is unambiguous.
| // --- HITL (Human-in-the-Loop) TYPES --- | ||
| export interface HITLRequest { | ||
| request_id: string; | ||
| execution_id: string; | ||
| node_id: string; | ||
| iteration: number; | ||
| node_type: "approval" | "input"; | ||
| message: string; | ||
| fields?: Array<{ | ||
| name: string; | ||
| type: string; | ||
| required?: boolean; | ||
| options?: string[]; | ||
| }>; | ||
| timeout_at: string; | ||
| channel: string; | ||
| created_at: string; | ||
| status: string; | ||
| approvers?: string[]; | ||
| assignee?: string; | ||
| timeout_action?: string; | ||
| loop_back_node_id?: string; | ||
| loop_back_condition?: string; | ||
| time_remaining_seconds?: number; | ||
| workflow_id?: string; | ||
| workflow_title?: string; | ||
| previous_node_result?: string; | ||
| } | ||
|
|
||
| export interface HITLResponseRequest { | ||
| response_data: Record<string, any>; | ||
| comment?: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove duplicate HITL interface definitions.
HITLRequest and HITLResponseRequest are defined twice in this file:
- First at lines 69-100 (complete, properly typed)
- Again at lines 240-268 (marked as stub with TODO comment, includes
[key: string]: any)
This duplication creates type confusion, undermines TypeScript's type safety, and increases maintenance burden. The stub version with index signatures allows any property, which could mask errors.
Remove the duplicate stub definitions (lines 240-268) and ensure all code uses the complete definitions from lines 69-100. If backward compatibility is truly needed, use a discriminated union or separate the types explicitly rather than relying on index signatures.
Also applies to: 240-268
🤖 Prompt for AI Agents
In services/WorkflowService.ts around lines 68-100 and again 240-268, there are
duplicate HITL type definitions: the full HITLRequest and HITLResponseRequest
are declared at 68-100 while a permissive stub with an index signature is
repeated at 240-268; remove the duplicate stub definitions at 240-268 and update
any local references to import/consume the canonical types declared at 68-100
instead; if some call-sites relied on the permissive index signature, replace
those usages with an explicit union or a separate permissive type (e.g.,
HITLRequestLoose) and migrate code to use the canonical types or the explicit
alternative so type safety is preserved.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/(auth)/sign-in/page.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use PascalCase for component names and camelCase for utility names
Order imports: React/Next imports first, then components, then utilities
Use Next.js error boundaries, optional chaining, and nullish coalescing for error handling
Files:
app/(auth)/sign-in/page.tsx
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.tsx: Use Tailwind CSS for styling and thecnutility for conditional classes
Use functional components with TypeScript FC types
Prefer destructuring props at the component level
Use the "use client" directive for client components
Files:
app/(auth)/sign-in/page.tsx
🔇 Additional comments (3)
app/(auth)/sign-in/page.tsx (3)
74-94: LGTM! Well-structured API payload.The structured payload approach is a good improvement over sending the raw Firebase user object. The fallback logic for
displayName, date handling with optional chaining, and clarifying comments all enhance code quality and safety.
105-110: LGTM! Improved error handling.The enhanced error handling with console logging and extracted error messages provides better debugging capabilities and user feedback.
116-116: LGTM! Helpful debugging addition.The console error logging for Firebase authentication errors aids in debugging and troubleshooting.
| toast.error(errorMessage); | ||
| }); | ||
| toast.success("Logged in successfully as " + user.displayName); | ||
| toast.success("Logged in successfully as " + (user.displayName || user.email)); |
There was a problem hiding this comment.
Add fallback for null email/displayName.
While unlikely, if both user.displayName and user.email are null or undefined, the toast will display "null" or "undefined". Consider adding a final fallback string.
🔎 Suggested fix
- toast.success("Logged in successfully as " + (user.displayName || user.email));
+ toast.success("Logged in successfully as " + (user.displayName || user.email || "user"));📝 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.
| toast.success("Logged in successfully as " + (user.displayName || user.email)); | |
| toast.success("Logged in successfully as " + (user.displayName || user.email || "user")); |
🤖 Prompt for AI Agents
In app/(auth)/sign-in/page.tsx around line 111, the toast message can show
"null" or "undefined" if both user.displayName and user.email are missing;
update the string construction to provide a final fallback (e.g.,
user.displayName || user.email || "your account") so the toast reads a sane
message when both values are null/undefined.
1 participant
Human-in-the-Loop (HITL) Feature - Frontend Implementation
Overview
This PR implements the frontend UI components for the Human-in-the-Loop (HITL) feature, enabling users to interact with HITL requests directly in the chat interface and through dedicated pages.
Changes
Key Components
Chat Integration
app/(main)/chat/[chatId]/components/HITLRequestChat.tsx- Interactive HITL component in chatapp/(main)/chat/[chatId]/components/Thread.tsx- HITL metadata parsing and renderinglib/utils/hitlMetadata.ts- Metadata parsing utilitiesWorkflow Editor
app/(main)/workflows/components/editor/nodes/manual-steps/approval-node.tsx- Approval node configurationapp/(main)/workflows/components/editor/nodes/manual-steps/input-node.tsx- Input node configuration with loop-back supportPending Requests Pages
app/(main)/workflows/pending-requests/page.tsx- List all pending HITL requestsapp/(main)/workflows/pending-requests/[requestId]/page.tsx- Detailed request view and responseService Layer
services/WorkflowService.ts- Added HITL API methods and typesFeatures
Technical Details
Related PR
Backend changes are in a separate PR in the
potpie-workflowsrepository:feat/hitl-cleanbranchSummary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.