feat(experiments): add group default metric sort, applied client-side#578
Merged
Conversation
Signed-off-by: shanaiabuggy <[email protected]>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExperiment group sorting now uses a single sort string end to end. OpenAPI, backend validation, tests, and Studio UI switched from structured criteria to string values, and omitted experiment-list sorting now defaults to ChangesExperiment group sort string migration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (2)
web/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsx (1)
45-63: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize
seedSortFromDefaultresult.
seedSortFromDefault(group.default_metric_sort)returns a fresh object literal every render, even whendefault_metric_sorthasn't changed.useStudioDataViewStateusesdefaultSortinside auseMemo([sortParam, defaultSort])(seeweb/packages/common/src/hooks/useStudioDataViewState/index.ts) and indefaultSortString; if downstream code treatssortingas anything other than a one-time initial value, an unstable reference here could cause unwanted re-syncs of the sort state on every render (defeating the "initialized once" comment at lines 83-85).♻️ Stabilize with useMemo
- const dataViewState = useStudioDataViewState<ExperimentFilter>({ - // Seed the sort from default_metric_sort so its column header reflects the order on load. - defaultSort: seedSortFromDefault(group.default_metric_sort), + const seededSort = useMemo( + () => seedSortFromDefault(group.default_metric_sort), + [group.default_metric_sort] + ); + const dataViewState = useStudioDataViewState<ExperimentFilter>({ + // Seed the sort from default_metric_sort so its column header reflects the order on load. + defaultSort: seededSort,Also applies to: 99-108
🤖 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/components/dataViews/ExperimentGroupDataView/index.tsx` around lines 45 - 63, `seedSortFromDefault` currently returns a new sort object each render, which makes the `defaultSort` value passed from `ExperimentGroupDataView` unstable. Memoize the computed result where `group.default_metric_sort` is converted into `defaultSort` (or wrap the call in `useMemo`) so the same object reference is reused until `default_metric_sort` changes; keep the existing `seedSortFromDefault` logic and ensure the `useStudioDataViewState` consumers see a stable `defaultSort`.web/packages/studio/src/components/DefaultSortControl/util.ts (1)
17-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing explicit return types on exported helpers.
evaluatorField,isEvaluatorField,evaluatorNameOfare public exports but lack explicit return type annotations, unlikeparseSortString/formatSortStringbelow them.As per coding guidelines, "Use explicit return types for public APIs and complex functions in TypeScript".Proposed fix
-export const evaluatorField = (name: string) => `evaluators.${name}.mean`; -export const isEvaluatorField = (field: string) => EVALUATOR_FIELD.test(field); -export const evaluatorNameOf = (field: string) => field.match(EVALUATOR_FIELD)?.[1] ?? ''; +export const evaluatorField = (name: string): string => `evaluators.${name}.mean`; +export const isEvaluatorField = (field: string): boolean => EVALUATOR_FIELD.test(field); +export const evaluatorNameOf = (field: string): string => field.match(EVALUATOR_FIELD)?.[1] ?? '';🤖 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/components/DefaultSortControl/util.ts` around lines 17 - 20, Add explicit return type annotations to the exported helper functions in evaluator utilities: update evaluatorField, isEvaluatorField, and evaluatorNameOf in util.ts to declare their return types, matching the style used by parseSortString and formatSortString. Keep the existing behavior unchanged; just make the public API signatures explicit so these helpers are consistently typed.Source: Coding guidelines
🤖 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/studio/src/components/DefaultSortControl/util.ts`:
- Around line 5-20: Preserve non-.mean sort fields in DefaultSortControl by
updating the field helpers in util.ts so they don’t assume every non-static
value is an evaluator mean. Adjust evaluatorField, isEvaluatorField, and
evaluatorNameOf to support arbitrary rollup suffixes or add a raw-field
fallback, and make sure selectValueFor and labelForOption can round-trip values
like cost_usd.p95 and evaluators.foo.p95 without collapsing them to evaluator:
or rendering them as Avg.
In `@web/packages/studio/src/components/ExperimentGroupCreateModal/index.tsx`:
- Around line 64-66: The comment above defaultSort is stale and still refers to
the old SortCriterion[] array-of-objects implementation. Update the inline
comment near the defaultSort state in ExperimentGroupCreateModal to accurately
describe the current single-string control and its handling outside
react-hook-form, keeping the note aligned with how handleSubmit merges the value
into the payload.
In `@web/packages/studio/src/routes/ExperimentGroupDetailRoute/index.tsx`:
- Around line 18-22: The experiment-group fetch result is only reading data from
useGetExperimentGroup, so failures are left unhandled and the route can
half-render without feedback. Update ExperimentGroupDetailRoute to also read the
hook’s error state and render an appropriate error/empty state when the fetch
fails, instead of continuing as if data is available. Apply the same error
handling pattern in ExperimentGroupMetrics, since it makes the same uncaught
request.
---
Nitpick comments:
In
`@web/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsx`:
- Around line 45-63: `seedSortFromDefault` currently returns a new sort object
each render, which makes the `defaultSort` value passed from
`ExperimentGroupDataView` unstable. Memoize the computed result where
`group.default_metric_sort` is converted into `defaultSort` (or wrap the call in
`useMemo`) so the same object reference is reused until `default_metric_sort`
changes; keep the existing `seedSortFromDefault` logic and ensure the
`useStudioDataViewState` consumers see a stable `defaultSort`.
In `@web/packages/studio/src/components/DefaultSortControl/util.ts`:
- Around line 17-20: Add explicit return type annotations to the exported helper
functions in evaluator utilities: update evaluatorField, isEvaluatorField, and
evaluatorNameOf in util.ts to declare their return types, matching the style
used by parseSortString and formatSortString. Keep the existing behavior
unchanged; just make the public API signatures explicit so these helpers are
consistently typed.
🪄 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: b667b79e-8e55-43c9-be29-97602f1173e2
⛔ Files ignored due to path filters (14)
sdk/python/nemo-platform/.nmpcontext/openapi.yamlis excluded by!sdk/**sdk/python/nemo-platform/.nmpcontext/stainless.yamlis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiment_groups/api.mdis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiment_groups/experiment_groups.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/resources/experiments/experiments.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/experiment_group_create_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/experiment_group_response.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/experiment_group_update_params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/sort_criterion.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiment_groups/sort_criterion_param.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/types/experiments/experiment_list_params.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/api_resources/test_experiment_groups.pyis excluded by!sdk/**sdk/stainless.yamlis excluded by!sdk/**
📒 Files selected for processing (15)
openapi/ga/individual/platform.openapi.yamlopenapi/ga/openapi.yamlopenapi/openapi.yamlservices/intake/src/nmp/intake/api/v2/experiments/endpoints.pyservices/intake/src/nmp/intake/api/v2/experiments/schemas.pyservices/intake/src/nmp/intake/entities/experiments.pyservices/intake/tests/test_experiment_default_sort.pyweb/packages/studio/src/components/DefaultSortControl/index.tsxweb/packages/studio/src/components/DefaultSortControl/util.test.tsweb/packages/studio/src/components/DefaultSortControl/util.tsweb/packages/studio/src/components/ExperimentGroupCreateModal/index.tsxweb/packages/studio/src/components/ExperimentGroupEditModal/index.tsxweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsxweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/useExperimentGroupExperiments.tsweb/packages/studio/src/routes/ExperimentGroupDetailRoute/index.tsx
Signed-off-by: shanaiabuggy <[email protected]> # Conflicts: # web/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsx
Contributor
|
Signed-off-by: shanaiabuggy <[email protected]>
Signed-off-by: shanaiabuggy <[email protected]>
shanaiabuggy
commented
Jul 6, 2026
Signed-off-by: shanaiabuggy <[email protected]>
shanaiabuggy
commented
Jul 6, 2026
Signed-off-by: shanaiabuggy <[email protected]> # Conflicts: # web/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsx
BrianNewsom
approved these changes
Jul 6, 2026
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
…#578) * feat(experiments): add group default metric sort, applied client-side Signed-off-by: shanaiabuggy <[email protected]> * lint Signed-off-by: shanaiabuggy <[email protected]> * lint Signed-off-by: shanaiabuggy <[email protected]> * default_metric_sort -> default_sort Signed-off-by: shanaiabuggy <[email protected]> --------- Signed-off-by: shanaiabuggy <[email protected]>
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
3 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.
Screen.Recording.2026-07-06.at.2.33.18.PM.mov
Summary by CodeRabbit
Summary by CodeRabbit
sortis omitted, ordering is now consistently pinned first, then newest (-created_at).-created_at.default_sortas a string (and removed the old structured sort criterion schema).