feat(studio): add model palette and config panel to DAG build route - #546
Conversation
|
This change is part of the following stack:
Change managed by git-spice. |
9543e73 to
aa19740
Compare
f38aab5 to
c5c9078
Compare
aa19740 to
e9346e6
Compare
c5c9078 to
20b2d36
Compare
e9346e6 to
63b8022
Compare
20b2d36 to
b5c80de
Compare
63b8022 to
baed57c
Compare
b5c80de to
a011098
Compare
baed57c to
9f24420
Compare
a011098 to
c757189
Compare
c757189 to
b417303
Compare
|
|
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:
📝 WalkthroughWalkthroughThe PR adds model-aware templates, sampler-specific configuration, centralized builder state, job submission wiring, and new palette, configuration, details, and toolbar components. ChangesData Designer builder
Sequence Diagram(s)sequenceDiagram
participant DataDesignerJobBuildRoute
participant useAllModels
participant useJobBuilder
participant useDataDesignerCreateJob
DataDesignerJobBuildRoute->>useAllModels: load workspace models
useAllModels->>useJobBuilder: provide grouped model data
useJobBuilder-->>DataDesignerJobBuildRoute: return builder state and handlers
DataDesignerJobBuildRoute->>useDataDesignerCreateJob: submit validated config
useDataDesignerCreateJob-->>DataDesignerJobBuildRoute: return job result or error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
web/packages/studio/src/components/AddModelPalette/index.tsx (1)
16-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark props readonly.
AddModelPalettePropsfields aren't mutated internally; marking themreadonlyenforces that contract at the type level.As per coding guidelines, "Use
readonlyfor immutable properties in TypeScript interfaces and types."🤖 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/AddModelPalette/index.tsx` around lines 16 - 33, Mark every field in the AddModelPaletteProps interface as readonly, including models, selectedId, modelGroups, isLoadingModels, onAddModel, onSelectModel, and className, without changing their types or behavior.Source: Coding guidelines
web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderToolbar.tsx (3)
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
Number(rows)call.Computed twice inline; extract once.
🤖 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/DataDesignerJobBuildRoute/BuilderToolbar.tsx` at line 45, In the BuilderToolbar preview-row calculation, avoid calling Number(rows) twice. Extract the numeric value into a local variable, then use it for both the positivity check and Math.min while preserving the existing fallback and maximum of 10.
44-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename input has no Enter/Escape handling.
Only
onBlurexits edit mode; pressing Enter should confirm, Escape should cancel. Minor UX gap, not blocking.🤖 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/DataDesignerJobBuildRoute/BuilderToolbar.tsx` around lines 44 - 68, Add Enter and Escape keyboard handling to the editing input in BuilderToolbar’s isEditingName branch: Enter should confirm and exit edit mode, while Escape should cancel the edit by restoring the original name and exiting edit mode. Preserve the existing onBlur behavior and use the component’s name-change state/handlers to implement cancellation.
10-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark props readonly.
Same as
AddModelPaletteProps— these fields are never reassigned within the component.As per coding guidelines, "Use
readonlyfor immutable properties in TypeScript interfaces and types."🤖 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/DataDesignerJobBuildRoute/BuilderToolbar.tsx` around lines 10 - 25, Mark every property in the BuilderToolbarProps interface as readonly, matching the convention used by AddModelPaletteProps; retain the existing types and callback signatures unchanged.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/routes/DataDesignerJobBuildRoute/BuilderToolbar.tsx`:
- Line 83: Add aria-hidden="true" to the decorative separator Text element in
BuilderToolbar so screen readers skip the non-semantic “·” character.
In `@web/packages/studio/src/routes/DataDesignerJobBuildRoute/columns.ts`:
- Around line 220-241: Add builder-editable parameter definitions to
PARAM_FIELDS_BY_SAMPLER_TYPE for every sampler subtype exposed by
COLUMN_TYPE_GROUPS, especially binomial with required fields n and p using
appropriate numeric ColumnField configurations. Ensure getSamplerParamFields
supplies these fields so toSamplerConfig serializes valid params instead of an
empty object.
In `@web/packages/studio/src/routes/DataDesignerJobBuildRoute/index.tsx`:
- Around line 88-141: Validate the job name in validateAndCollectErrors before
submission: trim name and add an error when it is empty or whitespace-only, so
handleSubmit cannot call createJob.mutateAsync with an invalid name. Reuse this
validation for both preview and submit flows, and ensure the existing validation
error display reflects the new message.
In `@web/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.ts`:
- Around line 67-86: Gate the auto-fill effect in useJobBuilder on complete
model loading, not merely modelGroups.length > 0. Thread the useAllModels
pagination state into the hook as needed and require !hasNextPage and
!isFetchingNextPage before setting autoFilled.current or resolving models, so
later pages can provide the preferred model.
---
Nitpick comments:
In `@web/packages/studio/src/components/AddModelPalette/index.tsx`:
- Around line 16-33: Mark every field in the AddModelPaletteProps interface as
readonly, including models, selectedId, modelGroups, isLoadingModels,
onAddModel, onSelectModel, and className, without changing their types or
behavior.
In `@web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderToolbar.tsx`:
- Line 45: In the BuilderToolbar preview-row calculation, avoid calling
Number(rows) twice. Extract the numeric value into a local variable, then use it
for both the positivity check and Math.min while preserving the existing
fallback and maximum of 10.
- Around line 44-68: Add Enter and Escape keyboard handling to the editing input
in BuilderToolbar’s isEditingName branch: Enter should confirm and exit edit
mode, while Escape should cancel the edit by restoring the original name and
exiting edit mode. Preserve the existing onBlur behavior and use the component’s
name-change state/handlers to implement cancellation.
- Around line 10-25: Mark every property in the BuilderToolbarProps interface as
readonly, matching the convention used by AddModelPaletteProps; retain the
existing types and callback signatures unchanged.
🪄 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: cea0aa07-9155-46b2-9cdf-37bb8c6bb7d2
📒 Files selected for processing (16)
web/packages/studio/src/components/AddModelPalette/AddModelPalette.stories.tsxweb/packages/studio/src/components/AddModelPalette/index.tsxweb/packages/studio/src/components/ColumnConfigPanel/ColumnConfigPanel.tsxweb/packages/studio/src/components/CreateFilesetStart/templates.tsweb/packages/studio/src/components/CreateFilesetStart/types.tsweb/packages/studio/src/components/ModelConfigPanel/index.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderConfigPane.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderDetailsPanel.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderPalette.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderToolbar.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/columns.test.tsweb/packages/studio/src/routes/DataDesignerJobBuildRoute/columns.tsweb/packages/studio/src/routes/DataDesignerJobBuildRoute/index.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/models.test.tsweb/packages/studio/src/routes/DataDesignerJobBuildRoute/models.tsweb/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.ts
6d6c86d to
f688c19
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderPalette.tsx (2)
14-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark immutable props as
readonlyin both interfaces.
web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderPalette.tsx#L14-L24: markBuilderPalettePropspropertiesreadonly.web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderDetailsPanel.tsx#L9-L15: markBuilderDetailsPanelPropspropertiesreadonly.🤖 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/DataDesignerJobBuildRoute/BuilderPalette.tsx` around lines 14 - 24, Mark every property in the BuilderPaletteProps interface in web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderPalette.tsx:14-24 as readonly, and do the same for every property in BuilderDetailsPanelProps in web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderDetailsPanel.tsx:9-15; preserve their existing types and callbacks.Source: Coding guidelines
4-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGroup external imports before internal aliases in both files.
web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderPalette.tsx#L4-L12: move thereactimport before@studio/...imports.web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderDetailsPanel.tsx#L4-L7: movelucide-reactandreactbefore the@studio/...import.🤖 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/DataDesignerJobBuildRoute/BuilderPalette.tsx` around lines 4 - 12, Reorder imports in BuilderPalette.tsx so the external react import appears before all `@studio` aliases. Apply the same import-grouping rule in BuilderDetailsPanel.tsx by placing lucide-react and react imports before the `@studio` import; no other changes are needed.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/routes/DataDesignerJobBuildRoute/BuilderDetailsPanel.tsx`:
- Around line 36-42: Update the Button using onToggle in BuilderDetailsPanel to
expose the current expanded state with aria-expanded, and associate it with the
details region via aria-controls. Ensure the controlled region has a stable
matching identifier and that the attribute reflects the actual
expanded/collapsed state.
- Around line 66-72: Update the CodeSnippet rendering in BuilderDetailsPanel so
the output of formatPreviewLogsForDisplay is treated as plain text rather than
JSON; change the language setting to the text/plain-text option while preserving
the existing previewLogs condition and styling.
- Around line 51-58: Update the validationErrors.map rendering in
BuilderDetailsPanel to use a stable, unique key for each list item instead of
the error string alone, since duplicate messages can collide; use the mapped
index as the fallback while preserving the existing displayed error text.
---
Nitpick comments:
In `@web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderPalette.tsx`:
- Around line 14-24: Mark every property in the BuilderPaletteProps interface in
web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderPalette.tsx:14-24
as readonly, and do the same for every property in BuilderDetailsPanelProps in
web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderDetailsPanel.tsx:9-15;
preserve their existing types and callbacks.
- Around line 4-12: Reorder imports in BuilderPalette.tsx so the external react
import appears before all `@studio` aliases. Apply the same import-grouping rule
in BuilderDetailsPanel.tsx by placing lucide-react and react imports before the
`@studio` import; no other changes are needed.
🪄 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: 3e2e6035-2c43-4831-91b5-bfa129e440e2
📒 Files selected for processing (16)
web/packages/studio/src/components/AddModelPalette/AddModelPalette.stories.tsxweb/packages/studio/src/components/AddModelPalette/index.tsxweb/packages/studio/src/components/ColumnConfigPanel/ColumnConfigPanel.tsxweb/packages/studio/src/components/CreateFilesetStart/templates.tsweb/packages/studio/src/components/CreateFilesetStart/types.tsweb/packages/studio/src/components/ModelConfigPanel/index.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderConfigPane.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderDetailsPanel.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderPalette.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderToolbar.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/columns.test.tsweb/packages/studio/src/routes/DataDesignerJobBuildRoute/columns.tsweb/packages/studio/src/routes/DataDesignerJobBuildRoute/index.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/models.test.tsweb/packages/studio/src/routes/DataDesignerJobBuildRoute/models.tsweb/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- web/packages/studio/src/components/AddModelPalette/AddModelPalette.stories.tsx
- web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderConfigPane.tsx
- web/packages/studio/src/routes/DataDesignerJobBuildRoute/columns.test.ts
- web/packages/studio/src/components/CreateFilesetStart/templates.ts
- web/packages/studio/src/components/ColumnConfigPanel/ColumnConfigPanel.tsx
- web/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderToolbar.tsx
- web/packages/studio/src/components/CreateFilesetStart/types.ts
- web/packages/studio/src/components/AddModelPalette/index.tsx
- web/packages/studio/src/routes/DataDesignerJobBuildRoute/models.test.ts
- web/packages/studio/src/components/ModelConfigPanel/index.tsx
- web/packages/studio/src/routes/DataDesignerJobBuildRoute/index.tsx
- web/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.ts
- web/packages/studio/src/routes/DataDesignerJobBuildRoute/models.ts
- web/packages/studio/src/routes/DataDesignerJobBuildRoute/columns.ts
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Signed-off-by: Sean Teramae <steramae@nvidia.com>
f688c19 to
b042a48
Compare
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Signed-off-by: Sean Teramae steramae@nvidia.com
Summary by CodeRabbit