From 9a4373dacdd1a10e463e8b91de8e0f8f5b8043bf Mon Sep 17 00:00:00 2001 From: Devarshi Shimpi Date: Fri, 3 Jul 2026 03:09:54 +0530 Subject: [PATCH 01/12] add: tabs and select component for motion UI --- .../features/job-detail/job-findings-list.tsx | 28 +- src/client/components/motion/tabs.tsx | 189 ++++++++ src/client/components/ui/dropdown-menu.tsx | 433 ++++++++++-------- src/client/components/ui/select.tsx | 309 ++++++++++--- src/client/lib/ease.ts | 1 + wrangler.jsonc | 4 +- 6 files changed, 689 insertions(+), 275 deletions(-) create mode 100644 src/client/components/motion/tabs.tsx create mode 100644 src/client/lib/ease.ts diff --git a/src/client/components/features/job-detail/job-findings-list.tsx b/src/client/components/features/job-detail/job-findings-list.tsx index 084eb1f..dd8b821 100644 --- a/src/client/components/features/job-detail/job-findings-list.tsx +++ b/src/client/components/features/job-detail/job-findings-list.tsx @@ -1,8 +1,8 @@ import { useState } from 'react'; import { FileText } from 'lucide-react'; -import { cn } from '@client/lib/utils'; import type { JobDetail } from '@shared/schema'; import { reviewSeverities } from '@shared/schema'; +import { Tabs, TabsList, TabsTrigger } from '@client/components/motion/tabs'; import { FileFinding } from './file-finding'; import { CommentCard } from './comment-card'; import { severityConfig } from './constants'; @@ -24,22 +24,16 @@ export function JobFindingsList({ job }: JobFindingsListProps) {
View by -
- {(['files', 'severity'] as const).map((view) => ( - - ))} -
+ setViewBy(v as 'files' | 'severity')} variant="segment"> + + + Files + + + Severity + + +
diff --git a/src/client/components/motion/tabs.tsx b/src/client/components/motion/tabs.tsx new file mode 100644 index 0000000..038883e --- /dev/null +++ b/src/client/components/motion/tabs.tsx @@ -0,0 +1,189 @@ +// beui.dev/components/motion/tabs +import { motion, MotionConfig, useReducedMotion, type Transition } from 'motion/react'; +import { createContext, useContext, useId, useState, type ReactNode } from 'react'; +import { EASE_OUT } from '@client/lib/ease'; +import { cn } from '@client/lib/utils'; + +type Variant = 'pill' | 'underline' | 'segment'; + +type Ctx = { + value: string; + setValue: (v: string) => void; + layoutId: string; + variant: Variant; +}; + +const TabsCtx = createContext(null); + +function useTabs() { + const ctx = useContext(TabsCtx); + if (!ctx) throw new Error('Tabs.* must be used inside '); + return ctx; +} + +// Weighty spring for the active-tab indicator: a touch of overshoot so it +// settles with life instead of snapping. +const transition: Transition = { + type: 'spring', + stiffness: 170, + damping: 24, + mass: 1.2, +}; + +export function Tabs({ + defaultValue, + value, + onValueChange, + variant = 'pill', + children, + className, +}: { + defaultValue?: string; + value?: string; + onValueChange?: (v: string) => void; + variant?: Variant; + children: ReactNode; + className?: string; +}) { + const [internal, setInternal] = useState(defaultValue ?? ''); + const layoutId = useId(); + const reduce = useReducedMotion(); + const controlled = value !== undefined; + const current = controlled ? value : internal; + const setValue = (v: string) => { + if (!controlled) setInternal(v); + onValueChange?.(v); + }; + return ( + + + {/* layoutRoot: the indicator's layoutId measures in page coordinates, so + inside fixed/scrolled containers it would replay scroll offsets as + movement. The pill only ever travels within the list, so scoping + projection to the Tabs wrapper is always correct. */} + + {children} + + + + ); +} + +const listClasses: Record = { + pill: 'inline-flex items-center gap-1 rounded-full bg-card p-1', + underline: 'inline-flex items-center gap-1 border-b border-border', + segment: 'inline-flex items-center gap-0 rounded-lg bg-card p-0.5', +}; + +export function TabsList({ children, className }: { children: ReactNode; className?: string }) { + const { variant } = useTabs(); + return ( +
+ {children} +
+ ); +} + +export function TabsTrigger({ + value, + children, + className, + indicatorClassName, +}: { + value: string; + children: ReactNode; + className?: string; + indicatorClassName?: string; +}) { + const { value: current, setValue, layoutId, variant } = useTabs(); + const active = current === value; + + if (variant === 'underline') { + return ( + + ); + } + + // Pill + Segment use the same trick: a max-contrast pill slides via layoutId, + // text uses `mix-blend-exclusion` so it inverts dynamically against the moving bg. + const radius = variant === 'pill' ? 'rounded-full' : 'rounded-md'; + + return ( +
+ {active ? ( + + ) : null} + +
+ ); +} + +export function TabsContent({ + value, + children, + className, +}: { + value: string; + children: ReactNode; + className?: string; +}) { + const { value: current } = useTabs(); + const reduce = useReducedMotion(); + const active = current === value; + // Inactive panels stay mounted but hidden, so their content (e.g. source + // code) is present in the server-rendered HTML for crawlers and assistive + // tech, instead of being dropped from the DOM. + if (!active) { + return ( + + ); + } + return ( + + {children} + + ); +} diff --git a/src/client/components/ui/dropdown-menu.tsx b/src/client/components/ui/dropdown-menu.tsx index b10afc0..ce0551a 100644 --- a/src/client/components/ui/dropdown-menu.tsx +++ b/src/client/components/ui/dropdown-menu.tsx @@ -1,209 +1,238 @@ -import * as React from 'react'; -import * as DropdownMenuPrimitive from '@radix-ui/react-dropdown-menu'; -import { Check, ChevronRight, Circle } from 'lucide-react'; +import { Slot } from '@radix-ui/react-slot'; +import { motion, useReducedMotion, type Transition } from 'motion/react'; +import { + createContext, + useContext, + useEffect, + useId, + useLayoutEffect, + useRef, + useState, + type MouseEvent, + type ReactNode, + type RefCallback, +} from 'react'; +import { createPortal } from 'react-dom'; import { cn } from '@client/lib/utils'; -const DropdownMenu = DropdownMenuPrimitive.Root; - -const DropdownMenuTrigger = DropdownMenuPrimitive.Trigger; - -const DropdownMenuGroup = DropdownMenuPrimitive.Group; - -const DropdownMenuPortal = DropdownMenuPrimitive.Portal; - -const DropdownMenuSub = DropdownMenuPrimitive.Sub; - -const DropdownMenuRadioGroup = DropdownMenuPrimitive.RadioGroup; - -const menuContentClass = - 'z-50 min-w-[8rem] overflow-hidden rounded-lg border border-zinc-200 bg-white p-1 text-zinc-900 shadow-sm shadow-black/[0.02] dark:border-border dark:bg-popover dark:text-popover-foreground dark:shadow-black/50 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 duration-200'; - -const menuItemClass = - 'relative flex cursor-default select-none items-center rounded-md text-sm outline-none transition-colors hover:bg-zinc-200 hover:text-zinc-900 focus:bg-zinc-200 focus:text-zinc-900 data-[highlighted]:bg-zinc-200 data-[highlighted]:text-zinc-900 dark:hover:bg-primary/[0.12] dark:hover:text-foreground dark:focus:bg-primary/[0.12] dark:focus:text-foreground dark:data-[highlighted]:bg-primary/[0.12] dark:data-[highlighted]:text-foreground data-[disabled]:pointer-events-none data-[disabled]:opacity-50'; - -const DropdownMenuSubTrigger = React.forwardRef< - React.ElementRef, - React.ComponentPropsWithoutRef & { - inset?: boolean; - } ->(({ className, inset, children, ...props }, ref) => ( - - {children} - - -)); -DropdownMenuSubTrigger.displayName = - DropdownMenuPrimitive.SubTrigger.displayName; - -const DropdownMenuSubContent = React.forwardRef< - React.ElementRef, - React.ComponentPropsWithoutRef ->(({ className, ...props }, ref) => ( - -)); -DropdownMenuSubContent.displayName = - DropdownMenuPrimitive.SubContent.displayName; - -const DropdownMenuContent = React.forwardRef< - React.ElementRef, - React.ComponentPropsWithoutRef ->(({ className, sideOffset = 4, ...props }, ref) => ( - - void; + triggerRef: React.RefObject; + panelRef: React.RefObject; + contentId: string; +} + +const DropdownMenuContext = createContext(null); + +function useMenuCtx(component: string) { + const ctx = useContext(DropdownMenuContext); + if (!ctx) throw new Error(`${component} must be used within `); + return ctx; +} + +export function DropdownMenu({ children }: { children: ReactNode }) { + const [open, setOpen] = useState(false); + const triggerRef = useRef(null); + const panelRef = useRef(null); + const contentId = `${useId()}-menu`; + + useEffect(() => { + if (!open) return; + const onKey = (e: KeyboardEvent) => e.key === 'Escape' && setOpen(false); + const onPointer = (e: PointerEvent) => { + const target = e.target as Node; + if (triggerRef.current?.contains(target)) return; + if (panelRef.current?.contains(target)) return; + setOpen(false); + }; + window.addEventListener('keydown', onKey); + window.addEventListener('pointerdown', onPointer); + return () => { + window.removeEventListener('keydown', onKey); + window.removeEventListener('pointerdown', onPointer); + }; + }, [open]); + + return ( + + {children} + + ); +} + +export function DropdownMenuTrigger({ + asChild, + children, +}: { + asChild?: boolean; + children: ReactNode; +}) { + const ctx = useMenuCtx('DropdownMenuTrigger'); + const Comp = asChild ? Slot : 'button'; + const setRef: RefCallback = (node) => { + ctx.triggerRef.current = node; + }; + + return ( + ctx.setOpen(!ctx.open)} + > + {children} + + ); +} + +export interface DropdownMenuContentProps { + side?: Side; + align?: Align; + sideOffset?: number; + alignOffset?: number; + className?: string; + children: ReactNode; +} + +export function DropdownMenuContent({ + side = 'bottom', + align = 'start', + sideOffset = 4, + alignOffset = 0, + className, + children, +}: DropdownMenuContentProps) { + const ctx = useMenuCtx('DropdownMenuContent'); + const reduce = useReducedMotion() ?? false; + const [style, setStyle] = useState({}); + // Mount once on first open and keep mounted so the close animation can play + // (matches the pattern used by ui/select.tsx). + const [mounted, setMounted] = useState(false); + + const setRefs: RefCallback = (node) => { + ctx.panelRef.current = node; + }; + + useEffect(() => { + if (ctx.open) setMounted(true); + }, [ctx.open]); + + useLayoutEffect(() => { + if (!ctx.open) return; + const trigger = ctx.triggerRef.current; + if (!trigger) return; + const update = () => { + const r = trigger.getBoundingClientRect(); + const next: React.CSSProperties = { position: 'fixed' }; + + if (side === 'bottom') next.top = r.bottom + sideOffset; + else if (side === 'top') next.bottom = window.innerHeight - r.top + sideOffset; + else if (side === 'right') next.left = r.right + sideOffset; + else next.right = window.innerWidth - r.left + sideOffset; + + if (side === 'top' || side === 'bottom') { + if (align === 'start') next.left = r.left + alignOffset; + else next.right = window.innerWidth - r.right + alignOffset; + } else { + if (align === 'start') next.top = r.top + alignOffset; + else next.bottom = window.innerHeight - r.bottom + alignOffset; + } + + setStyle(next); + }; + update(); + window.addEventListener('scroll', update, true); + window.addEventListener('resize', update); + return () => { + window.removeEventListener('scroll', update, true); + window.removeEventListener('resize', update); + }; + }, [ctx.open, ctx.triggerRef, side, align, sideOffset, alignOffset]); + + const originX = side === 'top' || side === 'bottom' ? (align === 'start' ? 'left' : 'right') : side === 'right' ? 'left' : 'right'; + const originY = side === 'bottom' ? 'top' : side === 'top' ? 'bottom' : align === 'start' ? 'top' : 'bottom'; + const slideDistance = 6; + const hiddenOffset = + side === 'bottom' + ? { y: -slideDistance } + : side === 'top' + ? { y: slideDistance } + : side === 'right' + ? { x: -slideDistance } + : { x: slideDistance }; + + const transition: Transition = reduce + ? { duration: 0.1 } + : { type: 'spring', duration: 0.35, bounce: 0.15 }; + + if (!mounted) return null; + + return createPortal( + - -)); -DropdownMenuContent.displayName = DropdownMenuPrimitive.Content.displayName; - -const DropdownMenuItem = React.forwardRef< - React.ElementRef, - React.ComponentPropsWithoutRef & { - inset?: boolean; - } ->(({ className, inset, ...props }, ref) => ( - -)); -DropdownMenuItem.displayName = DropdownMenuPrimitive.Item.displayName; - -const DropdownMenuCheckboxItem = React.forwardRef< - React.ElementRef, - React.ComponentPropsWithoutRef ->(({ className, children, checked, ...props }, ref) => ( - - - - - - - {children} - -)); -DropdownMenuCheckboxItem.displayName = - DropdownMenuPrimitive.CheckboxItem.displayName; - -const DropdownMenuRadioItem = React.forwardRef< - React.ElementRef, - React.ComponentPropsWithoutRef ->(({ className, children, ...props }, ref) => ( - - - - - - - {children} - -)); -DropdownMenuRadioItem.displayName = DropdownMenuPrimitive.RadioItem.displayName; - -const DropdownMenuLabel = React.forwardRef< - React.ElementRef, - React.ComponentPropsWithoutRef & { - inset?: boolean; - } ->(({ className, inset, ...props }, ref) => ( - -)); -DropdownMenuLabel.displayName = DropdownMenuPrimitive.Label.displayName; - -const DropdownMenuSeparator = React.forwardRef< - React.ElementRef, - React.ComponentPropsWithoutRef ->(({ className, ...props }, ref) => ( - -)); -DropdownMenuSeparator.displayName = DropdownMenuPrimitive.Separator.displayName; - -const DropdownMenuShortcut = ({ - className, - ...props -}: React.HTMLAttributes) => { + > + {children} + , + document.body, + ); +} + +export interface DropdownMenuItemProps { + asChild?: boolean; + className?: string; + onClick?: (e: MouseEvent) => void; + children: ReactNode; +} + +export function DropdownMenuItem({ asChild, className, onClick, children }: DropdownMenuItemProps) { + const ctx = useMenuCtx('DropdownMenuItem'); + const Comp = asChild ? Slot : 'button'; + return ( - + ) => { + onClick?.(e); + ctx.setOpen(false); + }} + className={cn( + 'relative flex w-full cursor-default select-none items-center rounded-md px-2 py-1.5 text-sm outline-none transition-colors', + 'hover:bg-zinc-200 hover:text-zinc-900 focus:bg-zinc-200 focus:text-zinc-900', + 'dark:hover:bg-primary/[0.12] dark:hover:text-foreground dark:focus:bg-primary/[0.12] dark:focus:text-foreground', + className, + )} + > + {children} + ); -}; -DropdownMenuShortcut.displayName = 'DropdownMenuShortcut'; - -export { - DropdownMenu, - DropdownMenuTrigger, - DropdownMenuContent, - DropdownMenuItem, - DropdownMenuCheckboxItem, - DropdownMenuRadioItem, - DropdownMenuLabel, - DropdownMenuSeparator, - DropdownMenuShortcut, - DropdownMenuGroup, - DropdownMenuPortal, - DropdownMenuSub, - DropdownMenuSubContent, - DropdownMenuSubTrigger, - DropdownMenuRadioGroup, -}; +} + +export function DropdownMenuSeparator({ className }: { className?: string }) { + return
; +} diff --git a/src/client/components/ui/select.tsx b/src/client/components/ui/select.tsx index 0f419aa..f5f9bbc 100644 --- a/src/client/components/ui/select.tsx +++ b/src/client/components/ui/select.tsx @@ -1,13 +1,32 @@ -import { ChevronDown } from 'lucide-react'; -import type { CSSProperties, ReactNode } from 'react'; -import { cn } from '@client/lib/utils'; +import { Check, ChevronDown } from 'lucide-react'; +import { motion, type Transition, useReducedMotion, type Variants } from 'motion/react'; import { - DropdownMenu, - DropdownMenuContent, - DropdownMenuItem, - DropdownMenuTrigger, -} from './dropdown-menu'; -import { Button } from './button'; + type CSSProperties, + type ReactNode, + useEffect, + useId, + useLayoutEffect, + useRef, + useState, +} from 'react'; +import { createPortal } from 'react-dom'; +import { cn } from '@client/lib/utils'; +import { EASE_OUT } from '@client/lib/ease'; + +// Spring with bounce powers the unfold/separation; per-property timings in the +// content choreograph it. See the `animate`/`transition` props on the panel below. +const CHEVRON_TRANSITION: Transition = { type: 'spring', duration: 0.4, bounce: 0.3 }; + +const LIST_VARIANTS: Variants = { + hidden: {}, + show: { transition: { staggerChildren: 0.035, delayChildren: 0.05 } }, +}; +const ITEM_VARIANTS: Variants = { + hidden: { opacity: 0, y: -6, filter: 'blur(3px)' }, + show: { opacity: 1, y: 0, filter: 'blur(0px)' }, +}; + +type Placement = 'bottom' | 'top'; interface SelectOption { value: string; @@ -26,9 +45,9 @@ interface SelectProps { leadingIcon?: ReactNode; /** * 'page' — trigger sits on the gray page background (e.g. "Last 30 days"). - * Dropdown gets white bg so it lifts off the page. - * 'card' — trigger sits inside a white card (e.g. "All statuses"). - * Dropdown gets zinc-50 bg so it's distinguishable from the card. + * Dropdown gets card bg so it lifts off the page. + * 'card' — trigger sits inside a card. + * Dropdown gets muted bg so it's distinguishable from the card. * Defaults to 'page'. */ variant?: 'page' | 'card'; @@ -46,8 +65,93 @@ export function Select({ leadingIcon, variant = 'page', }: SelectProps) { + const reduce = useReducedMotion() ?? false; + const baseId = useId(); + const triggerId = `${baseId}-trigger`; + const listId = `${baseId}-list`; + const triggerRef = useRef(null); + const panelRef = useRef(null); + const innerRef = useRef(null); + const [open, setOpen] = useState(false); + const [placement, setPlacement] = useState('bottom'); + const [height, setHeight] = useState(0); + const [rect, setRect] = useState<{ left: number; width: number; top: number; bottom: number } | null>(null); + const selectedOption = options.find((opt) => opt.value === value); + // close on outside pointer / escape + useEffect(() => { + if (!open) return; + const onKey = (e: KeyboardEvent) => e.key === 'Escape' && setOpen(false); + const onPointer = (e: PointerEvent) => { + const target = e.target as Node; + if (triggerRef.current?.contains(target)) return; + if (panelRef.current?.contains(target)) return; + setOpen(false); + }; + window.addEventListener('keydown', onKey); + window.addEventListener('pointerdown', onPointer); + return () => { + window.removeEventListener('keydown', onKey); + window.removeEventListener('pointerdown', onPointer); + }; + }, [open]); + + useLayoutEffect(() => { + const node = innerRef.current; + if (!node) return; + const measure = () => setHeight(node.offsetHeight); + measure(); + const observer = new ResizeObserver(measure); + observer.observe(node); + return () => observer.disconnect(); + }); + + // Track the trigger's viewport position so the portaled panel can follow it, + // and flip upward when there isn't room below and there's more above. + useLayoutEffect(() => { + if (!open) return; + const trigger = triggerRef.current; + if (!trigger) return; + const update = () => { + const r = trigger.getBoundingClientRect(); + setRect({ left: r.left, width: r.width, top: r.top, bottom: r.bottom }); + const h = innerRef.current?.offsetHeight ?? 0; + const below = window.innerHeight - r.bottom; + const above = r.top; + setPlacement(below < h + 16 && above > below ? 'top' : 'bottom'); + }; + update(); + window.addEventListener('scroll', update, true); + window.addEventListener('resize', update); + return () => { + window.removeEventListener('scroll', update, true); + window.removeEventListener('resize', update); + }; + }, [open]); + + const isTop = placement === 'top'; + + // Gooey: the edge facing the panel snaps flat (panel attached) then rounds + // back once the panel pulls away — the two pinch apart. + const kf = open ? [0, 0, 12] : [12, 0, 12]; + const kfT: Transition = reduce + ? { duration: 0 } + : open + ? { duration: 0.6, times: [0, 0.4, 1], ease: EASE_OUT } + : { duration: 0.42, times: [0, 0.5, 1], ease: EASE_OUT }; + const flatT: Transition = { duration: 0 }; + + const nearGap = open ? 8 : 0; + const nearRadius = open ? 12 : 0; + const gapT: Transition = open + ? { type: 'spring', duration: 0.6, bounce: 0.5, delay: 0.12 } + : { type: 'spring', duration: 0.3, bounce: 0.1 }; + const radiusT: Transition = open + ? { duration: 0.3, ease: EASE_OUT, delay: 0.14 } + : { duration: 0.16, ease: EASE_OUT }; + const instant: Transition = { duration: 0 }; + return (
{label && ( @@ -55,50 +159,147 @@ export function Select({ {label} )} - - - - - + + + + + +
+ + {/* Portaled to so the panel always renders above cards, tables, and + other stacking contexts — it can't be clipped/hidden by an ancestor. */} + {createPortal( + round; far corners stay rounded + borderTopLeftRadius: isTop ? 12 : nearRadius, + borderTopRightRadius: isTop ? 12 : nearRadius, + borderBottomLeftRadius: isTop ? nearRadius : 12, + borderBottomRightRadius: isTop ? nearRadius : 12, + } + } + transition={ + reduce + ? { duration: 0.12 } + : { + opacity: open ? { duration: 0.18 } : { duration: 0.16, delay: 0.12 }, + height: open + ? { type: 'spring', duration: 0.42, bounce: 0.14 } + : { duration: 0.26, ease: EASE_OUT, delay: 0.14 }, + marginTop: isTop ? instant : gapT, + marginBottom: isTop ? gapT : instant, + borderTopLeftRadius: isTop ? instant : radiusT, + borderTopRightRadius: isTop ? instant : radiusT, + borderBottomLeftRadius: isTop ? radiusT : instant, + borderBottomRightRadius: isTop ? radiusT : instant, + } + } + style={{ + position: 'fixed', + left: rect?.left ?? 0, + width: rect?.width ?? 0, + top: isTop ? undefined : (rect?.bottom ?? 0), + bottom: isTop ? window.innerHeight - (rect?.top ?? 0) : undefined, + transformOrigin: isTop ? 'bottom' : 'top', + overflow: 'hidden', + pointerEvents: open ? 'auto' : 'none', + }} + // flush against the trigger, then separates into its own rounded pill; + // sits above or below depending on available space + className="z-50 border border-border bg-popover shadow-lg shadow-black/[0.03] dark:shadow-black/40" > - {options.map((option) => ( - onValueChange(option.value)} - className={cn( - 'cursor-pointer whitespace-normal break-words py-2', - value === option.value && - 'bg-primary/10 font-medium text-primary dark:bg-primary/[0.12] dark:text-primary', - )} - > - {option.label} - - ))} - - + + {options.map((option) => { + const selected = option.value === value; + return ( + + + + ); + })} + + , + document.body, + )}
); } diff --git a/src/client/lib/ease.ts b/src/client/lib/ease.ts new file mode 100644 index 0000000..8abd8d1 --- /dev/null +++ b/src/client/lib/ease.ts @@ -0,0 +1 @@ +export const EASE_OUT = [0.16, 1, 0.3, 1] as const; diff --git a/wrangler.jsonc b/wrangler.jsonc index 77def02..a5be170 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -97,8 +97,8 @@ ] }, "vars": { - "APP_URL": "https://app.codra.devarshi.dev", - "AUTH_CALLBACK_URL": "https://app.codra.devarshi.dev/auth/github/callback", + "APP_URL": "http://localhost:8787", + "AUTH_CALLBACK_URL": "http://localhost:8787/auth/github/callback", "BOT_USERNAME": "codra-app", "GITHUB_APP_SLUG": "codra-app-personal", "DASHBOARD_ALLOWED_USERS": "devarshishimpi", From 03b150696685e4db5c2dd79e7da461bc962d16dc Mon Sep 17 00:00:00 2001 From: Devarshi Shimpi Date: Fri, 3 Jul 2026 05:21:54 +0530 Subject: [PATCH 02/12] feat: add review performance settings and UI components - Implemented database migration to insert default review settings. - Created a SteppedSlider component for selecting concurrency levels and max comments. - Added ConfirmDialog component for user confirmation on exceeding limits. - Updated SettingsPage to manage and display review settings with automatic saving. - Introduced API routes for fetching and updating review settings. - Enhanced review logic to respect concurrency and comment limits based on user settings. --- .../004_review_performance_settings.sql | 4 + src/client/app.css | 45 +-- .../components/motion/stepped-slider.tsx | 321 ++++++++++++++++++ src/client/components/ui/confirm-dialog.tsx | 59 ++++ src/client/lib/api.ts | 11 +- src/client/main.tsx | 1 - src/client/pages/settings.tsx | 164 ++++++++- src/server/app.ts | 2 + src/server/core/review.ts | 32 +- src/server/db/app-settings.ts | 39 +++ src/server/routes/api/settings.ts | 26 ++ src/shared/schema.ts | 17 + 12 files changed, 668 insertions(+), 53 deletions(-) create mode 100644 db/migrations/004_review_performance_settings.sql create mode 100644 src/client/components/motion/stepped-slider.tsx create mode 100644 src/client/components/ui/confirm-dialog.tsx create mode 100644 src/server/db/app-settings.ts create mode 100644 src/server/routes/api/settings.ts diff --git a/db/migrations/004_review_performance_settings.sql b/db/migrations/004_review_performance_settings.sql new file mode 100644 index 0000000..794c252 --- /dev/null +++ b/db/migrations/004_review_performance_settings.sql @@ -0,0 +1,4 @@ +INSERT INTO global_settings (key, value) VALUES + ('review_concurrency_level', 'medium'), + ('review_max_comments', '10') +ON CONFLICT (key) DO NOTHING; diff --git a/src/client/app.css b/src/client/app.css index b761a31..f478402 100644 --- a/src/client/app.css +++ b/src/client/app.css @@ -903,8 +903,7 @@ gap: 0.625rem !important; padding: 0.75rem 0.875rem !important; border-radius: 0.625rem !important; - border-width: 1px !important; - border-style: solid !important; + border: none !important; font-family: var(--font-sans) !important; font-size: 0.8125rem !important; line-height: 1.45 !important; @@ -915,7 +914,6 @@ /* light defaults (overridden per-variant below) */ background: oklch(99.5% 0.004 115) !important; - border-color: oklch(88% 0.008 115) !important; color: oklch(15% 0.02 115) !important; /* smooth entrance */ @@ -924,7 +922,6 @@ .dark .codra-toast { background: oklch(13% 0.018 115) !important; - border-color: oklch(22% 0.022 115) !important; color: oklch(94% 0.006 115) !important; box-shadow: 0 6px 24px oklch(0% 0 0 / 0.5), @@ -985,15 +982,11 @@ /* ── SUCCESS ─────────────────────────────────────── */ .codra-toast-success { - background: oklch(98.5% 0.045 115) !important; - border-color: oklch(82% 0.16 115) !important; - color: oklch(28% 0.10 115) !important; + color: oklch(28% 0.10 115) !important; } .dark .codra-toast-success { - background: oklch(16% 0.08 115) !important; - border-color: oklch(32% 0.14 115) !important; - color: oklch(90% 0.18 115) !important; + color: oklch(90% 0.18 115) !important; } .codra-toast-success .codra-toast-description { @@ -1008,15 +1001,11 @@ /* ── ERROR ───────────────────────────────────────── */ .codra-toast-error { - background: oklch(98.5% 0.03 25) !important; - border-color: oklch(80% 0.14 25) !important; - color: oklch(32% 0.14 25) !important; + color: oklch(32% 0.14 25) !important; } .dark .codra-toast-error { - background: oklch(15% 0.07 25) !important; - border-color: oklch(35% 0.14 25) !important; - color: oklch(85% 0.08 25) !important; + color: oklch(85% 0.08 25) !important; } .codra-toast-error .codra-toast-description { @@ -1031,15 +1020,11 @@ /* ── LOADING ─────────────────────────────────────── */ .codra-toast-loading { - background: oklch(98% 0.004 115) !important; - border-color: oklch(86% 0.010 115) !important; - color: oklch(20% 0.020 115) !important; + color: oklch(20% 0.020 115) !important; } .dark .codra-toast-loading { - background: oklch(14% 0.020 115) !important; - border-color: oklch(24% 0.025 115) !important; - color: oklch(88% 0.008 115) !important; + color: oklch(88% 0.008 115) !important; } /* spinner inherits accent color */ @@ -1049,26 +1034,18 @@ /* ── WARNING ─────────────────────────────────────── */ .codra-toast-warning { - background: oklch(98.5% 0.04 65) !important; - border-color: oklch(82% 0.13 65) !important; - color: oklch(35% 0.12 65) !important; + color: oklch(35% 0.12 65) !important; } .dark .codra-toast-warning { - background: oklch(16% 0.08 65) !important; - border-color: oklch(35% 0.14 65) !important; - color: oklch(82% 0.14 65) !important; + color: oklch(82% 0.14 65) !important; } /* ── INFO ────────────────────────────────────────── */ .codra-toast-info { - background: oklch(98.5% 0.03 250) !important; - border-color: oklch(80% 0.12 250) !important; - color: oklch(30% 0.12 250) !important; + color: oklch(30% 0.12 250) !important; } .dark .codra-toast-info { - background: oklch(15% 0.07 250) !important; - border-color: oklch(33% 0.12 250) !important; - color: oklch(80% 0.12 250) !important; + color: oklch(80% 0.12 250) !important; } diff --git a/src/client/components/motion/stepped-slider.tsx b/src/client/components/motion/stepped-slider.tsx new file mode 100644 index 0000000..beb46ab --- /dev/null +++ b/src/client/components/motion/stepped-slider.tsx @@ -0,0 +1,321 @@ +// Discrete-step slider with a spring-smoothed thumb and a live value readout +// that sits statically above the track (no floating/portal-positioned +// tooltip involved). Adapted from a min/max/step numeric range slider so +// labeled steps (e.g. Low/Medium/High/Max) line up exactly on evenly spaced +// stops. +import { + motion, + useMotionTemplate, + useMotionValue, + useReducedMotion, + useSpring, + useTransform, +} from 'motion/react'; +import { + type KeyboardEvent, + type PointerEvent, + useCallback, + useEffect, + useMemo, + useRef, + useState, +} from 'react'; + +import { cn } from '@client/lib/utils'; +import { useIsDarkMode } from '@client/hooks/use-is-dark-mode'; + +const SPRING_GLIDE = { stiffness: 700, damping: 50, mass: 0.5 } as const; +const SPRING_BOUNCY = { type: 'spring', stiffness: 500, damping: 14, mass: 0.7 } as const; + +// Deterministic 0..1 seed derived from the slider's id, so two sliders maxed +// out at the same time get slightly different hues and animation timing +// instead of pulsing in perfect, obviously-copy-pasted unison. +function seedFromId(id: string | undefined) { + if (!id) return 0; + let hash = 0; + for (let i = 0; i < id.length; i += 1) { + hash = (hash * 31 + id.charCodeAt(i)) >>> 0; + } + return (hash % 997) / 997; +} + +export interface SteppedSliderStep { + value: number; + label: string; +} + +export interface SteppedSliderProps { + id?: string; + value?: number; + defaultValue?: number; + onValueChange?: (value: number) => void; + min?: number; + max?: number; + step?: number; + /** Tick dots + labels rendered at each defined step. */ + steps?: SteppedSliderStep[]; + /** Live value readout shown above the track, and used as the accessible aria-valuetext. Defaults to the raw value. */ + formatValue?: (value: number) => string; + disabled?: boolean; + className?: string; + 'aria-label'?: string; + 'aria-labelledby'?: string; +} + +const clamp = (v: number, lo: number, hi: number) => Math.min(hi, Math.max(lo, v)); + +export function SteppedSlider({ + id, + value, + defaultValue = 0, + onValueChange, + min = 0, + max = 100, + step = 1, + steps = [], + formatValue, + disabled = false, + className, + 'aria-label': ariaLabel, + 'aria-labelledby': ariaLabelledBy, +}: SteppedSliderProps) { + const reduce = useReducedMotion(); + const isDark = useIsDarkMode(); + const seed = useMemo(() => seedFromId(id), [id]); + const trackRef = useRef(null); + const [internal, setInternal] = useState(defaultValue); + const [active, setActive] = useState(false); + // Live position while dragging, decoupled from the committed value so that + // onValueChange fires exactly once per gesture (on release) instead of on + // every pointer-move tick — callers may react to a commit by opening a + // confirmation dialog, which must not re-fire while the pointer is still down. + const [dragValue, setDragValue] = useState(null); + const controlled = value !== undefined; + const committedValue = clamp(controlled ? (value as number) : internal, min, max); + const current = active && dragValue !== null ? clamp(dragValue, min, max) : committedValue; + const percent = ((current - min) / (max - min)) * 100; + const isMaxed = current === max; + + // Teal accent for the "maxed out" state, hue-shifted slightly per instance + // and timed with a per-instance offset so simultaneous max sliders read as + // one cohesive design rather than a mirrored duplicate. + const hue = 185 + (seed - 0.5) * 16; + const glowColor = isDark ? `oklch(68% 0.14 ${hue})` : `oklch(52% 0.13 ${hue})`; + const fillGradient = isDark + ? `linear-gradient(90deg, oklch(28% 0.08 ${hue}), oklch(62% 0.12 ${hue}))` + : `linear-gradient(90deg, oklch(93% 0.03 ${hue}), oklch(56% 0.13 ${hue}))`; + const dotDriftDuration = 1.4 + seed * 0.5; + const glowDuration = 1.6 + seed * 0.6; + + const target = useMotionValue(percent); + useEffect(() => { + target.set(percent); + }, [percent, target]); + const smooth = useSpring(target, SPRING_GLIDE); + const pos = reduce ? target : smooth; + const left = useMotionTemplate`${pos}%`; + const thumbX = useTransform(pos, (p) => `${-p}%`); + + const snapValue = useCallback( + (next: number) => clamp(Math.round((next - min) / step) * step + min, min, max), + [min, max, step], + ); + + const commit = useCallback( + (next: number) => { + const snapped = snapValue(next); + if (!controlled) setInternal(snapped); + onValueChange?.(snapped); + }, + [controlled, onValueChange, snapValue], + ); + + const valueFromX = useCallback( + (clientX: number) => { + const rect = trackRef.current?.getBoundingClientRect(); + if (!rect) return committedValue; + const ratio = clamp((clientX - rect.left) / rect.width, 0, 1); + return min + ratio * (max - min); + }, + [committedValue, min, max], + ); + + const onPointerDown = useCallback( + (event: PointerEvent) => { + if (disabled) return; + event.currentTarget.setPointerCapture(event.pointerId); + setActive(true); + setDragValue(snapValue(valueFromX(event.clientX))); + }, + [disabled, valueFromX, snapValue], + ); + + const onPointerMove = useCallback( + (event: PointerEvent) => { + if (!active || disabled) return; + setDragValue(snapValue(valueFromX(event.clientX))); + }, + [active, disabled, valueFromX, snapValue], + ); + + const endDrag = useCallback( + (event: PointerEvent) => { + event.currentTarget.releasePointerCapture?.(event.pointerId); + setActive(false); + setDragValue((pending) => { + if (pending !== null) commit(pending); + return null; + }); + }, + [commit], + ); + + const onKeyDown = useCallback( + (event: KeyboardEvent) => { + if (disabled) return; + const map: Record = { + ArrowRight: committedValue + step, + ArrowUp: committedValue + step, + ArrowLeft: committedValue - step, + ArrowDown: committedValue - step, + Home: min, + End: max, + }; + if (event.key in map) { + event.preventDefault(); + commit(map[event.key]); + } + }, + [disabled, committedValue, step, min, max, commit], + ); + + const valueLabel = formatValue ? formatValue(current) : String(current); + + return ( +
+ {/* live value readout — sits in normal flow above the track, always visible */} +
+ {valueLabel} +
+ +
+ {/* fill — runs from the left edge to the thumb; static teal gradient with a dot texture + steadily drifting left-to-right at the top step. Every dot has the same fixed + brightness — only position moves — so it reads as one smooth, consistent motion + instead of some dots being brighter than others. The pattern repeats every 10px and + shifts by exactly one period, so the loop point is never visible. */} + + {isMaxed ? ( +
+ +
+ ) : ( +
+ )} + + + {/* ticks — slight inset so the end dots don't clip; sized/ringed up at the top step so they + read as distinct markers instead of blending into the dot texture behind them */} +
+ {steps.map((tick) => { + const tp = ((tick.value - min) / (max - min)) * 100; + return ( + + ); + })} +
+ + {/* pulsing glow halo behind the thumb at the top step — kept as its own layer so only + opacity (a plain number) is animated, since Motion's box-shadow interpolator can't + parse CSS custom properties inside the color stops */} + {isMaxed && !reduce && ( + + )} + + {/* vertical bar thumb — contained at both ends via thumbX */} + +
+ + {steps.length > 0 && ( +
+
+ {steps.map((tick, index) => { + const isFirst = index === 0; + const isLast = index === steps.length - 1; + const tp = ((tick.value - min) / (max - min)) * 100; + return ( + + {tick.label} + + ); + })} +
+
+ )} +
+ ); +} diff --git a/src/client/components/ui/confirm-dialog.tsx b/src/client/components/ui/confirm-dialog.tsx new file mode 100644 index 0000000..260be9c --- /dev/null +++ b/src/client/components/ui/confirm-dialog.tsx @@ -0,0 +1,59 @@ +import * as Dialog from '@radix-ui/react-dialog'; +import { AlertTriangle } from 'lucide-react'; +import { Button } from '@client/components/ui/button'; + +interface ConfirmDialogProps { + open: boolean; + onOpenChange: (open: boolean) => void; + title: string; + description: string; + confirmLabel?: string; + cancelLabel?: string; + onConfirm: () => void; +} + +export function ConfirmDialog({ + open, + onOpenChange, + title, + description, + confirmLabel = 'Continue', + cancelLabel = 'Cancel', + onConfirm, +}: ConfirmDialogProps) { + return ( + + + + +
+ + + +
+ {title} + + {description} + +
+
+ +
+ + + + +
+ + + + ); +} diff --git a/src/client/lib/api.ts b/src/client/lib/api.ts index d218b3c..c4a02b5 100644 --- a/src/client/lib/api.ts +++ b/src/client/lib/api.ts @@ -10,7 +10,7 @@ import type { SyncReposResponse, UpdatesEmailResponse, } from '@shared/api'; -import type { LlmApiFormat, LlmProvider, ModelConfig, RepoConfig } from '@shared/schema'; +import type { LlmApiFormat, LlmProvider, ModelConfig, RepoConfig, ReviewSettings } from '@shared/schema'; const SAFE_METHODS = new Set(['GET', 'HEAD', 'OPTIONS']); @@ -244,4 +244,13 @@ export const api = { body: JSON.stringify(config), }); }, + getReviewSettings() { + return request<{ settings: ReviewSettings }>('/api/settings'); + }, + updateReviewSettings(settings: ReviewSettings) { + return request<{ ok: boolean }>('/api/settings', { + method: 'PATCH', + body: JSON.stringify(settings), + }); + }, }; diff --git a/src/client/main.tsx b/src/client/main.tsx index 0bb5880..989fdaf 100644 --- a/src/client/main.tsx +++ b/src/client/main.tsx @@ -26,7 +26,6 @@ function ToasterWrapper() { = { + low: 'Low', + medium: 'Medium', + high: 'High', + max: 'Max', +}; +const CONCURRENCY_STEPS = CONCURRENCY_LEVEL_ORDER.map(level => ({ + value: REVIEW_CONCURRENCY_LIMITS[level], + label: CONCURRENCY_LEVEL_LABEL[level], +})); +const CONCURRENCY_VALUE_TO_LEVEL: Record = Object.fromEntries( + CONCURRENCY_LEVEL_ORDER.map(level => [REVIEW_CONCURRENCY_LIMITS[level], level]), +) as Record; +const CONCURRENCY_MAX_VALUE = REVIEW_CONCURRENCY_LIMITS.max; +const MAX_COMMENTS_STEPS = reviewMaxCommentsOptions.map(n => ({ value: n, label: String(n) })); +const MAX_COMMENTS_CEILING = reviewMaxCommentsOptions[reviewMaxCommentsOptions.length - 1]; + type ProviderDraft = LlmProvider & { apiKey: string }; type NewProviderDraft = { preset: string; @@ -221,9 +243,9 @@ function SectionCard({ } /* ─── Field label ─────────────────────────────────────────────────────────── */ -function FieldLabel({ htmlFor, children }: { htmlFor: string; children: React.ReactNode }) { +function FieldLabel({ htmlFor, id, children }: { htmlFor: string; id?: string; children: React.ReactNode }) { return ( -
- } - > - {/* Add model form */} - {addingModel && ( -
-

New model

-
-
- Codra model ID - setNewModel(current => ({ ...current, modelId: e.target.value }))} - /> -
-
- Provider model name - setNewModel(current => ({ ...current, modelName: e.target.value }))} - /> -
- setNewModel(current => ({ ...current, [field]: parseOptionalLimit(e.target.value) }))} - /> -
- ))} -
-
- -
- - )} - - {/* Filters */} -
- -
- updateModel(cfg.modelId, { providerId })} - options={providerOptions} - /> -
- Provider model name - updateModel(cfg.modelId, { modelName: e.target.value })} - /> -
-
- {(['rpm', 'rpd', 'tpm'] as const).map(field => { - const limitId = domId(`model-${field}`, cfg.modelId); - return ( -
- {field.toUpperCase()} - updateQuota(cfg.modelId, field, parseOptionalLimit(e.target.value))} - /> -
- ); - })} -
-
-
- )} - - ); - })} - - {filteredConfigs.length === 0 && ( -
- No models match the current filters. -
- )} - - )} - - - {/* ── System Information ────────────────────────────────────────────── */} -
{/* Version row */} -
- - - -
+
+
+

Version

-

Installed Codra release

- + v{pkg.version} - +
- {/* Changelog / links row */} + {/* License row */}
- - -
-

Releases

-

Browse all versions and release notes on GitHub

+

License

+ {pkg.license} +
+ +
+ + {/* Links */} +
+ {[ + { href: `${pkg.repository.url.replace(/\.git$/, '')}/releases/`, label: 'Releases', sub: 'Version history & notes' }, + { href: pkg.homepage, label: 'Homepage', sub: 'codra.run' }, + { href: pkg.bugs.url, label: 'Report an issue', sub: 'GitHub issue tracker' }, + ].map(({ href, label, sub }) => ( - View releases - + + + {label} + + + {sub} + -
- + ))}
diff --git a/src/client/pages/stats.tsx b/src/client/pages/stats.tsx index bd7e555..921b863 100644 --- a/src/client/pages/stats.tsx +++ b/src/client/pages/stats.tsx @@ -5,24 +5,11 @@ import { Bar, BarChart, CartesianGrid, - Line, - LineChart, ResponsiveContainer, Tooltip, XAxis, YAxis, } from 'recharts'; -import { - Activity, - BarChart3, - Bot, - Gauge, - GitBranch, - RefreshCw, - Server, - TrendingUp, -} from 'lucide-react'; - import { TimeRangeSelect } from '@client/components/features/stats/time-range-select'; import { PageHeaderActions } from '@client/components/shared/page-header-actions'; import { PageHeader } from '@client/components/layout/page-header'; @@ -42,7 +29,6 @@ const CHART = { warning: '#f59e0b', danger: '#ef4444', quiet: '#64748b', - violet: '#8b5cf6', }; const axisProps = { @@ -63,11 +49,6 @@ function formatCompact(value: number) { return value >= 1000 ? fmtNumber(value) : value.toLocaleString(); } -function ratio(numerator: number, denominator: number, decimals = 1) { - if (!denominator) return '0'; - return (numerator / denominator).toFixed(decimals); -} - function percent(numerator: number, denominator: number) { if (!denominator) return 0; return Math.round((numerator / denominator) * 100); @@ -111,224 +92,46 @@ function GraphSectionTitle() { function GraphShell({ title, - eyebrow, - icon: Icon, children, className = '', }: { title: string; - eyebrow: string; - icon: typeof Activity; children: ReactNode; className?: string; }) { return ( -
-
-
-

{eyebrow}

-

{title}

-
- - - +
+
+

{title}

- {children} +
{children}
); } -function GraphCardGallery({ stats, days, isDark }: { stats: StatsPayload; days: number; isDark: boolean }) { +function MetricsGrid({ stats, days, isDark }: { stats: StatsPayload; days: number; isDark: boolean }) { const color = isDark ? CHART.primaryDark : CHART.primary; - const tokenTrend = stats.trend.map((day) => ({ - ...day, - tokens: day.inputTokens + day.outputTokens, - tokenDensity: Math.round((day.inputTokens + day.outputTokens) / Math.max(day.jobs, 1)), - commentRate: Number(ratio(day.comments, Math.max(day.jobs, 1), 1)), - })); - const cumulativeTrend = stats.trend.reduce<{ day: string; reviews: number; comments: number }[]>((acc, day) => { - const prev = acc[acc.length - 1]; - acc.push({ - day: day.day, - reviews: (prev?.reviews ?? 0) + day.jobs, - comments: (prev?.comments ?? 0) + day.comments, - }); - return acc; - }, []); const repoMax = Math.max(...stats.topRepos.map((repo) => repo.jobs), 1); const modelMax = Math.max(...stats.models.map((model) => model.calls), 1); - return ( -
- - -
- -
- - - - - - } /> - - - - -
- - - -
- - - - - - - - - - - - } /> - - - -
- -
- -
- -
- - - - - - } /> - - - - -
- - - -
- - - - - - } /> - - - -
- -
- -
- -
- {stats.topRepos.slice(0, 7).map((repo) => ( -
-
-
- {repo.owner}/{repo.repo} -
-
-
-
-
- {repo.jobs} -
- ))} -
- - - -
- {stats.models.slice(0, 7).map((model) => ( -
-
-
- {modelName(model.modelUsed)} -
-
-
-
-
- {model.calls} -
- ))} -
- -
- -
- ); -} + const STATUS_COLOR: Record = { + done: color, + running: CHART.comments, + queued: CHART.quiet, + failed: CHART.danger, + superseded: CHART.quiet, + }; + const statusTotal = Math.max(stats.statuses.reduce((sum, s) => sum + s.count, 0), 1); -function PanelHeader({ - label, - value, - detail, -}: { - label: string; - value?: string; - detail?: string; -}) { return ( -
-
- - - -
-

{label}

- {detail &&

{detail}

} -
-
- {value && ( - - {value} - - )} -
- ); -} - -function ReviewFlowCard({ - stats, - days, - isDark, -}: { - stats: StatsPayload; - days: number; - isDark: boolean; -}) { - const color = isDark ? CHART.primaryDark : CHART.primary; - const totalReviews = stats.trend.reduce((sum, day) => sum + day.jobs, 0); - const maxDay = stats.trend.reduce( - (best, day) => (day.jobs > best.jobs ? day : best), - stats.trend[0] ?? { day: '', jobs: 0 }, - ); +
+ - return ( -
-
-
- -
+
+ +
- + @@ -340,12 +143,7 @@ function ReviewFlowCard({ - + } cursor={{ stroke: color, strokeDasharray: '4 4' }} />
-
- - -
-
- ); -} + -function ReviewFlowSkeleton() { - return ( -
-
-
-
- - - -
- - -
+ +
+ {stats.topRepos.slice(0, 8).map((repo) => ( +
+
+
+ {repo.owner}/{repo.repo} +
+
+
+
+
+ {repo.jobs} +
+ ))}
-
-
-
- - -
-
- - - + + + +
+ {stats.models.slice(0, 8).map((model) => ( +
+
+
+ {modelName(model.modelUsed)} +
+
+
+
+
+ {model.calls} +
+ ))}
-
+
); @@ -444,14 +255,10 @@ function ReviewFlowSkeleton() { function GraphCardSkeleton({ className = '' }: { className?: string }) { return (
-
-
- - -
- +
+
-
+
@@ -461,15 +268,11 @@ function GraphCardSkeleton({ className = '' }: { className?: string }) { function GraphBarCardSkeleton({ className = '' }: { className?: string }) { return (
-
-
- - -
- +
+
-
- {Array.from({ length: 5 }).map((_, i) => ( +
+ {Array.from({ length: 8 }).map((_, i) => (
@@ -483,21 +286,18 @@ function GraphBarCardSkeleton({ className = '' }: { className?: string }) { ); } -function GraphCardGallerySkeleton() { +function MetricsGridSkeleton() { return ( -
+
diff --git a/src/server/core/review.ts b/src/server/core/review.ts index 8103d8f..d75e0e6 100644 --- a/src/server/core/review.ts +++ b/src/server/core/review.ts @@ -52,12 +52,32 @@ const MAX_RETRYABLE_FILE_REVIEW_FAILURES = 3; // every prepare/review-chunk/finalize phase. 6h comfortably covers even a job that // hits every retryable-failure backoff (up to 15 min each, several times over). const DIFF_CACHE_TTL_SECONDS = 6 * 60 * 60; -// Conservative worst-case subrequest cost of reviewing one file: up to ~3 models in a -// fallback chain (primary + fallbacks), each costing up to ~3 subrequests (model config -// lookup, an optional provider-availability check, and the provider call itself). Used only -// to size how many files can safely be reviewed concurrently in a chunk given the job's -// remaining subrequest budget for this invocation -- see budgetAwareChunkFileLimit below. -const ESTIMATED_SUBREQUESTS_PER_FILE = 9; +// Estimated subrequest cost of reviewing one file, used only to size how many files can +// safely be reviewed concurrently in a chunk given the job's remaining subrequest budget for +// this invocation (see budgetAwareChunkFileLimit below). A file walks a fallback chain of up +// to ~3 models, but the per-model model-config lookup is now cached per invocation +// (ModelService.resolveModel), so the recurring cost per file is ~1 provider call per model +// tried plus the persisted-review write -- roughly 5 in the worst case rather than 9. Lower +// estimate => more files reviewed in parallel per chunk within the same 50-subrequest cap. +const ESTIMATED_SUBREQUESTS_PER_FILE = 5; + +/** + * How many files a single review chunk may process concurrently: the configured concurrency + * level, capped only by what the invocation's remaining subrequest budget can safely cover. + * + * The cap is deliberately sized so it does NOT silently override the user's chosen concurrency + * at a healthy budget -- that would make the concurrency setting a no-op above the cap. With + * MAX_SUBREQUESTS 50, SAFE_MARGIN 22 and ESTIMATED_SUBREQUESTS_PER_FILE 5, a fresh invocation + * yields floor(28/5) = 5, comfortably above the highest configured level (max = 4) even after + * the getPullRequest preamble spends a couple of subrequests. It only throttles (down to 1) + * once earlier failures in this invocation have actually eaten into the budget; any files a + * throttled chunk can't reach simply roll into the next chunk. The + * chunk-file-limit-honors-configured-level invariant is pinned by a regression test. + */ +export function budgetAwareFileLimit(remainingSafeBudget: number, configuredChunkFileLimit: number) { + const budgetLimit = Math.max(1, Math.floor(remainingSafeBudget / ESTIMATED_SUBREQUESTS_PER_FILE)); + return Math.min(configuredChunkFileLimit, budgetLimit); +} function isRetryableFileReviewErrorMessage(message: string | null | undefined) { if (!message) return false; @@ -537,14 +557,11 @@ async function runReviewPhase( const totalLineCount = files.reduce((sum, file) => sum + file.lineCount, 0); const { concurrencyLevel } = await getReviewSettings(env); const configuredChunkFileLimit = REVIEW_CONCURRENCY_LIMITS[concurrencyLevel]; - // Cap this chunk's concurrency by the shared job's remaining subrequest budget, so a run - // of model/provider failures earlier in this invocation can't push it over Cloudflare's - // per-invocation subrequest cap (Workers Free plan: 50). While the budget is healthy this - // is a no-op and the chunk still runs at the fully configured concurrency for speed; it - // only throttles down (to as little as 1 file at a time) once failures have actually eaten - // into the budget, and any files it can't get to this chunk simply roll into the next one. - const budgetAwareChunkFileLimit = Math.max(1, Math.floor(tracker.remainingSafeBudget() / ESTIMATED_SUBREQUESTS_PER_FILE)); - const reviewChunkFileLimit = Math.min(configuredChunkFileLimit, budgetAwareChunkFileLimit); + // Cap this chunk's concurrency by the invocation's remaining subrequest budget so a run of + // model/provider failures can't push it over Cloudflare's per-invocation cap (Workers Free + // plan: 50) -- but sized (see budgetAwareFileLimit) so the configured concurrency level is + // honored in full at a healthy budget and only throttled once the budget is actually spent. + const reviewChunkFileLimit = budgetAwareFileLimit(tracker.remainingSafeBudget(), configuredChunkFileLimit); const startedAt = Date.now(); let processedThisChunk = 0; diff --git a/src/server/db/stats.ts b/src/server/db/stats.ts index a4727d4..fd74a3c 100644 --- a/src/server/db/stats.ts +++ b/src/server/db/stats.ts @@ -1,8 +1,13 @@ import type { AppBindings } from '@server/env'; import { queryRows } from './client'; -import { statsSchema } from '@shared/schema'; +import { statsSchema, jobStatuses, reviewTriggers, reviewSeverities, reviewCategories } from '@shared/schema'; import { getModelUsageStats } from './file-reviews'; +const jobStatusSet = new Set(jobStatuses); +const reviewTriggerSet = new Set(reviewTriggers); +const reviewSeveritySet = new Set(reviewSeverities); +const reviewCategorySet = new Set(reviewCategories); + export async function getStats(env: Pick, days = 30) { const parsedDays = Number(days); const safeDays = Number.isFinite(parsedDays) ? Math.trunc(parsedDays) : 30; @@ -133,10 +138,13 @@ export async function getStats(env: Pick, days = 30) outputTokens: row.output_tokens ?? 0, })), topRepos: topRepos.map((row) => ({ owner: row.owner, repo: row.repo, jobs: row.jobs })), - statuses: statusRows.map((row) => ({ status: row.status as any, count: row.count })), - triggers: triggerRows.map((row) => ({ trigger: row.trigger as any, count: row.count })), - severities: severityRows.map((row) => ({ severity: row.severity as any, count: row.count })), - categories: categoryRows.map((row) => ({ category: row.category as any, count: row.count })), + // Drop any rows whose enum-typed column holds an unexpected value (e.g. legacy + // rows back-migrated from JSON, where severity/category have no DB CHECK + // constraint). Keeping them would fail statsSchema.parse and 500 the endpoint. + statuses: statusRows.filter((row) => jobStatusSet.has(row.status)).map((row) => ({ status: row.status as (typeof jobStatuses)[number], count: row.count })), + triggers: triggerRows.filter((row) => reviewTriggerSet.has(row.trigger)).map((row) => ({ trigger: row.trigger as (typeof reviewTriggers)[number], count: row.count })), + severities: severityRows.filter((row) => reviewSeveritySet.has(row.severity)).map((row) => ({ severity: row.severity as (typeof reviewSeverities)[number], count: row.count })), + categories: categoryRows.filter((row) => reviewCategorySet.has(row.category)).map((row) => ({ category: row.category as (typeof reviewCategories)[number], count: row.count })), performance: { avgDurationMs: performanceRow?.avg_duration_ms != null ? Math.round(performanceRow.avg_duration_ms) : null, p95DurationMs: performanceRow?.p95_duration_ms != null ? Math.round(performanceRow.p95_duration_ms) : null, diff --git a/src/server/env.ts b/src/server/env.ts index 0ab9fc6..e15e2a3 100644 --- a/src/server/env.ts +++ b/src/server/env.ts @@ -1,7 +1,7 @@ import type { ReviewJobMessage } from '@shared/schema'; export interface WorkersAiBinding { - run(model: string, input: Record): Promise; + run(model: string, input: Record, options?: { signal?: AbortSignal }): Promise; } export interface QueueProducer { diff --git a/src/server/models/cloudflare.ts b/src/server/models/cloudflare.ts index 8436c75..e3c51ee 100644 --- a/src/server/models/cloudflare.ts +++ b/src/server/models/cloudflare.ts @@ -3,8 +3,14 @@ import type { AppBindings } from '@server/env'; import { TimeoutError } from '@server/core/timeout'; import { ProviderRequestError, type ModelResponse } from './types'; -/** Max wall-clock time allowed for a single Workers-AI call. */ -const CLOUDFLARE_TIMEOUT_MS = 180_000; +/** + * Max wall-clock time allowed for a single Workers-AI call. Kept well under the review + * workflow's 15-minute step timeout: a model that hasn't answered a code-review prompt in + * this long (reasoning models under strict-JSON decoding are the usual offenders -- they burn + * the whole token budget "thinking" and never emit the JSON) is not going to, so we fail fast + * and let the file defer to a fresh invocation instead of stalling the whole review. + */ +const CLOUDFLARE_TIMEOUT_MS = 60_000; const CLOUDFLARE_MAX_RETRIES = 0; const CLOUDFLARE_MAX_OUTPUT_TOKENS = 8192; const REVIEW_RESPONSE_SCHEMA = { @@ -168,8 +174,16 @@ export async function reviewWithCloudflare( for (let attempt = 0; attempt <= maxRetries; attempt++) { let timer: ReturnType | undefined; + // Abort the underlying Workers-AI request when the timeout fires. Promise.race on its own + // only stops *us* awaiting -- the subrequest would keep running in the background, holding + // this invocation's wall-clock and pushing the workflow toward its 15-minute step cap long + // after we've given up. Aborting via the AI binding's signal actually cancels it. + const controller = new AbortController(); const timeoutPromise = new Promise((_, reject) => { - timer = setTimeout(() => reject(new TimeoutError(`Cloudflare (${model})`, CLOUDFLARE_TIMEOUT_MS)), CLOUDFLARE_TIMEOUT_MS); + timer = setTimeout(() => { + controller.abort(); + reject(new TimeoutError(`Cloudflare (${model})`, CLOUDFLARE_TIMEOUT_MS)); + }, CLOUDFLARE_TIMEOUT_MS); }); try { @@ -182,29 +196,30 @@ export async function reviewWithCloudflare( logger.info(`Calling Cloudflare model: ${model}`); const startTime = Date.now(); - const result = await Promise.race([ - env.AI.run(model as any, { - messages: [ - { - role: 'system', - content: `${input.systemPrompt}\n\nReturn only the JSON object. Do not include chain-of-thought, analysis, markdown, code fences, or explanatory prose.`, - }, - { role: 'user', content: `${input.userPrompt}\n\nRespond with the required JSON object only.` }, - ], - max_completion_tokens: CLOUDFLARE_MAX_OUTPUT_TOKENS, - response_format: { - type: 'json_schema', - json_schema: { - name: 'codra_file_review', - strict: true, - schema: REVIEW_RESPONSE_SCHEMA, - }, + const runPromise = env.AI.run(model as any, { + messages: [ + { + role: 'system', + content: `${input.systemPrompt}\n\nReturn only the JSON object. Do not include chain-of-thought, analysis, markdown, code fences, or explanatory prose.`, }, - temperature: 0, - top_p: 0.1, - }), - timeoutPromise, - ]); + { role: 'user', content: `${input.userPrompt}\n\nRespond with the required JSON object only.` }, + ], + max_completion_tokens: CLOUDFLARE_MAX_OUTPUT_TOKENS, + response_format: { + type: 'json_schema', + json_schema: { + name: 'codra_file_review', + strict: true, + schema: REVIEW_RESPONSE_SCHEMA, + }, + }, + temperature: 0, + top_p: 0.1, + }, { signal: controller.signal }); + // Once the timeout wins the race the aborted run still settles (as a rejection); attach a + // no-op handler so that late rejection can't surface as an unhandled promise rejection. + runPromise.catch(() => {}); + const result = await Promise.race([runPromise, timeoutPromise]); const durationMs = Date.now() - startTime; logger.info(`AI model ${model} responded in ${durationMs}ms`); diff --git a/src/server/routes/api/settings.ts b/src/server/routes/api/settings.ts index 2ec484b..fd14cef 100644 --- a/src/server/routes/api/settings.ts +++ b/src/server/routes/api/settings.ts @@ -1,8 +1,20 @@ import { Hono } from 'hono'; +import { z } from 'zod'; import type { AppEnv } from '@server/env'; import { getReviewSettings, updateReviewSettings } from '@server/db/app-settings'; import { jsonError } from '@server/core/http'; -import { reviewSettingsSchema } from '@shared/schema'; +import { reviewConcurrencyLevels, reviewMaxCommentsOptions, reviewSettingsSchema } from '@shared/schema'; + +const reviewSettingsPatchSchema = z.object({ + concurrencyLevel: z.enum(reviewConcurrencyLevels).optional(), + maxComments: z.number().int().refine( + (value) => (reviewMaxCommentsOptions as readonly number[]).includes(value), + 'Invalid max comments value.', + ).optional(), +}).strict().refine( + (settings) => settings.concurrencyLevel !== undefined || settings.maxComments !== undefined, + 'At least one setting must be provided.', +); export function createSettingsRouter() { const app = new Hono(); @@ -13,13 +25,15 @@ export function createSettingsRouter() { }); app.patch('/', async (c) => { - const parsed = reviewSettingsSchema.safeParse(await c.req.json()); + const parsed = reviewSettingsPatchSchema.safeParse(await c.req.json()); if (!parsed.success) { return jsonError('Invalid review settings.', 400); } - await updateReviewSettings(c.env, parsed.data); - return c.json({ ok: true }); + const current = await getReviewSettings(c.env); + const next = reviewSettingsSchema.parse({ ...current, ...parsed.data }); + await updateReviewSettings(c.env, next); + return c.json({ ok: true, settings: next }); }); return app; diff --git a/src/server/services/model.ts b/src/server/services/model.ts index 32c2131..7412ae2 100644 --- a/src/server/services/model.ts +++ b/src/server/services/model.ts @@ -84,6 +84,14 @@ function isTransientModelFailure(error: unknown) { } export class ModelService { + // Model configs don't change during a single review invocation, but resolveModel() is called + // once per file *and* once per fallback model. Left uncached that's a Hyperdrive round-trip + // (a counted subrequest) for every one of those, which both burns the per-invocation + // subrequest budget (shrinking how many files a chunk can review in parallel) and floods the + // connection pool. Memoize per ModelService instance (one instance == one invocation/chunk) + // so each distinct model is resolved from the DB at most once. + private readonly resolvedModelCache = new Map(); + constructor( private env: AppBindings, private tracker?: TokenTracker, @@ -161,7 +169,13 @@ export class ModelService { private async resolveModel(model: string) { const normalized = normalizeModel(model); - const resolved = await getResolvedModelConfig(this.env, normalized); + let resolved = this.resolvedModelCache.get(normalized); + if (resolved === undefined) { + // Cache the DB answer -- including a null "not configured" result -- so a missing or + // repeatedly-used model isn't re-queried for every file in the chunk. + resolved = await getResolvedModelConfig(this.env, normalized); + this.resolvedModelCache.set(normalized, resolved); + } if (!resolved) { throw new Error(`Model ${normalized} is not configured. Add it in Settings before using it in a route.`); } diff --git a/test-diff.ts b/test-diff.ts deleted file mode 100644 index bba1742..0000000 --- a/test-diff.ts +++ /dev/null @@ -1,19 +0,0 @@ -import { parseUnifiedDiff } from './src/server/core/diff.ts'; - -const files = [ - { path: 'src/one.ts', content: 'console.log(1);' }, - { path: 'src/two.ts', content: 'console.log(2);' }, - { path: 'src/three.ts', content: 'console.log(3);' }, -]; - -const mockDiff = files.map((f) => { - const lines = f.content.split('\n'); - return `diff --git a/${f.path} b/${f.path} -index 1234567..890abcd 100644 ---- a/${f.path} -+++ b/${f.path} -@@ -1,${lines.length} +1,${lines.length} @@ -${lines.map((l) => `+${l}`).join('\n')}`; - }).join('\n'); - -console.log(JSON.stringify(parseUnifiedDiff(mockDiff), null, 2)); diff --git a/test/api.spec.ts b/test/api.spec.ts index 144689a..b1011c9 100644 --- a/test/api.spec.ts +++ b/test/api.spec.ts @@ -155,6 +155,61 @@ describe('Dashboard API Suite', () => { expect(response.status).toBe(404); }); + it('preserves omitted review settings when patching a single setting', async () => { + const env = createTestEnv(); + const token = await getAuthCookie(env); + const headers = { + Cookie: `codra_session=${token}`, + 'x-requested-with': 'XMLHttpRequest', + 'content-type': 'application/json', + }; + + try { + const seed = await app.request('/api/settings', { + method: 'PATCH', + headers, + body: JSON.stringify({ concurrencyLevel: 'low', maxComments: 5 }), + }, env); + expect(seed.status).toBe(200); + + const commentsOnly = await app.request('/api/settings', { + method: 'PATCH', + headers, + body: JSON.stringify({ maxComments: 20 }), + }, env); + expect(commentsOnly.status).toBe(200); + + const afterCommentsOnly = await app.request('/api/settings', { + headers: { Cookie: `codra_session=${token}` }, + }, env); + expect(afterCommentsOnly.status).toBe(200); + await expect(afterCommentsOnly.json()).resolves.toMatchObject({ + settings: { concurrencyLevel: 'low', maxComments: 20 }, + }); + + const concurrencyOnly = await app.request('/api/settings', { + method: 'PATCH', + headers, + body: JSON.stringify({ concurrencyLevel: 'high' }), + }, env); + expect(concurrencyOnly.status).toBe(200); + + const afterConcurrencyOnly = await app.request('/api/settings', { + headers: { Cookie: `codra_session=${token}` }, + }, env); + expect(afterConcurrencyOnly.status).toBe(200); + await expect(afterConcurrencyOnly.json()).resolves.toMatchObject({ + settings: { concurrencyLevel: 'high', maxComments: 20 }, + }); + } finally { + await app.request('/api/settings', { + method: 'PATCH', + headers, + body: JSON.stringify({ concurrencyLevel: 'medium', maxComments: 10 }), + }, env); + } + }); + it('rejects logout without the CSRF header', async () => { const env = createTestEnv(); const token = await getAuthCookie(env); diff --git a/test/chunk-concurrency.spec.ts b/test/chunk-concurrency.spec.ts new file mode 100644 index 0000000..47ad013 --- /dev/null +++ b/test/chunk-concurrency.spec.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from 'vitest'; +import { budgetAwareFileLimit } from '@server/core/review'; +import { TokenTracker } from '@server/core/token-tracker'; +import { REVIEW_CONCURRENCY_LIMITS, reviewConcurrencyLevels } from '@shared/schema'; + +// Regression guard for the "concurrency slider is dead above medium" incident: the per-chunk +// budget cap must NOT silently override the user's configured concurrency at a healthy budget. +// These assertions exercise the REAL TokenTracker (so MAX_SUBREQUESTS / SAFE_MARGIN are in play) +// and the REAL REVIEW_CONCURRENCY_LIMITS, so bumping SAFE_MARGIN or ESTIMATED_SUBREQUESTS_PER_FILE +// back into a slider-defeating range fails this test. + +const maxLevel = Math.max(...reviewConcurrencyLevels.map((level) => REVIEW_CONCURRENCY_LIMITS[level])); + +describe('budgetAwareFileLimit', () => { + it('honors every configured concurrency level at a fresh budget', () => { + const fresh = new TokenTracker().remainingSafeBudget(); + for (const level of reviewConcurrencyLevels) { + const configured = REVIEW_CONCURRENCY_LIMITS[level]; + expect(budgetAwareFileLimit(fresh, configured)).toBe(configured); + } + }); + + it('still honors the highest level after the getPullRequest preamble spends a few subrequests', () => { + const tracker = new TokenTracker(); + tracker.incrementSubrequests(3); // token read + getPullRequest + a little slack + expect(budgetAwareFileLimit(tracker.remainingSafeBudget(), maxLevel)).toBe(maxLevel); + }); + + it('throttles below the configured level only once the budget has actually been eaten into', () => { + // Deep into a troubled invocation the cap should shrink to protect the 50-subrequest ceiling. + expect(budgetAwareFileLimit(4, maxLevel)).toBe(1); + expect(budgetAwareFileLimit(0, maxLevel)).toBe(1); // never returns 0 -- always makes progress + expect(budgetAwareFileLimit(10, maxLevel)).toBeLessThan(maxLevel); + }); + + it('never exceeds the configured level even with a huge budget', () => { + expect(budgetAwareFileLimit(10_000, 2)).toBe(2); + }); +}); diff --git a/test/model-config-cache.spec.ts b/test/model-config-cache.spec.ts new file mode 100644 index 0000000..c0dae19 --- /dev/null +++ b/test/model-config-cache.spec.ts @@ -0,0 +1,75 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { createTestEnv } from './helpers'; + +// Isolated in its own file: mocking @server/db/model-configs module-wide would break the +// other model-service tests that resolve configs against the real test DB. +const getResolvedModelConfigMock = vi.hoisted(() => vi.fn()); + +vi.mock('@server/db/model-configs', async (importOriginal) => { + const mod = await importOriginal(); + return { ...mod, getResolvedModelConfig: getResolvedModelConfigMock }; +}); + +import { ModelService } from '@server/services/model'; + +const cloudflareConfig = (modelId: string) => ({ + modelId, + providerId: 'cf', + providerName: 'Cloudflare', + apiFormat: 'cloudflare-workers-ai' as const, + modelName: modelId, + updatedAt: new Date().toISOString(), + providerEnabled: true, + baseUrl: null, + encryptedApiKey: null, +}); + +describe('ModelService model-config caching', () => { + beforeEach(() => { + getResolvedModelConfigMock.mockReset(); + }); + + it('resolves a given model config from the DB at most once per invocation', async () => { + getResolvedModelConfigMock.mockImplementation(async (_env: any, modelId: string) => cloudflareConfig(modelId)); + const service = new ModelService(createTestEnv()); + + // The same model is resolved repeatedly across a chunk (once per file); only the first + // should hit the DB. + await (service as any).resolveModel('@cf/zai-org/glm-4.7-flash'); + await (service as any).resolveModel('@cf/zai-org/glm-4.7-flash'); + await (service as any).resolveModel('@cf/zai-org/glm-4.7-flash'); + + expect(getResolvedModelConfigMock).toHaveBeenCalledTimes(1); + }); + + it('keeps a separate cache entry per distinct model id', async () => { + getResolvedModelConfigMock.mockImplementation(async (_env: any, modelId: string) => cloudflareConfig(modelId)); + const service = new ModelService(createTestEnv()); + + await (service as any).resolveModel('gemma-4-31b-it'); + await (service as any).resolveModel('gemma-4-26b-a4b-it'); + await (service as any).resolveModel('gemma-4-31b-it'); + + expect(getResolvedModelConfigMock).toHaveBeenCalledTimes(2); + }); + + it('caches a null "not configured" result so it is not re-queried every file', async () => { + getResolvedModelConfigMock.mockResolvedValue(null); + const service = new ModelService(createTestEnv()); + + await expect((service as any).resolveModel('does-not-exist')).rejects.toThrow('is not configured'); + await expect((service as any).resolveModel('does-not-exist')).rejects.toThrow('is not configured'); + + expect(getResolvedModelConfigMock).toHaveBeenCalledTimes(1); + }); + + it('does not share a cache across ModelService instances (one instance == one invocation)', async () => { + getResolvedModelConfigMock.mockImplementation(async (_env: any, modelId: string) => cloudflareConfig(modelId)); + const env = createTestEnv(); + + await (new ModelService(env) as any).resolveModel('@cf/zai-org/glm-4.7-flash'); + await (new ModelService(env) as any).resolveModel('@cf/zai-org/glm-4.7-flash'); + + expect(getResolvedModelConfigMock).toHaveBeenCalledTimes(2); + }); +}); diff --git a/test/model-service.spec.ts b/test/model-service.spec.ts index 21fcda0..1cbe3a8 100644 --- a/test/model-service.spec.ts +++ b/test/model-service.spec.ts @@ -208,6 +208,37 @@ describe('ModelService', () => { expect(attempts).toBe(1); }); + it('aborts and fails fast (as a retryable timeout) when a Cloudflare model hangs past the timeout', async () => { + vi.useFakeTimers(); + try { + let capturedSignal: AbortSignal | undefined; + const env = createTestEnv({ + AI: { + run(_model: string, _request: any, options?: { signal?: AbortSignal }) { + capturedSignal = options?.signal; + // Model never responds -- only the timeout can end this call. + return new Promise(() => {}); + }, + } as any, + }); + + const promise = reviewWithCloudflare(env, '@cf/zai-org/glm-4.7-flash', { + systemPrompt: 'system', + userPrompt: 'user', + }); + // Prevent an unhandled-rejection warning while the timer is still pending. + promise.catch(() => {}); + + await vi.advanceTimersByTimeAsync(60_000); + + await expect(promise).rejects.toThrow('timed out after 60000ms'); + // The underlying Workers-AI request was actually cancelled, not just abandoned. + expect(capturedSignal?.aborted).toBe(true); + } finally { + vi.useRealTimers(); + } + }); + it('tries the smaller Google fallback after the primary Google model fails', async () => { let cloudflareCalls = 0; const fetchMock = vi.spyOn(globalThis, 'fetch') From 5e48346dc1c172797895ccff31f2959812e6ed06 Mon Sep 17 00:00:00 2001 From: Devarshi Shimpi Date: Tue, 7 Jul 2026 23:46:23 +0530 Subject: [PATCH 09/12] fix: implement job continuation tracking and enhance concurrency handling in review process --- db/migrations/006_job_continuation_count.sql | 5 + src/client/lib/api.ts | 2 +- src/server/core/model-output.ts | 2 +- src/server/core/review.ts | 102 +++++++++++--- src/server/core/token-tracker.ts | 2 +- src/server/db/app-settings.ts | 19 ++- src/server/db/jobs.ts | 27 +++- src/server/models/google.ts | 139 ++++++++++++------- src/server/routes/api/settings.ts | 3 +- test/api.spec.ts | 68 ++++++++- test/chunk-concurrency.spec.ts | 4 +- test/model-service.spec.ts | 70 +++++++++- test/review-subrequest-completion.spec.ts | 28 ++++ test/token-tracker.spec.ts | 10 +- 14 files changed, 382 insertions(+), 99 deletions(-) create mode 100644 db/migrations/006_job_continuation_count.sql diff --git a/db/migrations/006_job_continuation_count.sql b/db/migrations/006_job_continuation_count.sql new file mode 100644 index 0000000..2e2cc8b --- /dev/null +++ b/db/migrations/006_job_continuation_count.sql @@ -0,0 +1,5 @@ +-- Tracks how many times a job has rescheduled the *same* phase without completing any +-- file review (a "no-progress continuation"). Reset to 0 whenever a chunk makes progress. +-- A hard ceiling on this counter (see MAX_JOB_CONTINUATIONS in review.ts) stops a job that +-- can never make headway from churning indefinitely on transient/budget deferrals. +ALTER TABLE jobs ADD COLUMN IF NOT EXISTS continuation_count INT NOT NULL DEFAULT 0; diff --git a/src/client/lib/api.ts b/src/client/lib/api.ts index f7d9a15..dfd7ef2 100644 --- a/src/client/lib/api.ts +++ b/src/client/lib/api.ts @@ -231,7 +231,7 @@ export const api = { return request<{ settings: ReviewSettings }>('/api/settings'); }, updateReviewSettings(settings: ReviewSettings) { - return request<{ ok: boolean }>('/api/settings', { + return request<{ ok: boolean; settings: ReviewSettings }>('/api/settings', { method: 'PATCH', body: JSON.stringify(settings), }); diff --git a/src/server/core/model-output.ts b/src/server/core/model-output.ts index d2298d5..3ad16f8 100644 --- a/src/server/core/model-output.ts +++ b/src/server/core/model-output.ts @@ -398,7 +398,7 @@ export function parseFileReviewResponse(raw: string, file: FileDiff): { while (current !== prev) { prev = current; current = current - .replace(/^([\u{1F300}-\u{1F9FF}]|\[QUALITY\]|\[SECURITY\]|\[BUG\]|\[PERFORMANCE\]|\[CORRECTNESS\]|\[P[0-3]\]|\[NIT\]|QUALITY|SECURITY|BUG|P[0-3]|NIT|[:\-\s\uFE0F]|[^\w\s])+/giu, '') + .replace(/^(?:[^\w\s]+|(?:QUALITY|SECURITY|BUG|PERFORMANCE|CORRECTNESS|P[0-3]|NIT)\b)+/giu, '') .replace(/\n\s*/g, ' ') // Flatten newlines in titles/snippets .trim(); } diff --git a/src/server/core/review.ts b/src/server/core/review.ts index d75e0e6..447c996 100644 --- a/src/server/core/review.ts +++ b/src/server/core/review.ts @@ -17,6 +17,7 @@ import { mapJob, markJobCheckRunCompleted, markJobContinuationQueued, + resetJobContinuationCount, releaseJobLease, supersedeOlderJobs, updateJobCheckRun, @@ -47,6 +48,12 @@ const JOB_LEASE_SECONDS = 15 * 60; const BUSY_RETRY_SECONDS = 60; const RETRYABLE_MODEL_FAILURE_RETRY_DELAYS_SECONDS = [60, 5 * 60, 15 * 60]; const MAX_RETRYABLE_FILE_REVIEW_FAILURES = 3; +// Belt-and-suspenders ceiling on how many times a job may reschedule the *same* phase without +// completing a single file (see markJobContinuationQueued / resetJobContinuationCount). The +// per-file cap above is the primary bound and resets this counter on any progress, so a healthy +// job never approaches this; it only fires when a job is genuinely wedged (e.g. a provider is +// down for the whole backoff window) and stops it from churning for hours before finalizing. +const MAX_JOB_CONTINUATIONS = 20; // A job's commit (and therefore its diff) never changes, so the raw diff can be // cached for the job's entire lifetime instead of being re-fetched from GitHub on // every prepare/review-chunk/finalize phase. 6h comfortably covers even a job that @@ -59,23 +66,22 @@ const DIFF_CACHE_TTL_SECONDS = 6 * 60 * 60; // (ModelService.resolveModel), so the recurring cost per file is ~1 provider call per model // tried plus the persisted-review write -- roughly 5 in the worst case rather than 9. Lower // estimate => more files reviewed in parallel per chunk within the same 50-subrequest cap. -const ESTIMATED_SUBREQUESTS_PER_FILE = 5; +const ESTIMATED_SUBREQUESTS_PER_FILE = 8; /** * How many files a single review chunk may process concurrently: the configured concurrency * level, capped only by what the invocation's remaining subrequest budget can safely cover. * * The cap is deliberately sized so it does NOT silently override the user's chosen concurrency - * at a healthy budget -- that would make the concurrency setting a no-op above the cap. With - * MAX_SUBREQUESTS 50, SAFE_MARGIN 22 and ESTIMATED_SUBREQUESTS_PER_FILE 5, a fresh invocation - * yields floor(28/5) = 5, comfortably above the highest configured level (max = 4) even after - * the getPullRequest preamble spends a couple of subrequests. It only throttles (down to 1) - * once earlier failures in this invocation have actually eaten into the budget; any files a - * throttled chunk can't reach simply roll into the next chunk. The + * at a healthy budget -- that would make the concurrency setting a no-op above the cap. It + * only throttles once earlier failures in this invocation have actually eaten into the budget; + * if there is not enough safe budget for one more file, the chunk yields and resumes in a fresh + * invocation instead of gambling past the margin. Any files a throttled chunk can't reach roll + * into the next chunk. The * chunk-file-limit-honors-configured-level invariant is pinned by a regression test. */ export function budgetAwareFileLimit(remainingSafeBudget: number, configuredChunkFileLimit: number) { - const budgetLimit = Math.max(1, Math.floor(remainingSafeBudget / ESTIMATED_SUBREQUESTS_PER_FILE)); + const budgetLimit = Math.floor(remainingSafeBudget / ESTIMATED_SUBREQUESTS_PER_FILE); return Math.min(configuredChunkFileLimit, budgetLimit); } @@ -96,9 +102,8 @@ function isRetryableFileReviewErrorMessage(message: string | null | undefined) { lower.includes('[redacted]') || lower.includes('returned no review content') || lower.includes('empty response') || - // A file that was deferred purely because this invocation ran out of subrequest budget - // must be retried on a later chunk, not treated as permanently handled. (The MAX-attempts - // backstop below still marks it failed if the pressure never clears.) + // Older jobs may have persisted subrequest-budget failures before budget exhaustion became + // a pure chunk-level deferral. Keep retrying those rows instead of treating them as handled. lower.includes('subrequest') ); } @@ -323,9 +328,7 @@ export async function runReviewJob(env: AppBindings, message: ReviewJobMessage): phase, delaySeconds, }); - await markJobContinuationQueued(env, job.id, delaySeconds); - await releaseJobLease(env, job.id, leaseOwner); - return { action: 'next_phase', phase, delaySeconds }; // Retry same phase + return continueOrFailWedgedJob(env, job, github, leaseOwner, phase, delaySeconds, 'transient model/provider failures'); } // Running out of this invocation's subrequest budget is not a job failure: the prepare and @@ -340,9 +343,7 @@ export async function runReviewJob(env: AppBindings, message: ReviewJobMessage): phase, delaySeconds, }); - await markJobContinuationQueued(env, job.id, delaySeconds); - await releaseJobLease(env, job.id, leaseOwner); - return { action: 'next_phase', phase, delaySeconds }; // Resume same phase, fresh budget + return continueOrFailWedgedJob(env, job, github, leaseOwner, phase, delaySeconds, 'per-invocation subrequest limits'); } logger.error(`Review job failed: ${job.owner}/${job.repo} PR #${job.prNumber}`, error); @@ -352,6 +353,38 @@ export async function runReviewJob(env: AppBindings, message: ReviewJobMessage): } } +// Records a same-phase continuation and enforces the MAX_JOB_CONTINUATIONS ceiling. As long as +// the job keeps completing files, resetJobContinuationCount() keeps this counter near zero; once +// it has rescheduled MAX_JOB_CONTINUATIONS times without a single file completing, the job is +// genuinely wedged (e.g. a provider is down for the entire backoff window), so we fail it +// terminally instead of letting it churn indefinitely. +async function continueOrFailWedgedJob( + env: AppBindings, + job: PersistedReviewJob, + github: GitHubService, + leaseOwner: string, + phase: 'prepare' | 'review' | 'finalize', + delaySeconds: number, + reason: string, +): Promise { + const continuationCount = await markJobContinuationQueued(env, job.id, delaySeconds); + + if (continuationCount > MAX_JOB_CONTINUATIONS) { + const message = `Review could not make progress after ${continuationCount} continuation attempts (${reason}). Failing the job to avoid an endless retry loop; re-run it once the underlying provider issue clears.`; + logger.error(`Review job exceeded the continuation ceiling; failing terminally: ${job.owner}/${job.repo} PR #${job.prNumber}`, { + phase, + continuationCount, + reason, + }); + await failJobAndCheckRun(env, job, github, message); + await releaseJobLease(env, job.id, leaseOwner); + return { action: 'ack' }; + } + + await releaseJobLease(env, job.id, leaseOwner); + return { action: 'next_phase', phase, delaySeconds }; // Resume same phase +} + async function resolveQueuedJob( env: AppBindings, message: ReviewJobMessage, @@ -562,6 +595,9 @@ async function runReviewPhase( // plan: 50) -- but sized (see budgetAwareFileLimit) so the configured concurrency level is // honored in full at a healthy budget and only throttled once the budget is actually spent. const reviewChunkFileLimit = budgetAwareFileLimit(tracker.remainingSafeBudget(), configuredChunkFileLimit); + if (reviewChunkFileLimit <= 0) { + throw new Error('Subrequest budget for this invocation was exhausted before starting the next review chunk.'); + } const startedAt = Date.now(); let processedThisChunk = 0; @@ -623,6 +659,14 @@ async function runReviewPhase( const results = await Promise.allSettled(reviewTasks); await heartbeatAndCheckSuperseded(env, job.id, leaseOwner); + // A fulfilled task means a file reached a terminal state this chunk (reviewed, inherited, or + // marked permanently failed) -- i.e. the job made progress. Clear the no-progress continuation + // counter so a slow-but-advancing job never trips the MAX_JOB_CONTINUATIONS safety net; only a + // chunk that completes *nothing* is allowed to push the job toward that ceiling. + if (results.some((result) => result.status === 'fulfilled')) { + await resetJobContinuationCount(env, job.id); + } + const rejected = results.filter((result): result is PromiseRejectedResult => result.status === 'rejected'); if (rejected.length > 0) { rejected.forEach((result, index) => { @@ -714,11 +758,23 @@ async function reviewAndPersistFile( const modelId = config.model?.main ?? 'unconfigured'; const modelProvider = await resolveFailureModelProvider(); - // A transient model/provider outage OR this invocation running out of subrequest budget: - // both should defer the file to a later chunk (fresh budget) rather than abandon it. The - // shared per-file attempt counter still bounds this, so a file that can never be reviewed - // is eventually marked failed and the job finalizes as a partial review instead of looping. - if (isRetryableModelError(error) || isSubrequestBudgetError(error)) { + // Per-invocation subrequest pressure clears on the next Worker invocation, so do not count + // it as a per-file provider outage. Let the job-level no-progress continuation ceiling bound + // a genuinely wedged job while this file remains pending for the fresh-budget retry. + if (isSubrequestBudgetError(error)) { + logger.warn(`File review deferred for ${file.path}; subrequest budget will retry in a fresh invocation`, { + error: errorMessage, + }); + Object.defineProperty(error, 'retryAfterSeconds', { + value: RETRYABLE_MODEL_FAILURE_RETRY_DELAYS_SECONDS[0], + configurable: true, + }); + throw error; + } + + // Transient model/provider outages count against the file so a single unrecoverable file + // eventually becomes a partial-review failure instead of blocking the entire job forever. + if (isRetryableModelError(error)) { const failureCount = await recordRetryableFileReviewFailure(env, job.id, { filePath: file.path, modelUsed: modelId, @@ -754,7 +810,7 @@ async function reviewAndPersistFile( return; } - logger.warn(`File review deferred for ${file.path}; transient model/provider failure or subrequest-budget limit will retry later`, { + logger.warn(`File review deferred for ${file.path}; transient model/provider failure will retry later`, { error: errorMessage, attempts: failureCount, }); diff --git a/src/server/core/token-tracker.ts b/src/server/core/token-tracker.ts index 422c3e6..88d1e0c 100644 --- a/src/server/core/token-tracker.ts +++ b/src/server/core/token-tracker.ts @@ -14,7 +14,7 @@ export class TokenTracker { private usage: Map = new Map(); private subrequests = 0; private readonly MAX_SUBREQUESTS = 50; - private readonly SAFE_MARGIN = 22; // Increased even further to handle finalization overhead + private readonly SAFE_MARGIN = 10; // Lowered since finalize runs in a separate invocation incrementSubrequests(count = 1) { this.subrequests += count; diff --git a/src/server/db/app-settings.ts b/src/server/db/app-settings.ts index 6dfe908..5c496e6 100644 --- a/src/server/db/app-settings.ts +++ b/src/server/db/app-settings.ts @@ -1,12 +1,14 @@ import type { AppBindings } from '@server/env'; import { queryRows } from './client'; import { logger } from '@server/core/logger'; -import { reviewSettingsSchema, type ReviewSettings } from '@shared/schema'; +import { reviewConcurrencyLevels, reviewMaxCommentsOptions, reviewSettingsSchema, type ReviewSettings } from '@shared/schema'; const CONCURRENCY_KEY = 'review_concurrency_level'; const MAX_COMMENTS_KEY = 'review_max_comments'; const DEFAULT_REVIEW_SETTINGS: ReviewSettings = reviewSettingsSchema.parse({}); +const CONCURRENCY_LEVELS = new Set(reviewConcurrencyLevels); +const MAX_COMMENTS_OPTIONS = new Set(reviewMaxCommentsOptions); export async function getReviewSettings(env: Pick): Promise { try { @@ -16,11 +18,18 @@ export async function getReviewSettings(env: Pick): P [[CONCURRENCY_KEY, MAX_COMMENTS_KEY]], ); const map = new Map(rows.map((row) => [row.key, row.value])); - const parsed = reviewSettingsSchema.safeParse({ - concurrencyLevel: map.get(CONCURRENCY_KEY), - maxComments: map.has(MAX_COMMENTS_KEY) ? Number(map.get(MAX_COMMENTS_KEY)) : undefined, + const storedConcurrency = map.get(CONCURRENCY_KEY); + const storedMaxComments = map.get(MAX_COMMENTS_KEY); + const parsedMaxComments = storedMaxComments === undefined ? NaN : Number(storedMaxComments); + + return reviewSettingsSchema.parse({ + concurrencyLevel: storedConcurrency && CONCURRENCY_LEVELS.has(storedConcurrency) + ? storedConcurrency + : DEFAULT_REVIEW_SETTINGS.concurrencyLevel, + maxComments: MAX_COMMENTS_OPTIONS.has(parsedMaxComments) + ? parsedMaxComments + : DEFAULT_REVIEW_SETTINGS.maxComments, }); - return parsed.success ? parsed.data : DEFAULT_REVIEW_SETTINGS; } catch (error) { logger.warn('Failed to load review settings, using defaults', { error: error instanceof Error ? error.message : String(error), diff --git a/src/server/db/jobs.ts b/src/server/db/jobs.ts index 9f263b4..b0ba5de 100644 --- a/src/server/db/jobs.ts +++ b/src/server/db/jobs.ts @@ -26,6 +26,7 @@ export type JobRow = { lease_expires_at: string | null; heartbeat_at: string | null; recovery_count: number | null; + continuation_count: number | null; last_queue_message_at: string | null; total_input_tokens: number | null; total_output_tokens: number | null; @@ -518,21 +519,45 @@ export async function releaseJobLease(env: Pick, jobI ); } +// Records that a job is rescheduling the same phase (a continuation) and returns the resulting +// no-progress continuation count. The counter is bumped here and cleared by +// resetJobContinuationCount() whenever a chunk actually completes a file, so a healthy job that +// keeps making headway stays near zero while a job that can never progress climbs toward the +// MAX_JOB_CONTINUATIONS ceiling and is failed terminally. export async function markJobContinuationQueued(env: Pick, jobId: string, delaySeconds = 0) { - await queryRows( + const rows = await queryRows<{ continuation_count: number }>( env, ` UPDATE jobs SET heartbeat_at = now(), + continuation_count = continuation_count + 1, last_queue_message_at = CASE WHEN $2::int > 0 THEN now() + ($2::text || ' seconds')::interval ELSE now() END WHERE id = $1 AND status = 'running' + RETURNING continuation_count `, [jobId, delaySeconds], ); + return rows[0]?.continuation_count ?? 0; +} + +// Clears the no-progress continuation counter after a chunk completes at least one file review, +// so slow-but-progressing jobs never trip the MAX_JOB_CONTINUATIONS safety net. +export async function resetJobContinuationCount(env: Pick, jobId: string) { + await queryRows( + env, + ` + UPDATE jobs + SET continuation_count = 0 + WHERE id = $1 + AND status = 'running' + AND continuation_count <> 0 + `, + [jobId], + ); } export async function updateJobCheckRun(env: Pick, jobId: string, checkRunId: number) { diff --git a/src/server/models/google.ts b/src/server/models/google.ts index 085d182..ff9b2ae 100644 --- a/src/server/models/google.ts +++ b/src/server/models/google.ts @@ -9,7 +9,33 @@ const GEMINI_MAX_OUTPUT_TOKENS = 4096; const DEFAULT_GEMINI_BASE_URL = 'https://generativelanguage.googleapis.com/v1beta'; function isRetryableGeminiStatus(status: number) { - return status === 408 || status === 500 || status === 502 || status === 503 || status === 504 || status === 524; + return status === 408 || status === 429 || status === 500 || status === 502 || status === 503 || status === 504 || status === 524; +} + +function defaultRetryDelayMs(attempt: number) { + return Math.pow(2, attempt + 1) * 1000 + Math.random() * 1000; +} + +function retryAfterDelayMs(value: string | null) { + if (!value) return null; + const seconds = Number(value); + if (Number.isFinite(seconds) && seconds >= 0) { + return seconds * 1000; + } + + const dateMs = Date.parse(value); + if (Number.isFinite(dateMs)) { + return Math.max(0, dateMs - Date.now()); + } + + return null; +} + +function isRetryableTransportError(error: unknown) { + if (!(error instanceof Error)) return false; + if (error.name === 'TimeoutError' || error.message.toLowerCase().includes('timeout')) return true; + if (error.message.includes('fetch failed')) return true; + return error instanceof TypeError; } function isPrivateIP(hostname: string) { @@ -55,17 +81,19 @@ export async function reviewWithGoogle( const url = `${baseUrl}/models/${encodeURIComponent(model)}:generateContent?key=${encodeURIComponent(config.apiKey)}`; const maxRetries = GEMINI_MAX_RETRIES; let lastError: unknown; + let delayBeforeAttemptMs = 0; for (let attempt = 0; attempt <= maxRetries; attempt++) { + if (delayBeforeAttemptMs > 0) { + logger.info(`Retrying Gemini request (attempt ${attempt}/${maxRetries}) in ${Math.round(delayBeforeAttemptMs)}ms`); + await new Promise(resolve => setTimeout(resolve, delayBeforeAttemptMs)); + delayBeforeAttemptMs = 0; + } + + let response: Response; try { if (tracker) tracker.incrementSubrequests(1); - if (attempt > 0) { - const delay = Math.pow(2, attempt) * 1000 + Math.random() * 1000; - logger.info(`Retrying Gemini request (attempt ${attempt}/${maxRetries}) in ${Math.round(delay)}ms`); - await new Promise(resolve => setTimeout(resolve, delay)); - } - - const response = await withTimeout('Gemini API', GEMINI_TIMEOUT_MS, (signal) => + response = await withTimeout('Gemini API', GEMINI_TIMEOUT_MS, (signal) => fetch(url, { method: 'POST', signal, @@ -90,58 +118,63 @@ export async function reviewWithGoogle( }), }), ); - - if (!response.ok) { - const errorText = await response.text(); - const message = providerErrorMessage(errorText); - const isRetryable = isRetryableGeminiStatus(response.status); - - const logData = { - error: message, - attempt, - willRetry: isRetryable && attempt < maxRetries, - }; - if (isRetryable && attempt < maxRetries) { - logger.warn(`Gemini request failed with ${response.status}; retrying`, logData); - lastError = new ProviderRequestError(config.providerName ?? 'Google', response.status, message); - continue; - } - - logger.error(`Gemini request failed with ${response.status}`, logData); - throw new ProviderRequestError(config.providerName ?? 'Google', response.status, message); + } catch (error) { + lastError = error; + if (isRetryableTransportError(error) && attempt < maxRetries) { + delayBeforeAttemptMs = defaultRetryDelayMs(attempt); + continue; } + throw error; + } - const durationMs = Date.now() - startTime; - logger.info(`AI model ${model} responded in ${durationMs}ms`); - - const data = (await response.json()) as { - candidates?: Array<{ content?: { parts?: Array<{ text?: string }> } }>; - usageMetadata?: { - promptTokenCount?: number; - candidatesTokenCount?: number; - }; + if (!response.ok) { + const errorText = await response.text(); + const message = providerErrorMessage(errorText); + const isRetryable = isRetryableGeminiStatus(response.status); + const retryDelayMs = response.status === 429 + ? retryAfterDelayMs(response.headers.get('retry-after')) ?? defaultRetryDelayMs(attempt) + : defaultRetryDelayMs(attempt); + + const logData = { + error: message, + attempt, + willRetry: isRetryable && attempt < maxRetries, + retryDelayMs: isRetryable && attempt < maxRetries ? retryDelayMs : undefined, }; - - const rawText = data.candidates?.[0]?.content?.parts?.map((part) => part.text ?? '').join('')?.trim(); - if (!rawText) { - throw new Error('Gemini returned an empty response.'); + if (isRetryable && attempt < maxRetries) { + logger.warn(`Gemini request failed with ${response.status}; retrying`, logData); + lastError = new ProviderRequestError(config.providerName ?? 'Google', response.status, message); + delayBeforeAttemptMs = retryDelayMs; + continue; } - return { - rawText, - inputTokens: data.usageMetadata?.promptTokenCount ?? 0, - outputTokens: data.usageMetadata?.candidatesTokenCount ?? 0, - modelUsed: model, - provider: config.providerName ?? 'Google', + logger.error(`Gemini request failed with ${response.status}`, logData); + throw new ProviderRequestError(config.providerName ?? 'Google', response.status, message); + } + + const durationMs = Date.now() - startTime; + logger.info(`AI model ${model} responded in ${durationMs}ms`); + + const data = (await response.json()) as { + candidates?: Array<{ content?: { parts?: Array<{ text?: string }> } }>; + usageMetadata?: { + promptTokenCount?: number; + candidatesTokenCount?: number; }; - } catch (error) { - lastError = error; - const isTimeout = error instanceof Error && (error.name === 'TimeoutError' || error.message.includes('timeout')); - if (isTimeout && attempt < maxRetries) { - continue; - } - throw error; + }; + + const rawText = data.candidates?.[0]?.content?.parts?.map((part) => part.text ?? '').join('')?.trim(); + if (!rawText) { + throw new Error('Gemini returned an empty response.'); } + + return { + rawText, + inputTokens: data.usageMetadata?.promptTokenCount ?? 0, + outputTokens: data.usageMetadata?.candidatesTokenCount ?? 0, + modelUsed: model, + provider: config.providerName ?? 'Google', + }; } throw lastError; diff --git a/src/server/routes/api/settings.ts b/src/server/routes/api/settings.ts index fd14cef..774ffdc 100644 --- a/src/server/routes/api/settings.ts +++ b/src/server/routes/api/settings.ts @@ -25,7 +25,8 @@ export function createSettingsRouter() { }); app.patch('/', async (c) => { - const parsed = reviewSettingsPatchSchema.safeParse(await c.req.json()); + const body = await c.req.json().catch(() => null); + const parsed = reviewSettingsPatchSchema.safeParse(body); if (!parsed.success) { return jsonError('Invalid review settings.', 400); } diff --git a/test/api.spec.ts b/test/api.spec.ts index b1011c9..c9e70f1 100644 --- a/test/api.spec.ts +++ b/test/api.spec.ts @@ -1,6 +1,7 @@ import { createApp } from '@server/app'; import { getJobForProcessing, insertJob } from '@server/db/jobs'; import { insertFileReview } from '@server/db/file-reviews'; +import { queryRows } from '@server/db/client'; import { getRepoConfigRecord } from '@server/db/repo-configs'; import { loadRepoConfig, updateGlobalConfig } from '@server/core/config'; import { GitHubClient } from '@server/core/github'; @@ -210,6 +211,71 @@ describe('Dashboard API Suite', () => { } }); + it('returns 400 for malformed review settings JSON', async () => { + const env = createTestEnv(); + const token = await getAuthCookie(env); + + const response = await app.request('/api/settings', { + method: 'PATCH', + headers: { + Cookie: `codra_session=${token}`, + 'x-requested-with': 'XMLHttpRequest', + 'content-type': 'application/json', + }, + body: '{', + }, env); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toMatchObject({ error: 'Invalid review settings.' }); + }); + + it('falls back invalid stored review settings independently', async () => { + const env = createTestEnv(); + const token = await getAuthCookie(env); + + try { + await queryRows( + env, + `INSERT INTO global_settings (key, value) VALUES + ('review_concurrency_level', 'turbo'), + ('review_max_comments', '20') + ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value`, + ); + + const invalidConcurrency = await app.request('/api/settings', { + headers: { Cookie: `codra_session=${token}` }, + }, env); + expect(invalidConcurrency.status).toBe(200); + await expect(invalidConcurrency.json()).resolves.toMatchObject({ + settings: { concurrencyLevel: 'medium', maxComments: 20 }, + }); + + await queryRows( + env, + `INSERT INTO global_settings (key, value) VALUES + ('review_concurrency_level', 'high'), + ('review_max_comments', '999') + ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value`, + ); + + const invalidComments = await app.request('/api/settings', { + headers: { Cookie: `codra_session=${token}` }, + }, env); + expect(invalidComments.status).toBe(200); + await expect(invalidComments.json()).resolves.toMatchObject({ + settings: { concurrencyLevel: 'high', maxComments: 10 }, + }); + } finally { + await queryRows( + env, + `INSERT INTO global_settings (key, value) VALUES + ('review_concurrency_level', 'medium'), + ('review_max_comments', '10') + ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value`, + ); + } + }); + it('rejects logout without the CSRF header', async () => { const env = createTestEnv(); const token = await getAuthCookie(env); @@ -572,7 +638,7 @@ describe('Dashboard API Suite', () => { const env = createTestEnv(); const token = await getAuthCookie(env); await saveTestProviderApiKey(env); - vi.spyOn(globalThis, 'fetch').mockResolvedValue(Response.json({ + vi.spyOn(globalThis, 'fetch').mockImplementation(async () => Response.json({ error: { code: 429, message: 'Quota exceeded. Please retry later.', diff --git a/test/chunk-concurrency.spec.ts b/test/chunk-concurrency.spec.ts index 47ad013..87a12d2 100644 --- a/test/chunk-concurrency.spec.ts +++ b/test/chunk-concurrency.spec.ts @@ -28,8 +28,8 @@ describe('budgetAwareFileLimit', () => { it('throttles below the configured level only once the budget has actually been eaten into', () => { // Deep into a troubled invocation the cap should shrink to protect the 50-subrequest ceiling. - expect(budgetAwareFileLimit(4, maxLevel)).toBe(1); - expect(budgetAwareFileLimit(0, maxLevel)).toBe(1); // never returns 0 -- always makes progress + expect(budgetAwareFileLimit(4, maxLevel)).toBe(0); + expect(budgetAwareFileLimit(0, maxLevel)).toBe(0); expect(budgetAwareFileLimit(10, maxLevel)).toBeLessThan(maxLevel); }); diff --git a/test/model-service.spec.ts b/test/model-service.spec.ts index 1cbe3a8..2ced0a3 100644 --- a/test/model-service.spec.ts +++ b/test/model-service.spec.ts @@ -188,6 +188,66 @@ describe('ModelService', () => { expect(response.rawText).toContain('"findings"'); }); + it('honors Retry-After when retrying Google 429 responses', async () => { + vi.useFakeTimers(); + try { + const fetchMock = vi.spyOn(globalThis, 'fetch') + .mockResolvedValueOnce( + new Response( + JSON.stringify({ error: { code: 429, message: 'Rate limited.' } }), + { status: 429, headers: { 'content-type': 'application/json', 'retry-after': '7' } }, + ), + ) + .mockResolvedValueOnce( + new Response( + JSON.stringify({ + candidates: [{ content: { parts: [{ text: '{"findings":[],"overall_correctness":"patch is correct","overall_explanation":"ok","overall_confidence_score":0.9}' }] } }], + usageMetadata: { promptTokenCount: 1, candidatesTokenCount: 1 }, + }), + { status: 200, headers: { 'content-type': 'application/json' } }, + ), + ); + + const promise = reviewWithGoogle( + { apiKey: 'test-key' }, + 'gemma-4-31b-it', + { systemPrompt: 'system', userPrompt: 'user' }, + ); + promise.catch(() => {}); + + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(1)); + await vi.advanceTimersByTimeAsync(6_999); + expect(fetchMock).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(1); + const response = await promise; + + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(response.rawText).toContain('"findings"'); + } finally { + vi.useRealTimers(); + } + }); + + it('does not retry TypeErrors thrown after a successful Google response', async () => { + const fetchMock = vi.spyOn(globalThis, 'fetch').mockResolvedValue({ + ok: true, + json: async () => { + throw new TypeError('parser exploded after response'); + }, + } as unknown as Response); + + await expect( + reviewWithGoogle( + { apiKey: 'test-key' }, + 'gemma-4-31b-it', + { systemPrompt: 'system', userPrompt: 'user' }, + ), + ).rejects.toThrow('parser exploded after response'); + + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + it('does not spend an extra queue slice retrying the same Cloudflare model inline', async () => { let attempts = 0; const env = createTestEnv({ @@ -338,7 +398,7 @@ describe('ModelService', () => { const env = createTestEnv(); await saveTestProviderApiKey(env); const tracker = new TokenTracker(); - tracker.incrementSubrequests(30); // already past MAX_SUBREQUESTS (50) - SAFE_MARGIN (22) + tracker.incrementSubrequests(40); // already at MAX_SUBREQUESTS (50) - SAFE_MARGIN (10) const service = new ModelService(env, tracker); const response = await service.reviewFile({ @@ -370,9 +430,9 @@ describe('ModelService', () => { it('skips remaining fallback models (instead of spending more of the shared budget) once near the subrequest limit', async () => { // Google's client retries a 5xx once internally before giving up on a model, so the - // primary model alone can issue more than one raw fetch call; mockResolvedValue (not - // Once) keeps every call -- including that internal retry -- failing the same way. - const fetchMock = vi.spyOn(globalThis, 'fetch').mockResolvedValue( + // primary model alone can issue more than one raw fetch call; return a fresh Response for + // every call so retries do not reuse an already-consumed body. + const fetchMock = vi.spyOn(globalThis, 'fetch').mockImplementation(async () => new Response( JSON.stringify({ error: { code: 500, message: 'Internal error encountered.', status: 'INTERNAL' } }), { status: 500, headers: { 'content-type': 'application/json' } }, @@ -381,7 +441,7 @@ describe('ModelService', () => { const env = createTestEnv(); await saveTestProviderApiKey(env); const tracker = new TokenTracker(); - tracker.incrementSubrequests(30); // already past MAX_SUBREQUESTS (50) - SAFE_MARGIN (22) + tracker.incrementSubrequests(40); // already at MAX_SUBREQUESTS (50) - SAFE_MARGIN (10) const service = new ModelService(env, tracker); await expect( diff --git a/test/review-subrequest-completion.spec.ts b/test/review-subrequest-completion.spec.ts index 8c30d30..9f59507 100644 --- a/test/review-subrequest-completion.spec.ts +++ b/test/review-subrequest-completion.spec.ts @@ -122,4 +122,32 @@ describe('runReviewJob subrequest-budget handling', () => { expect(failJobMock).toHaveBeenCalledWith(expect.anything(), JOB_ID, 'totally unexpected boom'); expect(markJobContinuationQueuedMock).not.toHaveBeenCalled(); }); + + it('keeps rescheduling while the continuation count is still under the ceiling', async () => { + const env = createTestEnv(); + // markJobContinuationQueued returns the post-increment count; a value at/under the ceiling + // (MAX_JOB_CONTINUATIONS = 20) must still reschedule rather than give up. + markJobContinuationQueuedMock.mockResolvedValue(20); + getPullRequestMock.mockRejectedValue(new Error('Too many subrequests by single Worker invocation.')); + + const result = await runReviewJob(env, { jobId: JOB_ID, phase: 'review' } as any); + + expect(result).toEqual({ action: 'next_phase', phase: 'review', delaySeconds: expect.any(Number) }); + expect(failJobMock).not.toHaveBeenCalled(); + }); + + it('fails the job terminally once it exceeds the continuation ceiling without making progress', async () => { + const env = createTestEnv(); + // Post-increment count above MAX_JOB_CONTINUATIONS (20): the job is wedged, so it must stop + // churning and fail terminally instead of rescheduling yet again. + markJobContinuationQueuedMock.mockResolvedValue(21); + getPullRequestMock.mockRejectedValue(new Error('Too many subrequests by single Worker invocation.')); + + const result = await runReviewJob(env, { jobId: JOB_ID, phase: 'review' } as any); + + expect(result).toEqual({ action: 'ack' }); + expect(failJobMock).toHaveBeenCalledTimes(1); + expect(failJobMock.mock.calls[0][2]).toMatch(/could not make progress after 21 continuation attempts/); + expect(releaseJobLeaseMock).toHaveBeenCalled(); + }); }); diff --git a/test/token-tracker.spec.ts b/test/token-tracker.spec.ts index 720a0a1..f83435b 100644 --- a/test/token-tracker.spec.ts +++ b/test/token-tracker.spec.ts @@ -11,17 +11,17 @@ import { TokenTracker } from '@server/core/token-tracker'; describe('TokenTracker.remainingSafeBudget', () => { it('starts with the full margin below the hard cap available', () => { const tracker = new TokenTracker(); - // MAX_SUBREQUESTS (50) - SAFE_MARGIN (22) = 28, with nothing spent yet. - expect(tracker.remainingSafeBudget()).toBe(28); + // MAX_SUBREQUESTS (50) - SAFE_MARGIN (10) = 40, with nothing spent yet. + expect(tracker.remainingSafeBudget()).toBe(40); }); it('shrinks by exactly what has been spent so far', () => { const tracker = new TokenTracker(); tracker.incrementSubrequests(10); - expect(tracker.remainingSafeBudget()).toBe(18); + expect(tracker.remainingSafeBudget()).toBe(30); tracker.incrementSubrequests(5); - expect(tracker.remainingSafeBudget()).toBe(13); + expect(tracker.remainingSafeBudget()).toBe(25); }); it('never goes negative once spending exceeds the safe margin', () => { @@ -35,7 +35,7 @@ describe('TokenTracker.remainingSafeBudget', () => { it('agrees with isNearLimit at the same threshold', () => { const tracker = new TokenTracker(); - tracker.incrementSubrequests(27); + tracker.incrementSubrequests(39); expect(tracker.isNearLimit()).toBe(false); expect(tracker.remainingSafeBudget()).toBeGreaterThan(0); From cf8539addc3dce7f6cc0c5301fa4f20423dfc4ef Mon Sep 17 00:00:00 2001 From: Devarshi Shimpi Date: Wed, 8 Jul 2026 02:41:28 +0530 Subject: [PATCH 10/12] fix: enhance CI configuration, improve select component accessibility, and optimize stats queries --- .github/dependabot.yml | 11 ++++ .github/workflows/ci.yml | 7 ++- .github/workflows/codeql.yml | 33 ++++++++++ src/client/components/ui/select.tsx | 93 ++++++++++++++++++++++++++--- src/server/db/stats.ts | 25 +++++--- src/server/index.ts | 6 ++ src/server/models/anthropic.ts | 2 +- src/server/models/google.ts | 6 +- src/server/models/openai.ts | 2 +- src/server/services/model.ts | 19 ++++-- wrangler.jsonc | 5 ++ 11 files changed, 182 insertions(+), 27 deletions(-) create mode 100644 .github/dependabot.yml create mode 100644 .github/workflows/codeql.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..05a7337 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,11 @@ +version: 2 +updates: + - package-ecosystem: "npm" + directory: "/" + schedule: + interval: "weekly" + + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ed0bd40..56df09a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,6 +14,9 @@ concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true +permissions: + contents: read + jobs: verify: name: Verify Stability @@ -46,10 +49,10 @@ jobs: steps: - name: Checkout repository - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Setup Node.js - uses: actions/setup-node@v4 + uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: node-version: 20 cache: 'npm' diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 0000000..313d1dd --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,33 @@ +name: CodeQL + +on: + push: + branches: [main] + pull_request: + branches: [main] + schedule: + - cron: '0 0 * * 1' + +jobs: + analyze: + name: Analyze + runs-on: ubuntu-latest + permissions: + contents: read + security-events: write + + strategy: + matrix: + language: ['javascript-typescript'] + + steps: + - name: Checkout repository + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + + - name: Initialize CodeQL + uses: github/codeql-action/init@411c4c9a36b3fca4d674f06b6396b2c6d23522c6 # v3.36.3 + with: + languages: ${{ matrix.language }} + + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@411c4c9a36b3fca4d674f06b6396b2c6d23522c6 # v3.36.3 diff --git a/src/client/components/ui/select.tsx b/src/client/components/ui/select.tsx index f5f9bbc..e6a9173 100644 --- a/src/client/components/ui/select.tsx +++ b/src/client/components/ui/select.tsx @@ -2,6 +2,7 @@ import { Check, ChevronDown } from 'lucide-react'; import { motion, type Transition, useReducedMotion, type Variants } from 'motion/react'; import { type CSSProperties, + type KeyboardEvent as ReactKeyboardEvent, type ReactNode, useEffect, useId, @@ -76,9 +77,24 @@ export function Select({ const [placement, setPlacement] = useState('bottom'); const [height, setHeight] = useState(0); const [rect, setRect] = useState<{ left: number; width: number; top: number; bottom: number } | null>(null); + const [highlightedIndex, setHighlightedIndex] = useState(0); + const optionRefs = useRef>([]); const selectedOption = options.find((opt) => opt.value === value); + // Move the active-descendant highlight onto the current selection (or the first option) + // each time the listbox opens, so arrow-key navigation always starts from a sane position. + useEffect(() => { + if (!open) return; + const idx = options.findIndex((opt) => opt.value === value); + setHighlightedIndex(idx >= 0 ? idx : 0); + }, [open, options, value]); + + useEffect(() => { + if (!open) return; + optionRefs.current[highlightedIndex]?.scrollIntoView({ block: 'nearest' }); + }, [open, highlightedIndex]); + // close on outside pointer / escape useEffect(() => { if (!open) return; @@ -105,15 +121,19 @@ export function Select({ const observer = new ResizeObserver(measure); observer.observe(node); return () => observer.disconnect(); - }); + }, []); // Track the trigger's viewport position so the portaled panel can follow it, - // and flip upward when there isn't room below and there's more above. + // and flip upward when there isn't room below and there's more above. Scroll/resize + // fire far more often than the display repaints, so batch updates to at most once per + // animation frame instead of re-rendering on every raw event. useLayoutEffect(() => { if (!open) return; const trigger = triggerRef.current; if (!trigger) return; + let frame: number | null = null; const update = () => { + frame = null; const r = trigger.getBoundingClientRect(); setRect({ left: r.left, width: r.width, top: r.top, bottom: r.bottom }); const h = innerRef.current?.offsetHeight ?? 0; @@ -121,15 +141,64 @@ export function Select({ const above = r.top; setPlacement(below < h + 16 && above > below ? 'top' : 'bottom'); }; + const scheduleUpdate = () => { + if (frame !== null) return; + frame = requestAnimationFrame(update); + }; update(); - window.addEventListener('scroll', update, true); - window.addEventListener('resize', update); + window.addEventListener('scroll', scheduleUpdate, true); + window.addEventListener('resize', scheduleUpdate); return () => { - window.removeEventListener('scroll', update, true); - window.removeEventListener('resize', update); + if (frame !== null) cancelAnimationFrame(frame); + window.removeEventListener('scroll', scheduleUpdate, true); + window.removeEventListener('resize', scheduleUpdate); }; }, [open]); + const moveHighlight = (next: number) => { + setHighlightedIndex(Math.min(Math.max(next, 0), options.length - 1)); + }; + + const onTriggerKeyDown = (e: ReactKeyboardEvent) => { + if (!open) { + if (e.key === 'ArrowDown' || e.key === 'ArrowUp' || e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + setOpen(true); + } + return; + } + if (options.length === 0) return; + switch (e.key) { + case 'ArrowDown': + e.preventDefault(); + moveHighlight(highlightedIndex + 1); + break; + case 'ArrowUp': + e.preventDefault(); + moveHighlight(highlightedIndex - 1); + break; + case 'Home': + e.preventDefault(); + moveHighlight(0); + break; + case 'End': + e.preventDefault(); + moveHighlight(options.length - 1); + break; + case 'Enter': + case ' ': + e.preventDefault(); + onValueChange(options[highlightedIndex].value); + setOpen(false); + break; + case 'Tab': + setOpen(false); + break; + default: + break; + } + }; + const isTop = placement === 'top'; // Gooey: the edge facing the panel snaps flat (panel attached) then rounds @@ -167,7 +236,9 @@ export function Select({ aria-haspopup="listbox" aria-expanded={open} aria-controls={listId} + aria-activedescendant={open && options[highlightedIndex] ? `${listId}-option-${highlightedIndex}` : undefined} onClick={() => setOpen((v) => !v)} + onKeyDown={onTriggerKeyDown} initial={false} animate={{ borderTopLeftRadius: isTop ? kf : 12, @@ -271,14 +342,21 @@ export function Select({ animate={open ? 'show' : 'hidden'} className="max-h-[min(28rem,60vh)] overflow-y-auto p-1" > - {options.map((option) => { + {options.map((option, index) => { const selected = option.value === value; + const highlighted = index === highlightedIndex; return (
- + {file.fileStatus === 'done' && } {file.parsedComments.length > 0 && ( {file.parsedComments.length} diff --git a/src/client/components/features/job-detail/job-findings-list.tsx b/src/client/components/features/job-detail/job-findings-list.tsx index dd8b821..a1b4b32 100644 --- a/src/client/components/features/job-detail/job-findings-list.tsx +++ b/src/client/components/features/job-detail/job-findings-list.tsx @@ -53,6 +53,30 @@ export function JobFindingsList({ job }: JobFindingsListProps) {
) : (
+ {job.files.some((f) => f.fileStatus === 'failed') && ( +
+ {/* Group header */} +
+ + + Failed Files + + + {job.files.filter((f) => f.fileStatus === 'failed').length} + +
+ {/* Failed files list */} +
+ {job.files.filter((f) => f.fileStatus === 'failed').map((file) => ( + + ))} +
+
+ )} + {reviewSeverities.map((groupName) => { const comments = job.files.flatMap((f) => f.parsedComments diff --git a/src/server/core/review.ts b/src/server/core/review.ts index 447c996..5d20a7f 100644 --- a/src/server/core/review.ts +++ b/src/server/core/review.ts @@ -46,7 +46,16 @@ export type ReviewJobRunResult = const REVIEW_CHUNK_WALL_CLOCK_MS = 12 * 60 * 1000; const JOB_LEASE_SECONDS = 15 * 60; const BUSY_RETRY_SECONDS = 60; -const RETRYABLE_MODEL_FAILURE_RETRY_DELAYS_SECONDS = [60, 5 * 60, 15 * 60]; +// Backoff between deferred retries of a file that hit a transient model/provider failure. +// Kept short at first: most observed failures are momentary provider load (Gemini 500/503 +// "high demand") or self-inflicted connection queuing, both of which clear within seconds, +// so a long first delay just makes reviews grind. Later attempts back off harder in case the +// provider really is having an outage. +const RETRYABLE_MODEL_FAILURE_RETRY_DELAYS_SECONDS = [30, 2 * 60, 5 * 60]; +// Subrequest-budget exhaustion is not a provider outage: the budget resets the moment a fresh +// invocation starts, so the continuation only needs a token pause (the workflow already forces +// a >=1s yield between phases), not a provider-style backoff. +const SUBREQUEST_BUDGET_RETRY_DELAY_SECONDS = 5; const MAX_RETRYABLE_FILE_REVIEW_FAILURES = 3; // Belt-and-suspenders ceiling on how many times a job may reschedule the *same* phase without // completing a single file (see markJobContinuationQueued / resetJobContinuationCount). The @@ -337,7 +346,12 @@ export async function runReviewJob(env: AppBindings, message: ReviewJobMessage): // whole review. Finalize is intentionally excluded -- it posts the GitHub review and isn't // safe to re-run, and with no AI calls it does not realistically approach the cap anyway. if (phase !== 'finalize' && isSubrequestBudgetError(error)) { - const delaySeconds = getRetryableModelFailureDelaySeconds(error); + // Honor an explicit retryAfterSeconds if one was attached; otherwise resume almost + // immediately -- the fresh invocation is what fixes budget exhaustion, not waiting. + const record = error && typeof error === 'object' ? error as { retryAfterSeconds?: unknown } : null; + const delaySeconds = typeof record?.retryAfterSeconds === 'number' + ? record.retryAfterSeconds + : SUBREQUEST_BUDGET_RETRY_DELAY_SECONDS; logger.warn(`Review job hit the per-invocation subrequest limit; rescheduling ${phase} on a fresh budget: ${job.owner}/${job.repo} PR #${job.prNumber}`, { error: messageText, phase, @@ -766,7 +780,7 @@ async function reviewAndPersistFile( error: errorMessage, }); Object.defineProperty(error, 'retryAfterSeconds', { - value: RETRYABLE_MODEL_FAILURE_RETRY_DELAYS_SECONDS[0], + value: SUBREQUEST_BUDGET_RETRY_DELAY_SECONDS, configurable: true, }); throw error; diff --git a/src/server/models/anthropic.ts b/src/server/models/anthropic.ts index 3e45da1..423bf3d 100644 --- a/src/server/models/anthropic.ts +++ b/src/server/models/anthropic.ts @@ -15,16 +15,17 @@ export interface AnthropicResponse { } export async function reviewWithAnthropic( - config: { apiKey: string; baseUrl?: string | null; providerName: string }, + config: { apiKey: string; baseUrl?: string | null; providerName: string; timeoutMs?: number }, model: string, input: { systemPrompt: string; userPrompt: string }, tracker?: { incrementSubrequests(count?: number): void }, ): Promise { logger.info(`Calling Anthropic model: ${model}`); const baseUrl = (config.baseUrl || DEFAULT_ANTHROPIC_BASE_URL).replace(/\/+$/, ''); + const timeoutMs = config.timeoutMs ?? ANTHROPIC_TIMEOUT_MS; if (tracker) tracker.incrementSubrequests(1); - const response = await withTimeout('Anthropic API', ANTHROPIC_TIMEOUT_MS, (signal) => + const response = await withTimeout('Anthropic API', timeoutMs, (signal) => fetch(`${baseUrl}/messages`, { method: 'POST', signal, diff --git a/src/server/models/cloudflare.ts b/src/server/models/cloudflare.ts index e3c51ee..91a1670 100644 --- a/src/server/models/cloudflare.ts +++ b/src/server/models/cloudflare.ts @@ -4,13 +4,14 @@ import { TimeoutError } from '@server/core/timeout'; import { ProviderRequestError, type ModelResponse } from './types'; /** - * Max wall-clock time allowed for a single Workers-AI call. Kept well under the review - * workflow's 15-minute step timeout: a model that hasn't answered a code-review prompt in - * this long (reasoning models under strict-JSON decoding are the usual offenders -- they burn - * the whole token budget "thinking" and never emit the JSON) is not going to, so we fail fast - * and let the file defer to a fresh invocation instead of stalling the whole review. + * Default max wall-clock time allowed for a single Workers-AI call when the caller doesn't + * supply a diff-size-aware budget. Kept well under the review workflow's 15-minute step + * timeout: a model that hasn't answered a code-review prompt in this long (reasoning models + * under strict-JSON decoding are the usual offenders -- they burn the whole token budget + * "thinking" and never emit the JSON) is not going to, so we fail fast and let the file defer + * to a fresh invocation instead of stalling the whole review. */ -const CLOUDFLARE_TIMEOUT_MS = 60_000; +const CLOUDFLARE_TIMEOUT_MS = 45_000; const CLOUDFLARE_MAX_RETRIES = 0; const CLOUDFLARE_MAX_OUTPUT_TOKENS = 8192; const REVIEW_RESPONSE_SCHEMA = { @@ -167,8 +168,10 @@ export async function reviewWithCloudflare( input: { systemPrompt: string; userPrompt: string }, tracker?: { incrementSubrequests(count?: number): void }, providerName = 'Cloudflare', + options?: { timeoutMs?: number }, ): Promise { const maxRetries = CLOUDFLARE_MAX_RETRIES; + const timeoutMs = options?.timeoutMs ?? CLOUDFLARE_TIMEOUT_MS; let lastError: unknown; for (let attempt = 0; attempt <= maxRetries; attempt++) { @@ -182,8 +185,8 @@ export async function reviewWithCloudflare( const timeoutPromise = new Promise((_, reject) => { timer = setTimeout(() => { controller.abort(); - reject(new TimeoutError(`Cloudflare (${model})`, CLOUDFLARE_TIMEOUT_MS)); - }, CLOUDFLARE_TIMEOUT_MS); + reject(new TimeoutError(`Cloudflare (${model})`, timeoutMs)); + }, timeoutMs); }); try { diff --git a/src/server/models/google.ts b/src/server/models/google.ts index 32af0f3..a8dda6a 100644 --- a/src/server/models/google.ts +++ b/src/server/models/google.ts @@ -2,8 +2,9 @@ import { logger } from '@server/core/logger'; import { withTimeout } from '@server/core/timeout'; import { ProviderRequestError, providerErrorMessage, type ModelResponse } from './types'; -/** Max wall-clock time allowed for a single Google AI Studio call. */ -const GEMINI_TIMEOUT_MS = 80_000; +/** Default max wall-clock time for a single Google AI Studio call when the caller doesn't + * supply a diff-size-aware budget. */ +const GEMINI_TIMEOUT_MS = 45_000; const GEMINI_MAX_RETRIES = 1; const GEMINI_MAX_OUTPUT_TOKENS = 4096; const DEFAULT_GEMINI_BASE_URL = 'https://generativelanguage.googleapis.com/v1beta'; @@ -65,11 +66,12 @@ function isValidPublicUrl(urlString: string) { } export async function reviewWithGoogle( - config: { apiKey: string; baseUrl?: string | null; providerName?: string }, + config: { apiKey: string; baseUrl?: string | null; providerName?: string; timeoutMs?: number }, model: string, input: { systemPrompt: string; userPrompt: string }, tracker?: { incrementSubrequests(count?: number): void }, ): Promise { + const timeoutMs = config.timeoutMs ?? GEMINI_TIMEOUT_MS; logger.info(`Calling Google model: ${model}`); if (config.baseUrl && !isValidPublicUrl(config.baseUrl)) { @@ -93,7 +95,7 @@ export async function reviewWithGoogle( let response: Response; try { if (tracker) tracker.incrementSubrequests(1); - response = await withTimeout('Gemini API', GEMINI_TIMEOUT_MS, (signal) => + response = await withTimeout('Gemini API', timeoutMs, (signal) => fetch(url, { method: 'POST', signal, diff --git a/src/server/models/limits.ts b/src/server/models/limits.ts new file mode 100644 index 0000000..3ef41a2 --- /dev/null +++ b/src/server/models/limits.ts @@ -0,0 +1,84 @@ +/** + * Shared throttling/timeout policy for outbound model calls. + * + * Two hard Cloudflare Workers Free-plan constraints shape everything here: + * + * 1. Each Worker invocation may have at most SIX connections simultaneously waiting for + * response headers. Anything beyond that (fetch, AI binding, KV, Hyperdrive) is silently + * QUEUED by the runtime -- it does not error, it just doesn't start. A model call that sits + * queued behind other long-running model calls will burn its own client-side timeout without + * the request ever being dispatched, which shows up in logs as a provider "timing out" at + * exactly the configured timeout on every attempt. + * 2. 50 subrequests per invocation, so retries are expensive and every queued-then-timed-out + * call is a wasted subrequest. + */ + +/** Base wall-clock budget for a model call reviewing a small diff. Successful review calls in + * production land in the ~10-50s range; anything slower on a small prompt is almost always a + * stuck/overloaded model, so fail over to the next model quickly instead of stalling the chunk. */ +export const MODEL_TIMEOUT_BASE_MS = 45_000; +/** Extra time granted per diff line beyond MODEL_TIMEOUT_FREE_LINES -- large diffs legitimately + * need longer generations. */ +export const MODEL_TIMEOUT_PER_LINE_MS = 100; +/** Diff lines included in the base budget before per-line scaling kicks in. */ +export const MODEL_TIMEOUT_FREE_LINES = 100; +/** Ceiling regardless of diff size; keeps a single call well under the workflow's 15-minute + * step timeout even with several fallbacks. */ +export const MODEL_TIMEOUT_MAX_MS = 120_000; + +/** + * Wall-clock timeout for one model call, scaled by the size of the (already truncated) diff + * the model has to review. Small diffs fail over fast; large diffs get up to 2 minutes. + */ +export function adaptiveModelTimeoutMs(diffLineCount: number | null | undefined): number { + const lines = typeof diffLineCount === 'number' && Number.isFinite(diffLineCount) ? Math.max(0, diffLineCount) : 0; + const scaled = MODEL_TIMEOUT_BASE_MS + Math.max(0, lines - MODEL_TIMEOUT_FREE_LINES) * MODEL_TIMEOUT_PER_LINE_MS; + return Math.min(MODEL_TIMEOUT_MAX_MS, scaled); +} + +/** + * Max model calls in flight at once for a single invocation. Kept below the runtime's + * 6-connection cap so short-lived KV/Hyperdrive/GitHub requests issued by concurrent file + * reviews still have free connection slots and model calls are never queued behind each other + * by the runtime (queued calls burn their timeout without ever being dispatched). + */ +export const MAX_CONCURRENT_MODEL_CALLS = 3; + +/** + * Tiny FIFO semaphore. Callers wait *before* their provider timeout starts, so waiting for a + * slot never eats into a model call's own time budget. + */ +export class ModelCallGate { + private active = 0; + private readonly waiters: Array<() => void> = []; + + constructor(private readonly limit = MAX_CONCURRENT_MODEL_CALLS) {} + + async run(fn: () => Promise): Promise { + await this.acquire(); + try { + return await fn(); + } finally { + this.release(); + } + } + + private acquire(): Promise { + if (this.active < this.limit) { + this.active++; + return Promise.resolve(); + } + return new Promise((resolve) => this.waiters.push(resolve)); + } + + private release() { + // Hand the slot directly to the next waiter (active count unchanged) so a newly arriving + // caller can't sneak in between the release and the waiter resuming. + const next = this.waiters.shift(); + if (next) { + next(); + } else { + this.active--; + } + } +} diff --git a/src/server/models/openai.ts b/src/server/models/openai.ts index 0272930..aa13ab6 100644 --- a/src/server/models/openai.ts +++ b/src/server/models/openai.ts @@ -63,12 +63,13 @@ function isValidPublicUrl(urlString: string) { } export async function reviewWithOpenAI( - config: { apiKey: string | null; baseUrl: string; providerName: string }, + config: { apiKey: string | null; baseUrl: string; providerName: string; timeoutMs?: number }, model: string, input: { systemPrompt: string; userPrompt: string }, tracker?: { incrementSubrequests(count?: number): void }, ): Promise { logger.info(`Calling OpenAI-format model: ${model}`); + const timeoutMs = config.timeoutMs ?? OPENAI_TIMEOUT_MS; if (!isValidPublicUrl(config.baseUrl)) { throw new ProviderRequestError(config.providerName, 400, 'Invalid provider base URL.'); @@ -77,7 +78,7 @@ export async function reviewWithOpenAI( const url = `${config.baseUrl.replace(/\/+$/, '')}/chat/completions`; if (tracker) tracker.incrementSubrequests(1); - const response = await withTimeout('OpenAI API', OPENAI_TIMEOUT_MS, (signal) => + const response = await withTimeout('OpenAI API', timeoutMs, (signal) => fetch(url, { method: 'POST', signal, diff --git a/src/server/services/model.ts b/src/server/services/model.ts index ffe35df..dafe1da 100644 --- a/src/server/services/model.ts +++ b/src/server/services/model.ts @@ -14,6 +14,7 @@ import { logger } from '../core/logger'; import { normalizeModelId } from '@shared/schema'; import { getResolvedModelConfig, type ResolvedModelConfig } from '@server/db/model-configs'; import { decryptLlmApiKey } from '@server/core/llm-crypto'; +import { ModelCallGate, adaptiveModelTimeoutMs } from '../models/limits'; const PROVIDER_UNAVAILABLE_TTL_SECONDS = 24 * 60 * 60; const COMPACT_REVIEW_PROMPT_LINE_CAP = 400; @@ -95,6 +96,19 @@ export class ModelService { // their own. private readonly resolvedModelCache = new Map>(); + // The Workers runtime allows only 6 simultaneous connections per invocation; anything beyond + // that is queued without starting. When several files review in parallel, un-gated model + // calls queue behind each other and burn their entire client timeout before the request is + // even dispatched (observed as a provider "timing out" at exactly the configured timeout on + // every attempt). Gate all outbound model calls for this invocation so a call's timeout only + // starts once it actually has a connection slot. + private readonly callGate = new ModelCallGate(); + + // Provider-unavailable markers live in KV and every read is a counted subrequest. The marker + // can't flip from set back to unset within one invocation, so cache lookups per instance + // (one instance == one invocation) instead of re-reading KV for every file in the chunk. + private readonly providerUnavailableCache = new Map>(); + constructor( private env: AppBindings, private tracker?: TokenTracker, @@ -105,25 +119,37 @@ export class ModelService { return this.options.jobId ? `jobs:${this.options.jobId}:provider-unavailable:${providerId}` : null; } - private async isProviderUnavailable(providerId: string) { + private isProviderUnavailable(providerId: string): Promise { const key = this.providerUnavailableKey(providerId); - if (!key) return false; + if (!key) return Promise.resolve(false); - try { - return (await this.env.APP_KV.get(key)) !== null; - } catch (error) { - logger.warn(`Failed to read unavailable provider marker for ${providerId}`, { - error: error instanceof Error ? error.message : String(error), - }); - return false; + let pending = this.providerUnavailableCache.get(providerId); + if (!pending) { + pending = (async () => { + try { + this.tracker?.incrementSubrequests(1); + return (await this.env.APP_KV.get(key)) !== null; + } catch (error) { + logger.warn(`Failed to read unavailable provider marker for ${providerId}`, { + error: error instanceof Error ? error.message : String(error), + }); + return false; + } + })(); + this.providerUnavailableCache.set(providerId, pending); } + return pending; } private async markProviderUnavailable(providerId: string, reason: string) { const key = this.providerUnavailableKey(providerId); if (!key) return; + // Keep the in-invocation cache consistent with what we just wrote. + this.providerUnavailableCache.set(providerId, Promise.resolve(true)); + try { + this.tracker?.incrementSubrequests(1); await this.env.APP_KV.put( key, JSON.stringify({ @@ -204,43 +230,59 @@ export class ModelService { private async callResolvedModel( config: ResolvedModelConfig, input: { systemPrompt: string; userPrompt: string }, + timeoutMs?: number, ): Promise { + // Resolve credentials *before* taking a gate slot so slow KV/crypto work never occupies a + // model-call slot, then run the actual provider request under the gate. The provider's + // timeout only starts inside the gated call, so time spent waiting for a slot is free. if (config.apiFormat === 'cloudflare-workers-ai') { - return reviewWithCloudflare(this.env, config.modelName, input, this.tracker, config.providerName); + return this.callGate.run(() => + reviewWithCloudflare(this.env, config.modelName, input, this.tracker, config.providerName, { timeoutMs }), + ); } if (config.apiFormat === 'gemini') { - return reviewWithGoogle( - { apiKey: await this.decryptApiKey(config), baseUrl: config.baseUrl, providerName: config.providerName }, - config.modelName, - input, - this.tracker, + const apiKey = await this.decryptApiKey(config); + return this.callGate.run(() => + reviewWithGoogle( + { apiKey, baseUrl: config.baseUrl, providerName: config.providerName, timeoutMs }, + config.modelName, + input, + this.tracker, + ), ); } if (config.apiFormat === 'openai') { - return reviewWithOpenAI( - { - apiKey: await this.decryptApiKey(config), - baseUrl: config.baseUrl || 'https://api.openai.com/v1', - providerName: config.providerName, - }, - config.modelName, - input, - this.tracker, + const apiKey = await this.decryptApiKey(config); + return this.callGate.run(() => + reviewWithOpenAI( + { + apiKey, + baseUrl: config.baseUrl || 'https://api.openai.com/v1', + providerName: config.providerName, + timeoutMs, + }, + config.modelName, + input, + this.tracker, + ), ); } - return reviewWithAnthropic( - { apiKey: await this.decryptApiKey(config), baseUrl: config.baseUrl, providerName: config.providerName }, - config.modelName, - input, - this.tracker, + const apiKey = await this.decryptApiKey(config); + return this.callGate.run(() => + reviewWithAnthropic( + { apiKey, baseUrl: config.baseUrl, providerName: config.providerName, timeoutMs }, + config.modelName, + input, + this.tracker, + ), ); } - private async callModel(model: string, input: { systemPrompt: string; userPrompt: string }): Promise { - return this.callResolvedModel(await this.resolveModel(model), input); + private async callModel(model: string, input: { systemPrompt: string; userPrompt: string }, timeoutMs?: number): Promise { + return this.callResolvedModel(await this.resolveModel(model), input, timeoutMs); } async reviewFile(params: { @@ -268,6 +310,10 @@ export class ModelService { }); const modelsToTry = [primary, ...fallbacks]; + // Size the per-call timeout to the diff the model actually sees (post-truncation): small + // files fail over to the next model fast; large diffs get a proportionally longer budget. + const timeoutMs = adaptiveModelTimeoutMs(reviewFile.lineCount); + let lastError: unknown; let lastTransientError: unknown; let sawTransientFailure = false; @@ -309,7 +355,7 @@ export class ModelService { while (attempts < maxAttempts) { try { - const response = await this.callResolvedModel(resolved, { systemPrompt, userPrompt }); + const response = await this.callResolvedModel(resolved, { systemPrompt, userPrompt }, timeoutMs); if (this.tracker) { this.tracker.record(response.modelUsed, response.inputTokens, response.outputTokens); @@ -398,7 +444,7 @@ export class ModelService { const response = await this.callResolvedModel(resolved, { systemPrompt: SUMMARY_SYSTEM_PROMPT, userPrompt: buildSummaryPrompt(params), - }); + }, adaptiveModelTimeoutMs(0)); if (this.tracker) { this.tracker.record(response.modelUsed, response.inputTokens, response.outputTokens); diff --git a/test/model-limits.spec.ts b/test/model-limits.spec.ts new file mode 100644 index 0000000..01f4582 --- /dev/null +++ b/test/model-limits.spec.ts @@ -0,0 +1,62 @@ +import { describe, expect, it } from 'vitest'; +import { + ModelCallGate, + adaptiveModelTimeoutMs, + MODEL_TIMEOUT_BASE_MS, + MODEL_TIMEOUT_MAX_MS, +} from '../src/server/models/limits'; + +describe('adaptiveModelTimeoutMs', () => { + it('uses the base budget for small diffs', () => { + expect(adaptiveModelTimeoutMs(0)).toBe(MODEL_TIMEOUT_BASE_MS); + expect(adaptiveModelTimeoutMs(100)).toBe(MODEL_TIMEOUT_BASE_MS); + expect(adaptiveModelTimeoutMs(undefined)).toBe(MODEL_TIMEOUT_BASE_MS); + expect(adaptiveModelTimeoutMs(null)).toBe(MODEL_TIMEOUT_BASE_MS); + }); + + it('scales with diff size beyond the free-line allowance', () => { + expect(adaptiveModelTimeoutMs(400)).toBe(MODEL_TIMEOUT_BASE_MS + 300 * 100); + expect(adaptiveModelTimeoutMs(400)).toBeGreaterThan(adaptiveModelTimeoutMs(200)); + }); + + it('caps at the maximum regardless of diff size', () => { + expect(adaptiveModelTimeoutMs(100_000)).toBe(MODEL_TIMEOUT_MAX_MS); + }); +}); + +describe('ModelCallGate', () => { + it('never runs more than the limit concurrently and eventually runs everything', async () => { + const gate = new ModelCallGate(2); + let active = 0; + let peak = 0; + const done: number[] = []; + + const task = (id: number) => + gate.run(async () => { + active++; + peak = Math.max(peak, active); + // Yield a couple of microtasks so tasks genuinely overlap. + await Promise.resolve(); + await Promise.resolve(); + active--; + done.push(id); + }); + + await Promise.all([task(1), task(2), task(3), task(4), task(5)]); + + expect(peak).toBeLessThanOrEqual(2); + expect(done).toHaveLength(5); + }); + + it('releases the slot when a gated call rejects', async () => { + const gate = new ModelCallGate(1); + + await expect(gate.run(async () => { + throw new Error('boom'); + })).rejects.toThrow('boom'); + + // The slot must be free again for the next caller. + const result = await gate.run(async () => 'ok'); + expect(result).toBe('ok'); + }); +}); diff --git a/test/model-service.spec.ts b/test/model-service.spec.ts index 2ced0a3..bf804cd 100644 --- a/test/model-service.spec.ts +++ b/test/model-service.spec.ts @@ -289,9 +289,9 @@ describe('ModelService', () => { // Prevent an unhandled-rejection warning while the timer is still pending. promise.catch(() => {}); - await vi.advanceTimersByTimeAsync(60_000); + await vi.advanceTimersByTimeAsync(45_000); - await expect(promise).rejects.toThrow('timed out after 60000ms'); + await expect(promise).rejects.toThrow('timed out after 45000ms'); // The underlying Workers-AI request was actually cancelled, not just abandoned. expect(capturedSignal?.aborted).toBe(true); } finally { diff --git a/test/review-flow.spec.ts b/test/review-flow.spec.ts index 88436d1..de6f614 100644 --- a/test/review-flow.spec.ts +++ b/test/review-flow.spec.ts @@ -405,7 +405,7 @@ dbDescribe('Review Flow Lifecycle', () => { phase: 'review', }); - expect(result).toEqual({ action: 'next_phase', phase: 'review', delaySeconds: 60 }); + expect(result).toEqual({ action: 'next_phase', phase: 'review', delaySeconds: 30 }); expect(reviewSpy).toHaveBeenCalled(); expect((env.REVIEW_QUEUE as any).sent).toHaveLength(0); });