Release v0.9.4: job cancellation, telemetry opt-out, concurrency limits & motion UI#29
Release v0.9.4: job cancellation, telemetry opt-out, concurrency limits & motion UI#29devarshishimpi wants to merge 30 commits into
Conversation
- Deleted DLQ API routes and associated types from the client and server. - Removed DLQ message handling from the JobsPage component. - Cleaned up server-side job processing logic to eliminate DLQ dependencies. - Introduced a new ReviewWorkflow to manage job phases and execution. - Updated job schema to include workflow instance ID. - Refactored review job handling to support new workflow structure. - Adjusted tests to reflect changes in job processing and workflow management.
…ng and job management
…ror handling, and improve UI components
…king Feat: Add anonymous telemetry, UI enhancements, and remove DLQ
- 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
…, and optimize stats queries
… 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.
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
…ce job completion handling
…currency-refactor Feat: Review reliability and performance overhaul and UI refactor
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![codra-app-personal[bot]](https://avatars.githubusercontent.com/in/3347243?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08c954523f
ℹ️ 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
93 comments were omitted from this review to reduce noise and respect the configured max_comments limit (10). Showing the most critical issues.
| @@ -174,6 +172,21 @@ export const api = { | |||
| method: 'POST', | |||
| }); | |||
There was a problem hiding this comment.
The newly introduced rerunJob and stopJob methods in the api object use RetryJobResponse, but this type is not imported from the shared package, leading to a TypeScript compilation error.
| }); | |
| Import 'RetryJobResponse' from '@shared/api' or define it in the imports section. |
| children?: ReactNode; | ||
| }>; | ||
| const childKey = el.key ? String(el.key) : `item-${index}`; | ||
| return cloneElement( |
There was a problem hiding this comment.
The cloneElement function is called within the .map of children, where the children being rendered inside the clone contains the original child element el. This creates a recursive structure where the component attempts to render children infinitely (or until the call stack limits are reached), as it keeps nesting the component structure within itself every time the child is cloned.
| 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 removes default values and insert logic for 'rpm', 'tpm', and 'rpd' columns, but the database schema itself still contains these columns. If these columns have 'NOT NULL' constraints or application code depends on them, this will cause runtime failures. Ensure the database schema migration corresponds to these application-level changes to prevent 'null value violates not-null constraint' errors.
| SELECT $1, 10, 131072, 300, 'cloudflare', p.id, $1, now() | |
| Ensure the database table 'model_configs' has been altered to allow NULLs for 'rpm', 'tpm', and 'rpd' columns if they are not being dropped, or execute ALTER TABLE DROP COLUMN if they are no longer required. |
| } | ||
| } | ||
|
|
||
| const jobsSpinner = ora('Creating jobs queue (codra-review-jobs)...').start(); |
There was a problem hiding this comment.
The removal of the DLQ creation and ID extraction logic means that the 'CF_DLQ_ID' environment variable in 'wrangler.jsonc' will no longer be automatically updated. If the DLQ queue ID changes or is required for the application to function correctly, the setup process will fail or persist with incorrect/null configuration values without warning the user.
| const jobsSpinner = ora('Creating jobs queue (codra-review-jobs)...').start(); | |
| If the DLQ is no longer managed by this script, ensure the infrastructure-as-code documentation or the application bootstrap logic accounts for the missing 'CF_DLQ_ID' and provides a clear fallback or validation mechanism. |
| <div> | ||
| <p className="text-xs font-semibold uppercase tracking-widest text-primary/70 mb-1"> | ||
| {category} | ||
| </p> |
There was a problem hiding this comment.
The category prop was made optional, but the code that renders the category text (the paragraph element) was completely removed from the component. This ignores the intent of the prop change and makes it impossible to display the category in the UI.
| </p> | |
| {category && <p className="text-xs font-semibold uppercase tracking-widest text-primary/70 mb-1">{category}</p>} |
| </button> | ||
| </motion.li> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
The component uses 'document.body' directly in the createPortal call. In Next.js or other SSR frameworks, this code will throw an error on the server side because 'document' is undefined. Always check for the existence of 'document' or use a 'mounted' state/useEffect to render the portal only on the client.
| })} | |
| const [mounted, setMounted] = useState(false); | |
| useEffect(() => setMounted(true), []); | |
| if (!mounted) return null; | |
| return createPortal(..., document.body); |
| @@ -398,7 +398,7 @@ export function parseFileReviewResponse(raw: string, file: FileDiff): { | |||
| while (current !== prev) { | |||
There was a problem hiding this comment.
The while loop condition current !== prev relies on the regex replacement strictly decreasing the length or modifying the string. If the regex fails to match characters but the string structure results in an infinite loop due to non-converging state or specific character encodings, the server will block the event loop, causing a denial-of-service. While the current regex seems safer, it lacks an iteration limit to prevent hanging.
| while (current !== prev) { | |
| let iterations = 0; while (current !== prev && iterations < 10) { iterations++; ... } |
| // 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 stores database clients indefinitely for every unique connection string encountered. If env.HYPERDRIVE.connectionString is dynamic or if there are many unique environments during test execution, this will result in a permanent memory leak as entries are never removed.
| const fallbackClients = new Map<string, DbClient>(); | |
| Use a Least Recently Used (LRU) cache or a WeakMap if applicable, or implement a cleanup mechanism for entries no longer needed by the test suite. |
| verdict: verdictSummary.verdict, | ||
| severityDistribution, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The enqueueJobPhase function now throws a NextPhaseError. If this is caught by a broader generic catch block somewhere upstream that doesn't explicitly look for this custom error type, it could result in the job being erroneously marked as failed or having its state corrupted rather than triggering the expected phase transition.
| } | |
| Ensure the top-level job runner catch block explicitly checks for `NextPhaseError` and handles the queueing/delay logic properly instead of relying on an implicit re-throw. |
| @@ -29,6 +29,7 @@ export interface AppBindings { | |||
| AI: WorkersAiBinding; | |||
| APP_KV: KVNamespace; | |||
| REVIEW_QUEUE: QueueProducer<ReviewJobMessage>; | |||
There was a problem hiding this comment.
The interface 'AppBindings' now references a type 'Workflow' at line 32, but this type is not imported or defined within the provided file scope. This will cause a compilation error.
| REVIEW_QUEUE: QueueProducer<ReviewJobMessage>; | |
| import type { Workflow } from '@cloudflare/workers-types'; |
1 participant
Summary
This release bundles all
devwork merged since v0.9.2, focused on job lifecycle control, review reliability under concurrency, and UI polish.See CHANGELOG.md for the full breakdown.
Version bumped:
0.9.2→0.9.4(package.json, package-lock.json, SECURITY.md).Test plan