Feat: Add review concurrency/comment limits, motion UI components, and stats page refactor#26
Feat: Add review concurrency/comment limits, motion UI components, and stats page refactor#26devarshishimpi wants to merge 12 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}
There was a problem hiding this comment.
Codra Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e48346dc1
ℹ️ 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
40 comments were omitted from this review to reduce noise and respect the configured max_comments limit (10). Showing the most critical issues.
| WHERE rpm = 1 AND tpm = 1 AND rpd = 1 | ||
| `); | ||
|
|
||
| await query( |
There was a problem hiding this comment.
The migration removes the commands that make 'rpm', 'tpm', and 'rpd' columns nullable (DROP NOT NULL), while simultaneously removing those columns from the subsequent INSERT statements. If these columns remain in the schema as NOT NULL and lack default values, the INSERT queries will fail with a constraint violation error. If these fields are deprecated, they should be explicitly dropped from the table using 'ALTER TABLE model_configs DROP COLUMN ...' to ensure schema consistency across environments.
| await query( | |
| // If the columns are no longer needed: | |
| await query('ALTER TABLE model_configs DROP COLUMN IF EXISTS rpm'); | |
| await query('ALTER TABLE model_configs DROP COLUMN IF EXISTS tpm'); | |
| await query('ALTER TABLE model_configs DROP COLUMN IF EXISTS rpd'); |
| import { | ||
| createContext, | ||
| useContext, | ||
| useEffect, |
There was a problem hiding this comment.
The refactor completely removes several critical components provided by the previous Radix-UI implementation: DropdownMenuSub, DropdownMenuCheckboxItem, DropdownMenuRadioItem, and DropdownMenuLabel. This is a breaking change that removes essential UI patterns (checkboxes, radio selections, and nested submenus) and should be flagged as a regression rather than a refactor.
| </motion.button> | ||
|
|
||
| </div> | ||
|
|
There was a problem hiding this comment.
The component calls createPortal(..., document.body) directly within the render path. In Server-Side Rendering (SSR) environments (e.g., Next.js), document is not defined on the server, which will cause the application to crash during the initial render. Portal targets should be managed within a useEffect or handled via a client-only mounting check.
| // Use a state variable 'mounted' set to true in useEffect | |
| {mounted && createPortal(<motion.div ... />, document.body)} |
| @@ -93,21 +124,30 @@ | |||
There was a problem hiding this comment.
In lines 125-128, when a line starts with 'deleted file mode ', the code sets 'isIgnored = true'. Because the check 'if (isIgnored) continue;' occurs at line 146 (before hunk processing), all content for deleted files is skipped. This means the resulting FileDiff object will have 'isDeleted: true' but an empty 'hunks' array, losing the actual changes being deleted.
| @@ -93,21 +124,30 @@ export function parseUnifiedDiff(rawDiff: string): FileDiff[] { | |
| Remove 'isIgnored = true;' from the 'deleted file mode' block. Deleted files should still have their hunks parsed to show what was removed. |
| setStyle({ position: 'fixed', top, left, visibility: 'visible' }); | ||
| }; | ||
| update(); | ||
| window.addEventListener('scroll', update, true); |
There was a problem hiding this comment.
The useLayoutEffect adds a window 'scroll' event listener that calls 'setStyle' (a state update) on every single scroll event. Because it uses capture: true, this will trigger a re-render of the DropdownMenuContent component continuously during any page scroll while the menu is open, potentially leading to layout thrashing and decreased FPS.
| window.addEventListener('scroll', update, true); | |
| Consider using a requestAnimationFrame wrapper around the 'update' function or using CSS variables and updating them via ref.current.style.setProperty to avoid React state re-renders. |
| @@ -903,8 +903,7 @@ | |||
| gap: 0.625rem !important; | |||
There was a problem hiding this comment.
The CSS relies heavily on !important for almost every property (e.g., lines 906, 917, 924, 994). This creates extreme specificity that makes the styles nearly impossible to override or extend without further !important declarations, leading to a 'specificity war' and making the codebase difficult to maintain.
| gap: 0.625rem !important; | |
| Remove !important and instead increase selector specificity or reorganize the CSS cascade to ensure styles are applied correctly. |
| {/* Portaled to <body> so the panel always renders above cards, tables, and | ||
| other stacking contexts — it can't be clipped/hidden by an ancestor. */} | ||
| {createPortal( | ||
| <motion.div |
There was a problem hiding this comment.
The component implements role="listbox" and role="option", but lacks the required keyboard interaction patterns (Up/Down arrow keys to navigate options, Enter/Space to select). This is a significant accessibility regression from the previous DropdownMenu implementation, which typically handles these patterns automatically.
| window.removeEventListener('keydown', onKey); | ||
| window.removeEventListener('pointerdown', onPointer); | ||
| }; | ||
| }, [open]); |
There was a problem hiding this comment.
The useLayoutEffect starting at line 100 is missing a dependency array. Consequently, it executes on every single render, creating and disconnecting a new ResizeObserver instance repeatedly. This will cause significant performance overhead, especially when the parent component re-renders frequently.
| }, [open]); | |
| useLayoutEffect(() => { | |
| // ... existing logic | |
| }, []); // Add empty dependency array or specific dependencies |
| setError(null); | ||
| const tid = toast.loading('Saving…'); | ||
| try { | ||
| await api.updateReviewSettings(next); |
There was a problem hiding this comment.
The handleConcurrencyChange and handleCommentsChange functions call persistReviewSettings immediately on every slider value change. Since these are linked to UI sliders, this will trigger a high volume of API requests in rapid succession, potentially overloading the server and causing unstable UI behavior. A debounce mechanism (similar to the one implemented for persistGlobalConfig on lines 385-389) should be used here.
| await api.updateReviewSettings(next); | |
| // Example: Move state update to a debounced effect | |
| useEffect(() => { | |
| if (!reviewSettings || !reviewSettingsDirty) return; | |
| const handle = setTimeout(() => void persistReviewSettings(reviewSettings, 'Settings updated'), 600); | |
| return () => clearTimeout(handle); | |
| }, [reviewSettings, reviewSettingsDirty]); |
| currentFile.path = nextPath.startsWith('b/') ? nextPath.slice(2) : nextPath; | ||
| if (reviewConfig) { | ||
| isIgnored = !isReviewableFile(currentFile.path, customMatchers); | ||
| } |
There was a problem hiding this comment.
The 'isIgnored' flag is used to skip parsing the contents of a file (lines 146-148), but the file object itself is still created and subsequently pushed to the 'files' array via 'pushCurrentFile'. This results in the output containing multiple FileDiff objects with empty hunks, which is inefficient and likely incorrect for the consumer of the function.
| } | |
| Modify 'pushCurrentFile' to check if 'currentFile' is ignored before adding it to the 'files' array. |
…, and optimize stats queries
1 participant
Description
This branch bundles several related improvements to the review pipeline and dashboard UI:
SteppedSliderandConfirmDialogcomponents let users configure job concurrency and max comment limits from the Settings page, with autosave and API routes (/api/settings) backed by a new004_review_performance_settings.sqlmigration. Review logic inreview.tsnow respects these limits.diff.ts/review.tsto support custom file matchers when parsing diffs during review.tabs.tsxfor motion-based tabs, reworkeddropdown-menu.tsxandselect.tsx, and integrated Lenis for smooth scrolling (smooth-scroll.tsx).MetricsGridcomponent, expandedstats.tsto return job statuses, triggers, severities, categories, and performance metrics, and removed unusedrpm/tpm/rpdfields from model configs (005_drop_model_rate_limits.sql), including matching schema, route, and validation cleanup.Closes #22 #23
Type of change
How Has This Been Tested?
test/api.spec.ts,test/review-flow.spec.tsupdated for new data structures)Checklist: