From ea88d754d43d9b38ee718067cb33cfd34fe29066 Mon Sep 17 00:00:00 2001 From: sean-eskerium Date: Fri, 31 Oct 2025 10:32:14 -0400 Subject: [PATCH] code review cleanup --- .../components/EditRepositoryModal.tsx | 81 +++++++++++++++++-- .../components/ExecutionLogs.tsx | 7 +- .../components/SidebarRepositoryCard.tsx | 20 ++--- .../components/StepHistoryCard.tsx | 8 +- .../components/WorkOrderTable.tsx | 23 +++++- .../components/WorkflowStepButton.tsx | 15 +++- .../__tests__/CreateWorkOrderModal.test.tsx | 2 +- .../hooks/useAgentWorkOrderQueries.ts | 21 +---- .../state/slices/sseSlice.ts | 11 ++- .../components/KnowledgeInspector.tsx | 2 +- .../progress/hooks/useProgressQueries.ts | 2 +- .../src/features/ui/primitives/combobox.tsx | 2 +- 12 files changed, 134 insertions(+), 60 deletions(-) diff --git a/archon-ui-main/src/features/agent-work-orders/components/EditRepositoryModal.tsx b/archon-ui-main/src/features/agent-work-orders/components/EditRepositoryModal.tsx index 4a47eff8..5e40f2c3 100644 --- a/archon-ui-main/src/features/agent-work-orders/components/EditRepositoryModal.tsx +++ b/archon-ui-main/src/features/agent-work-orders/components/EditRepositoryModal.tsx @@ -11,6 +11,7 @@ import { Button } from "@/features/ui/primitives/button"; import { Checkbox } from "@/features/ui/primitives/checkbox"; import { Dialog, DialogContent, DialogHeader, DialogTitle } from "@/features/ui/primitives/dialog"; import { Label } from "@/features/ui/primitives/label"; +import { SimpleTooltip, TooltipProvider } from "@/features/ui/primitives/tooltip"; import { useUpdateRepository } from "../hooks/useRepositoryQueries"; import { useAgentWorkOrdersStore } from "../state/agentWorkOrdersStore"; import type { WorkflowStep } from "../types"; @@ -50,15 +51,32 @@ export function EditRepositoryModal({ open, onOpenChange }: EditRepositoryModalP useEffect(() => { if (repository) { setSelectedSteps(repository.default_commands); + setError(""); } }, [repository]); + /** + * Check if any selected steps depend on the given step + */ + const hasSelectedDependents = (step: WorkflowStep): boolean => { + return selectedSteps.some((selectedStep) => { + const stepDef = WORKFLOW_STEPS.find((s) => s.value === selectedStep); + return stepDef?.dependsOn?.includes(step) ?? false; + }); + }; + /** * Toggle workflow step selection + * Prevents unchecking a step if other selected steps depend on it */ const toggleStep = (step: WorkflowStep) => { setSelectedSteps((prev) => { if (prev.includes(step)) { + // Attempting to uncheck - check if any selected steps depend on this one + if (hasSelectedDependents(step)) { + // Don't allow unchecking if dependents exist + return prev; + } return prev.filter((s) => s !== step); } return [...prev, step]; @@ -163,27 +181,78 @@ export function EditRepositoryModal({ open, onOpenChange }: EditRepositoryModalP {/* Right Column (1/3 width) - Workflow Steps */}
+
{WORKFLOW_STEPS.map((step) => { const isSelected = selectedSteps.includes(step.value); - const isDisabled = isStepDisabled(step); + const isDisabledForEnable = isStepDisabled(step); + const hasDependents = isSelected && hasSelectedDependents(step.value); + const cannotUncheck = hasDependents; + const isCheckboxDisabled = isDisabledForEnable || cannotUncheck; - return ( -
+ // Get dependent step names for tooltip message + const dependentSteps = isSelected + ? selectedSteps + .filter((selectedStep) => { + const stepDef = WORKFLOW_STEPS.find((s) => s.value === selectedStep); + return stepDef?.dependsOn?.includes(step.value) ?? false; + }) + .map((depStep) => { + const stepDef = WORKFLOW_STEPS.find((s) => s.value === depStep); + return stepDef?.label ?? depStep; + }) + : []; + + const tooltipMessage = cannotUncheck + ? `Cannot uncheck: ${dependentSteps.join(", ")} ${dependentSteps.length === 1 ? "depends" : "depend"} on this step` + : isDisabledForEnable && step.dependsOn + ? `Requires: ${step.dependsOn.map((dep) => WORKFLOW_STEPS.find((s) => s.value === dep)?.label ?? dep).join(", ")}` + : undefined; + + const checkbox = ( !isDisabled && toggleStep(step.value)} - disabled={isDisabled} + onCheckedChange={() => { + if (!isCheckboxDisabled) { + toggleStep(step.value); + } + }} + disabled={isCheckboxDisabled} aria-label={step.label} + className={cannotUncheck ? "cursor-not-allowed opacity-75" : ""} /> -
+

Commit and PR require Execute

diff --git a/archon-ui-main/src/features/agent-work-orders/components/ExecutionLogs.tsx b/archon-ui-main/src/features/agent-work-orders/components/ExecutionLogs.tsx index 0b094fea..71d3194e 100644 --- a/archon-ui-main/src/features/agent-work-orders/components/ExecutionLogs.tsx +++ b/archon-ui-main/src/features/agent-work-orders/components/ExecutionLogs.tsx @@ -1,6 +1,4 @@ -import { Trash2 } from "lucide-react"; import { useEffect, useRef, useState } from "react"; -import { Button } from "@/features/ui/primitives/button"; import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@/features/ui/primitives/select"; import { cn } from "@/features/ui/primitives/styles"; import { Switch } from "@/features/ui/primitives/switch"; @@ -32,6 +30,7 @@ function formatRelativeTime(timestamp: string): string { const logTime = new Date(timestamp).getTime(); const diffSeconds = Math.floor((now - logTime) / 1000); + if (diffSeconds < 0) return "just now"; if (diffSeconds < 60) return `${diffSeconds}s ago`; if (diffSeconds < 3600) return `${Math.floor(diffSeconds / 60)}m ago`; return `${Math.floor(diffSeconds / 3600)}h ago`; @@ -137,10 +136,6 @@ export function ExecutionLogs({ logs, isLive = false }: ExecutionLogsProps) { - {/* Clear logs button */} - diff --git a/archon-ui-main/src/features/agent-work-orders/components/SidebarRepositoryCard.tsx b/archon-ui-main/src/features/agent-work-orders/components/SidebarRepositoryCard.tsx index 5bd0d39d..21adfa4d 100644 --- a/archon-ui-main/src/features/agent-work-orders/components/SidebarRepositoryCard.tsx +++ b/archon-ui-main/src/features/agent-work-orders/components/SidebarRepositoryCard.tsx @@ -10,6 +10,7 @@ import { StatPill } from "@/features/ui/primitives/pill"; import { SelectableCard } from "@/features/ui/primitives/selectable-card"; import { cn } from "@/features/ui/primitives/styles"; import { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from "@/features/ui/primitives/tooltip"; +import { copyToClipboard } from "@/features/shared/utils/clipboard"; import { useAgentWorkOrdersStore } from "../state/agentWorkOrdersStore"; import type { ConfiguredRepository } from "../types/repository"; @@ -40,19 +41,6 @@ export interface SidebarRepositoryCardProps { }; } -/** - * Copy text to clipboard - */ -async function copyToClipboard(text: string): Promise { - try { - await navigator.clipboard.writeText(text); - return true; - } catch (err) { - console.error("Failed to copy:", err); - return false; - } -} - /** * Static lookup map for background gradient classes */ @@ -105,9 +93,11 @@ export function SidebarRepositoryCard({ const handleCopyUrl = async (e: React.MouseEvent) => { e.stopPropagation(); - const success = await copyToClipboard(repository.repository_url); - if (success) { + const result = await copyToClipboard(repository.repository_url); + if (result.success) { console.log("Repository URL copied to clipboard"); + } else { + console.error("Failed to copy repository URL:", result.error); } }; diff --git a/archon-ui-main/src/features/agent-work-orders/components/StepHistoryCard.tsx b/archon-ui-main/src/features/agent-work-orders/components/StepHistoryCard.tsx index 9bf092c2..a8d93dbd 100644 --- a/archon-ui-main/src/features/agent-work-orders/components/StepHistoryCard.tsx +++ b/archon-ui-main/src/features/agent-work-orders/components/StepHistoryCard.tsx @@ -32,11 +32,12 @@ export const StepHistoryCard = ({ step, isExpanded, onToggle, document }: StepHi const [hasChanges, setHasChanges] = useState(false); const handleToggleEdit = () => { - if (!isEditingDocument && document) { + // Only initialize editedContent from document when entering edit mode and there's no existing draft + if (!isEditingDocument && document && !editedContent) { setEditedContent(document.content.markdown); } setIsEditingDocument(!isEditingDocument); - setHasChanges(false); + // Don't clear hasChanges when toggling - preserve unsaved drafts }; const handleContentChange = (value: string) => { @@ -224,7 +225,8 @@ export const StepHistoryCard = ({ step, isExpanded, onToggle, document }: StepHi ), }} > - {document.content.markdown} + {/* Prefer displaying live draft (editedContent) when non-empty/hasChanges over original document content */} + {editedContent && hasChanges ? editedContent : document.content.markdown} )} diff --git a/archon-ui-main/src/features/agent-work-orders/components/WorkOrderTable.tsx b/archon-ui-main/src/features/agent-work-orders/components/WorkOrderTable.tsx index c4163335..0dd4ab00 100644 --- a/archon-ui-main/src/features/agent-work-orders/components/WorkOrderTable.tsx +++ b/archon-ui-main/src/features/agent-work-orders/components/WorkOrderTable.tsx @@ -5,7 +5,7 @@ * and expandable real-time stats. */ -import { useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { useRepositories } from "../hooks/useRepositoryQueries"; import type { AgentWorkOrder } from "../types"; import { WorkOrderRow } from "./WorkOrderRow"; @@ -30,6 +30,7 @@ interface EnhancedWorkOrder extends AgentWorkOrder { export function WorkOrderTable({ workOrders, selectedRepositoryId, onStartWorkOrder }: WorkOrderTableProps) { const [justStartedId, setJustStartedId] = useState(null); + const timeoutRef = useRef(null); const { data: repositories = [] } = useRepositories(); // Create a map of repository URL to display name for quick lookup @@ -63,10 +64,28 @@ export function WorkOrderTable({ workOrders, selectedRepositoryId, onStartWorkOr setJustStartedId(id); onStartWorkOrder(id); + // Clear any existing timeout before scheduling a new one + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + } + // Clear the tracking after animation - setTimeout(() => setJustStartedId(null), 1000); + timeoutRef.current = setTimeout(() => { + setJustStartedId(null); + timeoutRef.current = null; + }, 1000); }; + // Cleanup timeout on unmount to prevent setState after unmount + useEffect(() => { + return () => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + timeoutRef.current = null; + } + }; + }, []); + // Show empty state if no work orders if (filteredWorkOrders.length === 0) { return ( diff --git a/archon-ui-main/src/features/agent-work-orders/components/WorkflowStepButton.tsx b/archon-ui-main/src/features/agent-work-orders/components/WorkflowStepButton.tsx index df59f018..e3288263 100644 --- a/archon-ui-main/src/features/agent-work-orders/components/WorkflowStepButton.tsx +++ b/archon-ui-main/src/features/agent-work-orders/components/WorkflowStepButton.tsx @@ -61,7 +61,16 @@ export const WorkflowStepButton: React.FC = ({ }, }; - const styles = colorMap[color]; + // Label colors matching the color prop + const labelColorMap = { + purple: "text-purple-400 dark:text-purple-300", + green: "text-green-400 dark:text-green-300", + blue: "text-blue-400 dark:text-blue-300", + cyan: "text-cyan-400 dark:text-cyan-300", + }; + + const styles = colorMap[color] || colorMap.cyan; + const labelColor = labelColorMap[color] || labelColorMap.cyan; return (
@@ -153,9 +162,9 @@ export const WorkflowStepButton: React.FC = ({ className={cn( "text-xs font-medium transition-colors", isCompleted - ? "text-cyan-400 dark:text-cyan-300" + ? labelColor : isActive - ? "text-blue-500 dark:text-blue-400" + ? labelColor : "text-gray-500 dark:text-gray-400", )} > diff --git a/archon-ui-main/src/features/agent-work-orders/components/__tests__/CreateWorkOrderModal.test.tsx b/archon-ui-main/src/features/agent-work-orders/components/__tests__/CreateWorkOrderModal.test.tsx index 5478ac88..58de7593 100644 --- a/archon-ui-main/src/features/agent-work-orders/components/__tests__/CreateWorkOrderModal.test.tsx +++ b/archon-ui-main/src/features/agent-work-orders/components/__tests__/CreateWorkOrderModal.test.tsx @@ -87,7 +87,7 @@ describe("CreateWorkOrderModal", () => { render(, { wrapper }); // Try to submit without filling required fields - const submitButton = screen.getByText("Create Work Order"); + const submitButton = screen.getByRole("button", { name: "Create Work Order" }); await user.click(submitButton); // Should show validation error diff --git a/archon-ui-main/src/features/agent-work-orders/hooks/useAgentWorkOrderQueries.ts b/archon-ui-main/src/features/agent-work-orders/hooks/useAgentWorkOrderQueries.ts index c387b1bb..5bf9cb6f 100644 --- a/archon-ui-main/src/features/agent-work-orders/hooks/useAgentWorkOrderQueries.ts +++ b/archon-ui-main/src/features/agent-work-orders/hooks/useAgentWorkOrderQueries.ts @@ -96,7 +96,7 @@ export function useWorkOrderLogs( }, ) { return useQuery({ - queryKey: workOrderId ? agentWorkOrderKeys.logs(workOrderId) : DISABLED_QUERY_KEY, + queryKey: workOrderId ? [...agentWorkOrderKeys.logs(workOrderId), options] : DISABLED_QUERY_KEY, queryFn: () => workOrderId ? agentWorkOrdersService.getWorkOrderLogs(workOrderId, options) @@ -150,11 +150,9 @@ export function useStartWorkOrder() { onMutate: async (id) => { // Cancel any outgoing refetches await queryClient.cancelQueries({ queryKey: agentWorkOrderKeys.detail(id) }); - await queryClient.cancelQueries({ queryKey: agentWorkOrderKeys.lists() }); // Snapshot the previous values const previousWorkOrder = queryClient.getQueryData(agentWorkOrderKeys.detail(id)); - const previousList = queryClient.getQueryData(agentWorkOrderKeys.lists()); // Optimistically update the work order status to "running" if (previousWorkOrder) { @@ -165,15 +163,9 @@ export function useStartWorkOrder() { }; queryClient.setQueryData(agentWorkOrderKeys.detail(id), optimisticWorkOrder); - - // Update in list as well if present - queryClient.setQueryData(agentWorkOrderKeys.lists(), (old) => { - if (!old) return old; - return old.map((wo) => (wo.agent_work_order_id === id ? optimisticWorkOrder : wo)); - }); } - return { previousWorkOrder, previousList }; + return { previousWorkOrder }; }, onError: (error, id, context) => { @@ -183,18 +175,13 @@ export function useStartWorkOrder() { if (context?.previousWorkOrder) { queryClient.setQueryData(agentWorkOrderKeys.detail(id), context.previousWorkOrder); } - if (context?.previousList) { - queryClient.setQueryData(agentWorkOrderKeys.lists(), context.previousList); - } }, onSuccess: (data, id) => { // Replace optimistic update with server response queryClient.setQueryData(agentWorkOrderKeys.detail(id), data); - queryClient.setQueryData(agentWorkOrderKeys.lists(), (old) => { - if (!old) return [data]; - return old.map((wo) => (wo.agent_work_order_id === id ? data : wo)); - }); + // Invalidate all list queries to refetch with server data + queryClient.invalidateQueries({ queryKey: agentWorkOrderKeys.lists() }); }, }); } diff --git a/archon-ui-main/src/features/agent-work-orders/state/slices/sseSlice.ts b/archon-ui-main/src/features/agent-work-orders/state/slices/sseSlice.ts index 062ea233..c53f42b7 100644 --- a/archon-ui-main/src/features/agent-work-orders/state/slices/sseSlice.ts +++ b/archon-ui-main/src/features/agent-work-orders/state/slices/sseSlice.ts @@ -108,8 +108,6 @@ export const createSSESlice: StateCreator = (set, ge }; eventSource.onerror = () => { - const currentState = get(); - set((state) => ({ connectionStates: { ...state.connectionStates, @@ -120,8 +118,13 @@ export const createSSESlice: StateCreator = (set, ge // Auto-reconnect after 5 seconds setTimeout(() => { eventSource.close(); - const connections = currentState.logConnections; - connections.delete(workOrderId); + // Use set() to properly update state instead of mutating stale reference + set((state) => { + const newConnections = new Map(state.logConnections); + newConnections.delete(workOrderId); + return { logConnections: newConnections }; + }); + // Use fresh get() to ensure we have the latest state before retry get().connectToLogs(workOrderId); // Retry }, 5000); }; diff --git a/archon-ui-main/src/features/knowledge/inspector/components/KnowledgeInspector.tsx b/archon-ui-main/src/features/knowledge/inspector/components/KnowledgeInspector.tsx index daf8c65f..55a7f767 100644 --- a/archon-ui-main/src/features/knowledge/inspector/components/KnowledgeInspector.tsx +++ b/archon-ui-main/src/features/knowledge/inspector/components/KnowledgeInspector.tsx @@ -36,7 +36,7 @@ export const KnowledgeInspector: React.FC = ({ useEffect(() => { setViewMode(initialTab); setSelectedItem(null); // Clear selected item when switching tabs - }, [initialTab]); + }, [initialTab, item.source_id]); // Use pagination hook for current view mode const paginationData = useInspectorPagination({ diff --git a/archon-ui-main/src/features/progress/hooks/useProgressQueries.ts b/archon-ui-main/src/features/progress/hooks/useProgressQueries.ts index d5686731..55635c82 100644 --- a/archon-ui-main/src/features/progress/hooks/useProgressQueries.ts +++ b/archon-ui-main/src/features/progress/hooks/useProgressQueries.ts @@ -45,7 +45,7 @@ export function useOperationProgress( hasCalledComplete.current = false; hasCalledError.current = false; consecutiveNotFound.current = 0; - }, []); + }, [progressId]); const query = useQuery({ queryKey: progressId ? progressKeys.detail(progressId) : DISABLED_QUERY_KEY, diff --git a/archon-ui-main/src/features/ui/primitives/combobox.tsx b/archon-ui-main/src/features/ui/primitives/combobox.tsx index 928fb08e..fcba540e 100644 --- a/archon-ui-main/src/features/ui/primitives/combobox.tsx +++ b/archon-ui-main/src/features/ui/primitives/combobox.tsx @@ -164,7 +164,7 @@ export const ComboBox = React.forwardRef( const highlightedElement = optionsRef.current.querySelector('[data-highlighted="true"]'); highlightedElement?.scrollIntoView({ block: "nearest" }); } - }, [open]); + }, [open, highlightedIndex]); return (