code review cleanup

This commit is contained in:
sean-eskerium
2025-10-31 10:32:14 -04:00
parent 7eabeebe5f
commit ea88d754d4
12 changed files with 134 additions and 60 deletions

View File

@@ -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 */}
<div className="space-y-4">
<Label>Default Workflow Steps</Label>
<TooltipProvider>
<div className="space-y-2">
{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 (
<div key={step.value} className="flex items-center gap-2">
// 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 = (
<Checkbox
id={`edit-step-${step.value}`}
checked={isSelected}
onCheckedChange={() => !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" : ""}
/>
<Label htmlFor={`edit-step-${step.value}`} className={isDisabled ? "text-gray-400" : ""}>
);
return (
<div key={step.value} className="flex items-center gap-2">
{tooltipMessage ? (
<SimpleTooltip content={tooltipMessage} side="right">
{checkbox}
</SimpleTooltip>
) : (
checkbox
)}
<Label
htmlFor={`edit-step-${step.value}`}
className={
isCheckboxDisabled
? "text-gray-400 dark:text-gray-500 cursor-not-allowed"
: "cursor-pointer"
}
>
{step.label}
{cannotUncheck && (
<span className="ml-1 text-xs text-cyan-500 dark:text-cyan-400" aria-hidden="true">
(locked)
</span>
)}
</Label>
</div>
);
})}
</div>
</TooltipProvider>
<p className="text-xs text-gray-500 dark:text-gray-400">Commit and PR require Execute</p>
</div>
</div>

View File

@@ -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) {
</span>
</div>
{/* Clear logs button */}
<Button variant="ghost" size="xs" aria-label="Clear logs">
<Trash2 className="w-3 h-3" aria-hidden="true" />
</Button>
</div>
</div>

View File

@@ -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<boolean> {
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);
}
};

View File

@@ -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}
</ReactMarkdown>
</div>
)}

View File

@@ -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<string | null>(null);
const timeoutRef = useRef<NodeJS.Timeout | null>(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 (

View File

@@ -61,7 +61,16 @@ export const WorkflowStepButton: React.FC<WorkflowStepButtonProps> = ({
},
};
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 (
<div className="flex flex-col items-center gap-2">
@@ -153,9 +162,9 @@ export const WorkflowStepButton: React.FC<WorkflowStepButtonProps> = ({
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",
)}
>

View File

@@ -87,7 +87,7 @@ describe("CreateWorkOrderModal", () => {
render(<CreateWorkOrderModal open={true} onOpenChange={vi.fn()} />, { 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

View File

@@ -96,7 +96,7 @@ export function useWorkOrderLogs(
},
) {
return useQuery<WorkOrderLogsResponse, Error>({
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<AgentWorkOrder>(agentWorkOrderKeys.detail(id));
const previousList = queryClient.getQueryData<AgentWorkOrder[]>(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<AgentWorkOrder[]>(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<AgentWorkOrder[]>(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() });
},
});
}

View File

@@ -108,8 +108,6 @@ export const createSSESlice: StateCreator<SSESlice, [], [], SSESlice> = (set, ge
};
eventSource.onerror = () => {
const currentState = get();
set((state) => ({
connectionStates: {
...state.connectionStates,
@@ -120,8 +118,13 @@ export const createSSESlice: StateCreator<SSESlice, [], [], SSESlice> = (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);
};

View File

@@ -36,7 +36,7 @@ export const KnowledgeInspector: React.FC<KnowledgeInspectorProps> = ({
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({

View File

@@ -45,7 +45,7 @@ export function useOperationProgress(
hasCalledComplete.current = false;
hasCalledError.current = false;
consecutiveNotFound.current = 0;
}, []);
}, [progressId]);
const query = useQuery<ProgressResponse | null>({
queryKey: progressId ? progressKeys.detail(progressId) : DISABLED_QUERY_KEY,

View File

@@ -164,7 +164,7 @@ export const ComboBox = React.forwardRef<HTMLButtonElement, ComboBoxProps>(
const highlightedElement = optionsRef.current.querySelector('[data-highlighted="true"]');
highlightedElement?.scrollIntoView({ block: "nearest" });
}
}, [open]);
}, [open, highlightedIndex]);
return (
<Popover.Root open={open} onOpenChange={setOpen}>