Open
Conversation
- Improve UX/UI in create monitor and graph views
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds duplicate-name validation to monitor create/edit flows, updates form labels and select sentinel handling, reverses build log order, refactors several monitor UI buttons/layouts, and enhances chart X-axis formatting and tooltip payload handling across eval workspace components. Changes
Sequence DiagramsequenceDiagram
participant User
participant CreateEditComp as Create/Edit Component
participant ListHook as useListMonitors
participant API
participant FormWizard as MonitorFormWizard
User->>CreateEditComp: Open create/edit monitor UI
CreateEditComp->>ListHook: invoke useListMonitors(org,proj,agent)
ListHook->>API: GET /monitors
API-->>ListHook: monitors data
ListHook-->>CreateEditComp: monitorsData
CreateEditComp->>CreateEditComp: derive existingMonitorNames
CreateEditComp->>FormWizard: render with existingMonitorNames & editingMonitorName
User->>FormWizard: type displayName
FormWizard->>FormWizard: check name against existingMonitorNames (exclude editingMonitorName)
alt name duplicate
FormWizard-->>User: show "A monitor with this name already exists" error
else unique
FormWizard-->>User: allow progression
end
User->>FormWizard: submit
FormWizard->>FormWizard: duplicate check on submit
alt duplicate
FormWizard-->>User: set field error, reset to page 1
else valid
FormWizard->>API: POST/PUT monitor
API-->>FormWizard: success
FormWizard-->>User: saved confirmation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
console/workspaces/pages/eval/src/subComponents/MonitorStartStopButton.tsx (1)
99-102: Dead code remains intooltipTitlecalculation.Since
isPastMonitornow causes an early return ofnull, the branch handlingisPastMonitorin thetooltipTitleternary (lines 60-61) is never reached and can be removed.♻️ Suggested cleanup
Simplify
tooltipTitleand the early return:- const tooltipTitle = isActive - ? "Stop Monitor" - : isPastMonitor - ? "Past monitors cannot be started" - : "Start Monitor"; + const tooltipTitle = isActive ? "Stop Monitor" : "Start Monitor";if (isPastMonitor) { - return ( - null - ); + return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/eval/src/subComponents/MonitorStartStopButton.tsx` around lines 99 - 102, The tooltipTitle calculation contains a dead branch for isPastMonitor because MonitorStartStopButton now returns null early when isPastMonitor is true; remove the isPastMonitor case from the tooltipTitle ternary and simplify tooltipTitle accordingly, and keep the existing early return (return null) guarded by isPastMonitor in the MonitorStartStopButton component so the tooltip logic only handles the remaining states.console/workspaces/pages/eval/src/subComponents/PerformanceByEvaluatorCard.tsx (2)
123-127: Sort the merged timestamps by epoch, not raw string.This now depends on
p.timestampbeing lexicographically sortable. Since the chart renders onxTimestamp, sorting withnew Date(...).getTime()will keep the series chronological even if the API returns offsets or another valid timestamp string shape.Suggested change
const allTimestamps = Array.from( new Set( Object.values(seriesMap).flatMap((pts) => pts.map((p) => p.timestamp)), ), - ).sort(); + ).sort((a, b) => new Date(a).getTime() - new Date(b).getTime());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/eval/src/subComponents/PerformanceByEvaluatorCard.tsx` around lines 123 - 127, The current allTimestamps aggregation uses default string sort which assumes lexicographic timestamp order; update the deduped timestamps built from seriesMap (from p.timestamp) so they are sorted by epoch milliseconds instead: when creating allTimestamps (used to drive xTimestamp), convert each p.timestamp to a Date and sort by date.getTime() to ensure chronological order regardless of timestamp string format, then map back to the original string/ISO form expected by the chart.
271-274: Replace the hardcoded tick font size with a theme token.
fontSize: 12can drift from the console typography scale. Reusetheme.typography.caption.fontSizehere instead.As per coding guidelines, `console/**/*.{ts,tsx,js,jsx}` should "Use theme tokens via the `sx` prop instead of hardcoded colors and spacing values".Suggested change
tick={{ - fontSize: 12, + fontSize: theme.typography.caption.fontSize, }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/workspaces/pages/eval/src/subComponents/PerformanceByEvaluatorCard.tsx` around lines 271 - 274, Replace the hardcoded tick font size in PerformanceByEvaluatorCard by reading the theme token instead of using 12; import and call useTheme() (from `@mui/material/styles`) inside PerformanceByEvaluatorCard and replace tick={{ fontSize: 12 }} with tick={{ fontSize: theme.typography.caption.fontSize }} (or apply the value via an sx-based style if your chart wrapper supports sx), referencing the tick prop in the component to ensure the console typography token is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@console/workspaces/libs/shared-component/src/components/BuildLogs.tsx`:
- Line 81: In BuildLogs.tsx the line creating reversedBuildLogs uses
buildLogs.reverse(), which mutates the React Query cached array from
useGetBuildLogs/getBuildLogs; replace this with a non-mutating reversal (e.g.,
create a copy via spread or slice and then reverse, or use toReversed() if
targeting ES2023+) so reversedBuildLogs is a new array and the cache isn't
mutated.
In `@console/workspaces/pages/eval/src/subComponents/MonitorFormWizard.tsx`:
- Around line 108-120: The duplicate-check is comparing next.name
(slug/identifier) against existingMonitorNames (MonitorResponse.name) instead of
comparing the user-facing displayName; update the logic in MonitorFormWizard to
compare normalized displayName values end-to-end: derive namesToCheck from
existing monitors' displayName (e.g., existingMonitorDisplayNames), exclude the
current monitor by monitorData?.displayName or editingDisplayName (instead of
editingMonitorName), and test next.displayName.trim().length and normalized
next.displayName against namesToCheck; apply the same change to the second
occurrence (lines ~212-223) and ensure callers provide m.displayName and
monitorData?.displayName where relevant so the component gets displayName values
to compare.
---
Nitpick comments:
In `@console/workspaces/pages/eval/src/subComponents/MonitorStartStopButton.tsx`:
- Around line 99-102: The tooltipTitle calculation contains a dead branch for
isPastMonitor because MonitorStartStopButton now returns null early when
isPastMonitor is true; remove the isPastMonitor case from the tooltipTitle
ternary and simplify tooltipTitle accordingly, and keep the existing early
return (return null) guarded by isPastMonitor in the MonitorStartStopButton
component so the tooltip logic only handles the remaining states.
In
`@console/workspaces/pages/eval/src/subComponents/PerformanceByEvaluatorCard.tsx`:
- Around line 123-127: The current allTimestamps aggregation uses default string
sort which assumes lexicographic timestamp order; update the deduped timestamps
built from seriesMap (from p.timestamp) so they are sorted by epoch milliseconds
instead: when creating allTimestamps (used to drive xTimestamp), convert each
p.timestamp to a Date and sort by date.getTime() to ensure chronological order
regardless of timestamp string format, then map back to the original string/ISO
form expected by the chart.
- Around line 271-274: Replace the hardcoded tick font size in
PerformanceByEvaluatorCard by reading the theme token instead of using 12;
import and call useTheme() (from `@mui/material/styles`) inside
PerformanceByEvaluatorCard and replace tick={{ fontSize: 12 }} with tick={{
fontSize: theme.typography.caption.fontSize }} (or apply the value via an
sx-based style if your chart wrapper supports sx), referencing the tick prop in
the component to ensure the console typography token is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e13f046-5e86-4bfe-a4ca-95574e47cb8c
📒 Files selected for processing (13)
console/workspaces/libs/shared-component/src/components/BuildLogs.tsxconsole/workspaces/pages/eval/src/CreateMonitor.Component.tsxconsole/workspaces/pages/eval/src/EditMonitor.Component.tsxconsole/workspaces/pages/eval/src/EvalMonitors.Component.tsxconsole/workspaces/pages/eval/src/form/schema.tsconsole/workspaces/pages/eval/src/subComponents/CreateMonitorForm.tsxconsole/workspaces/pages/eval/src/subComponents/EvaluatorLlmProviderSection.tsxconsole/workspaces/pages/eval/src/subComponents/MetricsTooltip.tsxconsole/workspaces/pages/eval/src/subComponents/MonitorFormWizard.tsxconsole/workspaces/pages/eval/src/subComponents/MonitorRunList.tsxconsole/workspaces/pages/eval/src/subComponents/MonitorStartStopButton.tsxconsole/workspaces/pages/eval/src/subComponents/MonitorTable.tsxconsole/workspaces/pages/eval/src/subComponents/PerformanceByEvaluatorCard.tsx
console/workspaces/libs/shared-component/src/components/BuildLogs.tsx
Outdated
Show resolved
Hide resolved
nadheesh
approved these changes
Mar 11, 2026
nadheesh
reviewed
Mar 11, 2026
| "displayName", | ||
| "A monitor with this name already exists.", | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
The duplicate-check here compares next.name (the auto-generated slug) against existingMonitorNames (also slugs from MonitorResponse.name). But the field being validated is displayName, not the slug.
This means:
- A
displayNamerename that happens to produce a colliding slug will be incorrectly blocked - Two monitors with identical
displayNamebut different slugs will pass validation (false negative)
Should compare normalized displayName values end-to-end instead. Callers (CreateMonitor.Component.tsx, EditMonitor.Component.tsx) should pass m.displayName and monitorData?.displayName.
Same issue applies to the submit-time check at lines ~212-223.
nadheesh
reviewed
Mar 11, 2026
| </IconButton> | ||
| </span> | ||
| </Tooltip> | ||
| null |
Contributor
There was a problem hiding this comment.
Nit: Since isPastMonitor now triggers an early return null, the isPastMonitor branch in the tooltipTitle ternary (around line 60) is unreachable dead code. Should simplify to:
const tooltipTitle = isActive ? "Stop Monitor" : "Start Monitor";Also, the return ( null ); can be simplified to just return null;.
nadheesh
reviewed
Mar 11, 2026
| @@ -125,16 +126,16 @@ const PerformanceByEvaluatorCard: React.FC<PerformanceByEvaluatorCardProps> = ({ | |||
| ), | |||
| ).sort(); | |||
Contributor
There was a problem hiding this comment.
Nit: Default .sort() uses string comparison which is fragile for timestamps. Since the chart now renders on xTimestamp (epoch), sort by epoch to be safe:
).sort((a, b) => new Date(a).getTime() - new Date(b).getTime());
nadheesh
reviewed
Mar 11, 2026
| tickFormatter={formatTickTimestamp} | ||
|
|
||
| tick={{ | ||
| fontSize: 12, |
Contributor
There was a problem hiding this comment.
Nit: Hardcoded fontSize: 12 should use a theme token per project guidelines:
tick={{
fontSize: theme.typography.caption.fontSize,
}}useTheme() is already imported in this component.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces several improvements and fixes across the monitor creation and evaluation UI, focusing on validation, user experience, and consistency. The most important changes are the addition of duplicate monitor name validation, updates to form labels and error messages, enhancements to tooltip and chart labeling, and UI refinements for action buttons and layout.
Monitor Name Validation & Form Improvements:
CreateMonitor.Component.tsx,EditMonitor.Component.tsx,MonitorFormWizard.tsx) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]schema.ts,CreateMonitorForm.tsx) [1] [2] [3] [4]Chart & Tooltip Enhancements:
MetricsTooltip.tsx,PerformanceByEvaluatorCard.tsx) [1] [2] [3] [4]UI & Layout Refinements:
EvalMonitors.Component.tsx,MonitorTable.tsx,MonitorRunList.tsx) [1] [2] [3] [4] [5]MonitorStartStopButton.tsx) [1] [2]Build Logs Display Fix:
BuildLogs.tsx) [1] [2]These changes collectively improve validation, usability, and visual consistency throughout the monitor management and evaluation interfaces.
Purpose
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Bug Fixes
UI Improvements