Feat: Review reliability and performance overhaul and UI refactor#26
Feat: Review reliability and performance overhaul and UI refactor#26devarshishimpi wants to merge 14 commits into
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.
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7578742e00
ℹ️ 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); } |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7578742e00
ℹ️ 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); } |
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7578742e00
ℹ️ 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: