feat: add readonly batch insights TUI - #2078
Conversation
github-actions
Bot
added
agentcore-harness-reviewing
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2078 +/- ##
===========================================
Coverage ? 97.41%
===========================================
Files ? 407
Lines ? 24407
Branches ? 0
===========================================
Hits ? 23776
Misses ? 631
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| @@ -41,6 +41,11 @@ function toRow(summary: BatchEvaluationSummary): BatchEvaluationRow { | |||
| export interface BatchEvaluationPickerProps extends ScreenProps { | |||
There was a problem hiding this comment.
Can you explain why we are reusing BatchEvaluationPicker constructs for BatchInsights. They are two different resource that will diverge in the future.
There was a problem hiding this comment.
The CLI experience and semantics are separate, but they aren't two different resources. Batch evals and insights have the same listBatchEvaluations API, response model, and display fields. Insights is just a filtered projection of BatchEvaluations, and has its own screen, routes, validations, etc.
If the resources diverge to separate APIs in the future, we'll need a significant refactor to the insights logic regardless of them sharing resources or not. Separating this constructs now creates more code to maintain upfront, and does not save us any effort in the future.
There was a problem hiding this comment.
I remember Swarmim said they were two different resources and I think we should seperate them but I'm fine just rename all shared compoenent like BatchEvalautionPicker to BatchEvaluation/InsightPicker so we know that its used for both.
There was a problem hiding this comment.
Our CLI experience does match the idea that they are two different resources to the customer/product side (screens, commands, routes, etc.). When I say they're the same resource I mean on the service/API abstraction side.
The name BatchEvaluationPicker is accurate to the service resource it loads. It requests and renders all batch evaluations, which includes insights. I feel like renaming it to include insights implies that the picker operates on two service resources, when it doesn't. The picker does exactly what it is named for: gives all batch insights.
src/handlers/eval/batch-insights/list/screen.tsx has to filter out insights from the full list of batch evaluations. So that logic is distinct to the insights flow. I think the idea that insights is getting batch evals, and filtering down is accurate with whats really happening. Naming it otherwise would be misleading to whats actually happening
| breadcrumb={breadcrumb} | ||
| description={description} | ||
| queryKey={["batch-evaluations", opts.region]} | ||
| queryKey={[queryKeyPrefix, opts.region]} |
|
This is great! Approved. |
b147dc9 to
9c8d0fb
Compare
9c8d0fb to
a25a329
Compare
jariy17
left a comment
jariy17
left a comment
There was a problem hiding this comment.
Nico and I have discussed. We have decided to have dedicated BatchInsights function in evalClient. And rename BatchEvaluationPicker to BatchEvaluationAndInsightsPicker.
4 participants
Description
Adds an interactive, read-only TUI for the
eval batch-insightscommand tree.getandlistcommands now expose TUI flows; run remains CLI only.Screens
eval batch-insightsRouterScreenmenueval batch-insights list/eval batch-insights getBatchEvaluationPickerusingPaginatedTablePickereval batch-insights get/:batchEvaluationIdJsonDetailType of Change
Testing
Added
batch-insights.screen.test.tsxcovering screen behaviorbun run test(1757 pass, 0 fail)Checklist
Stack created with GitHub Stacks CLI • Give Feedback 💬