refactor(studio): replace deprecated ReadOnlyField with KVPair (ASTD-227)#366
refactor(studio): replace deprecated ReadOnlyField with KVPair (ASTD-227)#366aray12 wants to merge 2 commits into
Conversation
…227) Migrate all usages of the deprecated ReadOnlyField component to KVPair from @nemo/common and delete the deprecated file. Signed-off-by: Alex Ray <alray@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)
📝 WalkthroughWalkthroughRemoves the local ChangesReadOnlyField → KVPair Migration
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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/components/evaluation/Configurations/ConfigurationDetailsPanel.tsx (1)
44-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove legacy
'-'fallback before passing values toKVPair.Pre-filling
config.name/config.idwith'-'bypassesKVPairmissing-value handling and keeps inconsistent placeholder rendering. Pass raw values and letKVPairapply itsdefaultValue.Suggested diff
- const configName = config.name || '-'; - const configId = config.id || '-'; + const configName = config.name; + const configId = config.id;Based on the KVPair fallback contract in
web/packages/common/src/components/KVPair/index.tsxand the PR’s fallback-handling objective.🤖 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/evaluation/Configurations/ConfigurationDetailsPanel.tsx` around lines 44 - 45, Remove the legacy `'-'` fallback values from the configName and configId variable assignments in ConfigurationDetailsPanel.tsx. Instead of pre-filling these variables with `'-'` when config.name or config.id are missing, pass the raw values directly (even if undefined) to the KVPair component. This allows KVPair to apply its own defaultValue handling logic consistently, rather than bypassing it with a hardcoded placeholder string. Update lines where configName and configId are assigned to use config.name and config.id directly without the `||` fallback operator.
🤖 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/evaluation/Configurations/MetricDisplay.tsx`:
- Around line 34-41: Replace truthy checks with nullish checks for metric
parameter values to avoid suppressing valid falsy values like 0 or false. In the
KVPair components throughout MetricDisplay.tsx (including the check field around
line 34 and reference, ground-truth, prediction fields around lines 48-90),
change the conditional check from a truthy check (params.check ?) to a nullish
check (params.check != null) or similar pattern that only checks for null or
undefined while allowing 0, false, empty strings, and other falsy but valid
values to be rendered properly.
---
Outside diff comments:
In
`@web/packages/studio/src/components/evaluation/Configurations/ConfigurationDetailsPanel.tsx`:
- Around line 44-45: Remove the legacy `'-'` fallback values from the configName
and configId variable assignments in ConfigurationDetailsPanel.tsx. Instead of
pre-filling these variables with `'-'` when config.name or config.id are
missing, pass the raw values directly (even if undefined) to the KVPair
component. This allows KVPair to apply its own defaultValue handling logic
consistently, rather than bypassing it with a hardcoded placeholder string.
Update lines where configName and configId are assigned to use config.name and
config.id directly without the `||` fallback operator.
🪄 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: 6fd5cbca-2a32-420d-9c7c-bb181334f705
📒 Files selected for processing (5)
web/packages/studio/src/components/common/ReadOnlyField.tsxweb/packages/studio/src/components/evaluation/Configurations/ConfigurationDetailsPanel.tsxweb/packages/studio/src/components/evaluation/Configurations/LLMJudgeDisplay.tsxweb/packages/studio/src/components/evaluation/Configurations/MetricDisplay.tsxweb/packages/studio/src/components/evaluation/Configurations/TaskDisplay.tsx
💤 Files with no reviewable changes (1)
- web/packages/studio/src/components/common/ReadOnlyField.tsx
|
…alues (ASTD-227) Signed-off-by: Alex Ray <alray@nvidia.com>
Summary
ReadOnlyFieldcomponent inevaluation/Configurations/toKVPairfrom@nemo/commoncomponents/common/ReadOnlyField.tsx'-'fallback strings that are now handled natively byKVPair'sdefaultValuepropCloses ASTD-227
Test plan
pnpm --filter nemo-studio-ui typecheck)Summary by CodeRabbit