feat(studio): agent-eval logs ui, add collapsible panel - #611
Conversation
Signed-off-by: Octavian Drulea <odrulea@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new AccordionPanel, threads job status into log polling, adds post-terminal settle refetches, and renders a Logs accordion in the agent evaluation detail route. ChangesLogs Accordion and Job Status Refetch
Sequence Diagram(s)sequenceDiagram
participant Route as AgentEvaluationDetailRoute
participant Logs as StatusLogsContent
participant Hook as useJobLogs
participant API as Log API
Route->>Logs: render(workspace, jobName, job.status)
Logs->>Hook: useJobLogs(workspace, name, jobStatus)
Hook->>API: refetch() on interval
API-->>Hook: log lines
Note over Hook: jobStatus becomes terminal
loop LOG_SETTLE_DELAYS_MS
Hook->>API: scheduled refetch()
API-->>Hook: late log lines
end
Hook-->>Logs: updated logs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/packages/studio/src/routes/agents/AgentEvaluationsRoute/AgentEvaluationDetailRoute.tsx (1)
47-58: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign agent-eval logs with the shared terminal set
AgentEvalJob.statusis a plain string, but this route treatssucceeded/success/failedas terminal whileuseJobLogsonly stops oncompleted/error/cancelled. Thejob.status as PlatformJobStatuscast lets those aliases keep polling forever.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/routes/agents/AgentEvaluationsRoute/AgentEvaluationDetailRoute.tsx` around lines 47 - 58, The terminal-status check in AgentEvaluationDetailRoute is using a broader local set than the shared log polling logic, so alias statuses like succeeded/success/failed can keep polling forever. Update the status handling in AgentEvaluationDetailRoute (including isTerminal and the AgentEvalJob.status cast) to use the same shared terminal set as useJobLogs, or normalize aliases to the canonical PlatformJobStatus values before passing status through. Make sure the route stops polling for all terminal agent-eval states via the shared status helper instead of relying on a separate local list.
🧹 Nitpick comments (1)
web/packages/common/src/components/AccordionPanel/index.tsx (1)
60-75: 📐 Maintainability & Code Quality | 🔵 TrivialMissing aria-controls linkage between header and content.
Header toggles content via
role="button"but there's noid/aria-controlspairing so assistive tech can associate them.
[optional_low_effort]♿ Add id/aria-controls
+ const contentId = useId(); return ( <PanelRoot elevation={elevation} density={density} className={className}> <PanelHeader role="button" tabIndex={0} aria-expanded={open} + aria-controls={contentId} onClick={toggle} onKeyDown={onKeyDown} className="cursor-pointer" > {slotIcon && <PanelIcon>{slotIcon}</PanelIcon>} <PanelHeading>{slotHeading}</PanelHeading> <PanelIcon className="ml-auto">{open ? <ChevronUp /> : <ChevronDown />}</PanelIcon> </PanelHeader> - {open && <PanelContent className={contentClassName}>{children}</PanelContent>} + {open && <PanelContent id={contentId} className={contentClassName}>{children}</PanelContent>}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/components/AccordionPanel/index.tsx` around lines 60 - 75, The AccordionPanel header is missing the accessibility linkage to its content. Update the AccordionPanel component so the PanelHeader in AccordionPanel/index.tsx gets a stable id and an aria-controls value that points to the PanelContent, and give PanelContent the matching id; keep the existing toggle behavior in sync with the open state so assistive tech can associate the two.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/packages/common/src/hooks/useJobLogs/index.ts`:
- Around line 113-119: The settle burst in useJobLogs is re-triggering on every
remount when the job is already terminal, because the effect only tracks local
hook state. Update useJobLogs to persist whether the burst has already run for a
given job outside the hook instance (for example via a job-scoped cache or
shared state keyed by job identity), and gate the existing isTerminal/refetch
timeout scheduling so the LOG_SETTLE_DELAYS_MS burst only happens once per job.
---
Outside diff comments:
In
`@web/packages/studio/src/routes/agents/AgentEvaluationsRoute/AgentEvaluationDetailRoute.tsx`:
- Around line 47-58: The terminal-status check in AgentEvaluationDetailRoute is
using a broader local set than the shared log polling logic, so alias statuses
like succeeded/success/failed can keep polling forever. Update the status
handling in AgentEvaluationDetailRoute (including isTerminal and the
AgentEvalJob.status cast) to use the same shared terminal set as useJobLogs, or
normalize aliases to the canonical PlatformJobStatus values before passing
status through. Make sure the route stops polling for all terminal agent-eval
states via the shared status helper instead of relying on a separate local list.
---
Nitpick comments:
In `@web/packages/common/src/components/AccordionPanel/index.tsx`:
- Around line 60-75: The AccordionPanel header is missing the accessibility
linkage to its content. Update the AccordionPanel component so the PanelHeader
in AccordionPanel/index.tsx gets a stable id and an aria-controls value that
points to the PanelContent, and give PanelContent the matching id; keep the
existing toggle behavior in sync with the open state so assistive tech can
associate the two.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3bd19712-cf69-430a-b43d-906d264412a3
📒 Files selected for processing (4)
web/packages/common/src/components/AccordionPanel/index.tsxweb/packages/common/src/hooks/useJobLogs/index.tsweb/packages/studio/src/components/evaluation/Jobs/StatusLogsContent.tsxweb/packages/studio/src/routes/agents/AgentEvaluationsRoute/AgentEvaluationDetailRoute.tsx
|
Signed-off-by: Octavian Drulea <odrulea@nvidia.com>
Summary by CodeRabbit