Feat: Review reliability and performance overhaul and UI refactor#26
Conversation
- 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.
- Updated the stats page to improve the layout and structure of metrics display, consolidating various graph components into a more cohesive MetricsGrid component. - Removed unused properties (rpm, tpm, rpd) from model configurations in the database schema and related queries. - Enhanced the stats retrieval function to include additional metrics such as job statuses, triggers, severities, categories, and performance metrics. - Updated the shared schema to reflect changes in the stats structure, including new fields for statuses, triggers, severities, categories, and performance. - Adjusted API model routes and validation schemas to remove references to removed properties. - Updated tests to align with the new data structure and ensure proper functionality.
…ew settings API, and add tests for concurrency limits
…ling in review process
![codra-app-personal[bot]](https://avatars.githubusercontent.com/in/3347243?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…, and optimize stats queries
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… handling - Updated jobStatuses to include 'cancelled' in schema. - Implemented API endpoint to stop ongoing jobs, marking them as 'cancelled'. - Added API endpoint for job deletion, ensuring jobs are removed from processing. - Introduced tests for stopping and deleting jobs, verifying correct status updates. - Enhanced review flow to inherit parent reviews correctly based on model configurations. - Added async batch review tests to ensure proper handling of review submissions and polling. - Improved error handling for Cloudflare model responses, ensuring failures are correctly reported. - Updated token tracker to reflect new safe margin limits and ensure budget tracking is accurate. - Added scheduled maintenance tests to verify proper handling of active jobs and maintenance work.
devarshishimpi
changed the title
devarshishimpi
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
refactor(db): remove batchInsertFileReviews function and related logic feat(jobs): add setJobPullRequestMeta to refresh job PR metadata refactor(jobs): remove startJobProcessing and recoverStaleJobs functions fix(model): cap in-call retry delay for Google 429 responses feat(model): classify 5xx errors as transient for retry logic feat(transient-errors): create shared transient error handling utilities test(model): add tests for transient error classification and retry logic test(review): ensure queued jobs respect concurrency limits while running jobs do not
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ce job completion handling
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9ae6d7f5f
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
Note
59 comments were omitted from this review to reduce noise and respect the configured max_comments limit (10). Showing the most critical issues.
| modelUsed: model, | ||
| provider: config.providerName ?? 'Google', | ||
| logger.error(`Gemini request failed with ${response.status}`, logData); | ||
| throw new ProviderRequestError(config.providerName ?? 'Google', response.status, message); |
There was a problem hiding this comment.
The startTime variable is used at line 176 (inside the try block or following lines), but it is never defined within the scope of reviewWithGoogle. It appears it was accidentally removed during the refactor. This will cause a ReferenceError when the code executes.
| throw new ProviderRequestError(config.providerName ?? 'Google', response.status, message); | |
| const startTime = Date.now(); // Should be initialized before the loop or inside the request block |
| ); | ||
|
|
||
| export function createSettingsRouter() { | ||
| const app = new Hono<AppEnv>(); |
There was a problem hiding this comment.
The settings router does not implement any authentication or authorization middleware. This allows any user with access to the API to read and modify sensitive system settings, potentially leading to unauthorized configuration changes.
| const app = new Hono<AppEnv>(); | |
| app.use('*', authMiddleware); // Add authentication middleware to the Hono instance |
| await query( | ||
| ` | ||
| INSERT INTO model_configs (model_id, rpm, tpm, rpd, provider, provider_id, model_name, updated_at) | ||
| SELECT $1, 10, 131072, 300, 'cloudflare', p.id, $1, now() |
There was a problem hiding this comment.
The migration script removes the 'rpm', 'tpm', and 'rpd' columns from the INSERT and UPDATE statements, but does not remove the NOT NULL constraints or default values for these columns in the database table schema itself. If the table columns remain defined as NOT NULL without default values, these INSERT statements will fail with a constraint violation error.
| SELECT $1, 10, 131072, 300, 'cloudflare', p.id, $1, now() | |
| Ensure the database table 'model_configs' has its columns 'rpm', 'tpm', and 'rpd' updated to allow NULL values, or provide default values for them in the table schema definition. |
| } finally { | ||
| setIsStopping(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The handleDelete function handles potential errors in the catch block but fails to reset the isDeleting state inside a finally block. If the operation fails, the loading state will remain true indefinitely, preventing subsequent user interactions.
| }; | |
| Move 'setIsDeleting(false)' into a finally block to ensure the state is reset regardless of the outcome. |
| </button> | ||
| </motion.li> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
Using document.body directly in createPortal is risky in Server-Side Rendering (SSR) environments like Next.js because document is undefined on the server. This will cause the component to crash during server-side hydration. It is safer to check if typeof document !== 'undefined' or use a ref-based container.
| })} | |
| const [container, setContainer] = useState<HTMLElement | null>(null); useEffect(() => setContainer(document.body), []); return container ? createPortal(..., container) : null; |
| useEffect(() => { | ||
| if (ctx.open) { | ||
| wasOpen.current = true; | ||
| const id = requestAnimationFrame(() => getItems()[0]?.focus()); |
There was a problem hiding this comment.
The custom implementation of DropdownMenuContent uses requestAnimationFrame to focus the first menu item when it opens. This is fragile because it doesn't guarantee that the DOM node is rendered or available. Furthermore, manually managing focus states and event listeners (keydown, pointerdown, resize, scroll) is complex and prone to edge-case bugs compared to using established libraries like Radix UI or Floating UI. The current implementation lacks robust focus trapping logic.
| const id = requestAnimationFrame(() => getItems()[0]?.focus()); | |
| Use a production-ready accessibility library like @floating-ui/react or keep using @radix-ui/react-dropdown-menu. |
| }; | ||
| }, [ctx.open, ctx.triggerRef, side, align, sideOffset, alignOffset]); | ||
|
|
||
| const onKeyDown = (e: ReactKeyboardEvent<HTMLDivElement>) => { |
There was a problem hiding this comment.
The menu implementation lacks a focus trap. Keyboard users navigating within the menu using arrow keys can easily tab out of the menu, which breaks standard accessibility patterns for dropdown menus where focus should be contained while the menu is active.
| lineCount: 0, | ||
| hunks: [], | ||
| }; | ||
|
|
There was a problem hiding this comment.
In parseUnifiedDiff, the isIgnored variable is only set when a file header or change is encountered. However, if a file starts and does not explicitly match a skip rule until a rename to or +++ line is processed, lines processed before those identifiers (such as lines after diff --git but before +++) might be incorrectly included in the diff. Additionally, isIgnored is not reset correctly for every file context if a file is marked ignored mid-process.
| Ensure `isIgnored` is evaluated immediately after the `diff --git` header and explicitly re-checked or strictly enforced before processing hunk lines. |
| 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, '') |
There was a problem hiding this comment.
The regex ^(?:[^\w\s]+|(?:QUALITY|SECURITY|BUG|PERFORMANCE|CORRECTNESS|P[0-3]|NIT)\b)+ uses nested repetition (a quantifier applied to a group containing alternations and another quantifier). In pathological input strings, this can lead to catastrophic backtracking, causing significant performance degradation or a DoS vulnerability. Using non-capturing groups helps, but the nested structure is risky for input sanitization.
| .replace(/^(?:[^\w\s]+|(?:QUALITY|SECURITY|BUG|PERFORMANCE|CORRECTNESS|P[0-3]|NIT)\b)+/giu, '') | |
| Refactor the regex to avoid nested quantifiers, such as splitting into two separate .replace() calls or using a more constrained pattern. |
| // Reused clients for the no-runWithDb() fallback path below. Keyed by connection string so a caller | ||
| // that queries outside a runWithDb() scope shares one bounded pool instead of leaking a fresh pool | ||
| // (up to `max` connections) on every single query. | ||
| const fallbackClients = new Map<string, DbClient>(); |
There was a problem hiding this comment.
The fallbackClients map caches DbClient instances indefinitely. While intended to prevent connection exhaustion in tests, these clients are never disposed of or closed. In a long-running server process or environments where connection strings might rotate or change, this creates a permanent memory leak and a growing number of zombie connection pools. If the application environment changes, these stale clients remain in memory forever.
| const fallbackClients = new Map<string, DbClient>(); | |
| // Implement a cleanup mechanism or ensure DbClient instances can be gracefully closed when no longer needed. |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9ae6d7f5f
ℹ️ About Codra in GitHub
Your team has set up Codra to review pull requests in this repo. Reviews are triggered when you:
- Open a pull request for review
- Mark a draft as ready
- Comment "@codra-app review"
If Codra has suggestions, it will comment; otherwise it will react with 👍.
Codra can also answer questions or update the PR. Try commenting "@codra-app address that feedback".
Note
52 comments were omitted from this review to reduce noise and respect the configured max_comments limit (10). Showing the most critical issues.
| candidatesTokenCount?: number; | ||
| }; | ||
| const logData = { | ||
| error: message, |
There was a problem hiding this comment.
The variable 'startTime' is referenced on line 157, but it is not defined within the scope of the reviewWithGoogle function, causing a ReferenceError. It appears this was removed from the scope of the loop in the refactor.
| error: message, | |
| const startTime = Date.now(); // Define before the loop |
| rpm: optionalLimitSchema, | ||
| tpm: optionalLimitSchema, | ||
| rpd: optionalLimitSchema, | ||
| }).strict(); |
There was a problem hiding this comment.
The diff removes the rpm, tpm, and rpd validation schemas from the modelConfigUpdateSchema. If these fields are intended to be updated, they are no longer being validated, which could lead to type errors, invalid data insertion into the database, or downstream logic failures when processing these inputs.
| }).strict(); | |
| Ensure these fields are either added back with appropriate schema validation or explicitly marked as not allowed in the schema. |
| } | ||
|
|
||
| const current = await getReviewSettings(c.env); | ||
| const next = reviewSettingsSchema.parse({ ...current, ...parsed.data }); |
There was a problem hiding this comment.
The code uses reviewSettingsSchema.parse() at line 35. If the schema validation fails (e.g., if the combined object is invalid despite the merge), this will throw a synchronous error, causing the Hono handler to crash and potentially resulting in a 500 error without proper cleanup. It is safer to use safeParse or handle the potential error gracefully.
| const next = reviewSettingsSchema.parse({ ...current, ...parsed.data }); | |
| const parsedNext = reviewSettingsSchema.safeParse({ ...current, ...parsed.data }); if (!parsedNext.success) return jsonError('Invalid updated configuration', 400); await updateReviewSettings(c.env, parsedNext.data); |
| "class_name": "ReviewWorkflow" | ||
| } | ||
| ], | ||
| "triggers": { |
There was a problem hiding this comment.
The 'triggers' configuration block is not a top-level property supported in the wrangler.jsonc schema for Cloudflare Workers. Scheduled events (cron triggers) must be defined within the 'triggers' array at the root level if using the deprecated format, or more commonly configured within the 'rules' or 'cron' specific fields depending on the version. However, placing 'triggers' as a top-level object containing a 'crons' property is not compliant with the standard Cloudflare Wrangler configuration schema.
| "triggers": { | |
| "triggers": { "crons": ["*/2 * * * *"] } should be removed or corrected to "triggers": ["*/2 * * * *"] at root level if supported, or handled via wrangler.toml specific syntax mapping. |
| await api.updateGlobalConfig(next); | ||
| setSavedGlobalConfig(next); | ||
| toast.success('Global strategy saved', { | ||
| id: tid, |
There was a problem hiding this comment.
The persistReviewSettings function updates the UI state (the reviewSettings state) before the API request completes. If multiple calls are made rapidly (e.g., if a user interacts with the slider multiple times), it's possible for the local state to diverge from the actual server state, especially since setReviewSettings is called immediately rather than waiting for the promise to resolve.
| id: tid, | |
| Update the state only after the API request successfully resolves to ensure the UI remains in sync with the source of truth. |
|
|
||
| const clamp = (value: number, min: number, max: number) => Math.min(Math.max(value, min), max); | ||
|
|
||
| export function DropdownMenuContent({ |
There was a problem hiding this comment.
The custom DropdownMenu implementation replicates complex accessibility and positioning logic (keyboard focus management, portal management, viewport collision detection) that is already robustly handled by libraries like @radix-ui/react-dropdown-menu. Reimplementing this manually introduces significant risks of accessibility regressions, z-index issues, and maintenance overhead.
| export function DropdownMenuContent({ | |
| Revert to using the @radix-ui/react-dropdown-menu library for primitives, as it provides screen reader support and focus trap management out of the box. |
| stroke={color} | ||
| strokeWidth={2.5} | ||
| fill="url(#reviewFill)" | ||
| dot={false} |
There was a problem hiding this comment.
The MetricsGrid component iterates over stats.statuses to map, calculate totals, and render UI elements without ensuring the statuses array exists or is properly initialized. If stats or stats.statuses were to be undefined or null due to API changes or data fetching issues, this will throw an unhandled exception crashing the component tree.
| dot={false} | |
| Use optional chaining for array methods and provide empty array fallback: `(stats.statuses ?? []).map(...)` |
| </div> | ||
| </div> | ||
|
|
||
| <aside className="border-t border-border bg-secondary/25 p-5 lg:border-l lg:border-t-0"> |
There was a problem hiding this comment.
The MetricsGridSkeleton component attempts to render Skeleton components. However, the Skeleton component is not imported in the provided diff snippet, suggesting it might be undefined or missing from the module scope, leading to runtime ReferenceErrors.
| <aside className="border-t border-border bg-secondary/25 p-5 lg:border-l lg:border-t-0"> | |
| Ensure Skeleton is imported or defined within the file scope. |
| 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, '') |
There was a problem hiding this comment.
The regex ^(?:[^ws]+|(?:QUALITY|SECURITY|BUG|PERFORMANCE|CORRECTNESS|P[0-3]|NIT)�)+ uses nested repetition (a quantifier on a group containing multiple alternations) which can lead to catastrophic backtracking on malicious or specifically crafted input strings. While the input is currently subjected to a while-loop for iterative replacement, the regex complexity should be minimized to ensure linear time complexity and prevent DoS vectors.
| .replace(/^(?:[^\w\s]+|(?:QUALITY|SECURITY|BUG|PERFORMANCE|CORRECTNESS|P[0-3]|NIT)\b)+/giu, '') | |
| Refactor to use a simpler, non-nested regex or sanitize input prior to matching if possible. Ensure clear boundary definitions to prevent excessive backtracking. |
| import { runReviewJob } from './core/review'; | ||
| import { ReviewWorkflow } from './workflows/review'; | ||
| import type { AppBindings } from './env'; | ||
| import { reviewJobMessageSchema } from '@shared/schema'; |
There was a problem hiding this comment.
The scheduled function calls runBestEffortJobMaintenance, but it does not include a try-catch block to handle potential promise rejections. In Cloudflare Workers, an unhandled exception in the scheduled event will crash the worker process and potentially prevent other background tasks from completing.
| import { reviewJobMessageSchema } from '@shared/schema'; | |
| try { await runBestEffortJobMaintenance(env); } catch (err) { console.error('Maintenance failed:', err); } |
1 participant
Description
This branch is a broad reliability and performance pass over the review pipeline, plus a UI and stats refactor.
Review performance settings
New
SteppedSliderandConfirmDialogcomponents let users configure job concurrency and max comment limits from the Settings page, with autosave and/api/settingsroutes backed by migration004_review_performance_settings.sql. Review logic now respects these limits.Model call reliability
New
src/server/models/limits.tscentralizes per call timeouts and concurrency limits for model calls, with model service caching improvements and hardened Cloudflare error handling so provider failures report correctly.Async batch reviews
File reviews can now be submitted to the Workers AI asynchronous Batch API (
submitCloudflareBatch/pollCloudflareBatch), decoupling long inference from any single invocation's timeout and subrequest cap. Migration007_file_review_async_batch.sqlpersists the queue request id so a later invocation can poll for the result.Job continuation tracking and budgets
Migration
006_job_continuation_count.sqladds acontinuation_countcolumn and a hard ceiling (MAX_JOB_CONTINUATIONS) that stops jobs which can never make progress from churning. Improved subrequest budget handling, job recovery, and token tracker safe margins.Job cancellation and deletion
New terminal
cancelledstatus (migration008_job_cancelled_status.sql), plus API endpoints to stop an in progress job and to delete a job, wired into the job detail UI.Diff parsing, motion UI, and stats
Refactored
diff.ts/review.tsto support custom file matchers. Added motion basedtabs.tsx, reworkeddropdown-menu.tsxandselect.tsx, and integrated Lenis smooth scrolling. Consolidated stats graphs into a singleMetricsGrid, expandedstats.tsto return statuses, triggers, severities, categories, and performance metrics, and dropped unusedrpm/tpm/rpdmodel fields (migration005_drop_model_rate_limits.sql). Added GitHub Actions CI, CodeQL, and Dependabot config.Closes #22 #23
Type of change
How Has This Been Tested?
Checklist: