fix(studio): Use RHF for render performance#746
Open
steramae-nvidia wants to merge 5 commits into
Open
Conversation
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Contributor
|
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe data designer builder now uses shared react-hook-form state for columns, models, toolbar fields, configuration panels, graph rendering, validation, submission, and preview. Preview config errors are surfaced in logs, and controlled switches support value conversion hooks. ChangesData designer form state
Sequence Diagram(s)sequenceDiagram
participant User
participant DataDesignerJobBuildRoute
participant FormProvider
participant BuilderConfigPane
participant BuilderCanvas
User->>BuilderConfigPane: edit column or model
BuilderConfigPane->>FormProvider: update controlled field
FormProvider->>DataDesignerJobBuildRoute: provide current form values
DataDesignerJobBuildRoute->>BuilderCanvas: pass form-backed columns
BuilderCanvas->>BuilderCanvas: buildGraph(columns)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 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/DataDesignerJobBuildRoute/index.tsx (1)
135-154: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not swallow configuration-build failures.
buildDataDesignerConfigruns inside thistry, but the catch assumes every error populatedcreateJob.error. A build failure therefore opens an empty details panel. Build separately and surface that error, or narrow this catch to mutation failures.Based on learnings, submit handlers must not silently swallow non-mutation build errors.
🤖 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/index.tsx` around lines 135 - 154, The submit flow should not route buildDataDesignerConfig failures through the mutation-error catch. Move configuration construction outside the try/catch, or otherwise narrow the catch to createJob.mutateAsync failures, and surface any configuration-build error directly while preserving the existing mutation failure handling and navigation behavior.Source: Learnings
🧹 Nitpick comments (3)
web/packages/studio/src/components/NewDataDesignerJobForm/usePreview.test.tsx (2)
36-40: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the original config-build error.
This regex also matches the fallback preview message, so the test would pass even if the thrown
SyntaxError.messagewere discarded. AssertUnexpected token n in JSON at position 2directly.🤖 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/NewDataDesignerJobForm/usePreview.test.tsx` around lines 36 - 40, Strengthen the assertion in the previewLogs test for the config-build error so it checks for the exact original SyntaxError message, “Unexpected token n in JSON at position 2,” rather than the broad JSON/error/preview regex. Keep the existing non-empty previewLogs assertion and verify that the thrown error message is preserved and shown to the user.
46-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the unchecked callback assertion with narrowing.
Use a type predicate in
findinstead ofas ((l: string) => void) | undefined.As per coding guidelines, TypeScript should prefer type guards and narrowing over type assertions.
🤖 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/NewDataDesignerJobForm/usePreview.test.tsx` around lines 46 - 49, Update the streamPreviewMock implementation to replace the callback type assertion in the args.find call with an inline type predicate that narrows the matching value to the expected line-handler function, preserving the existing optional invocation of onLine.Source: Coding guidelines
web/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.ts (1)
51-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the hook's return type and remove the assertion.
Add a
JobBuilderReturninterface, annotateuseJobBuilder, and returnformdirectly. The current assertion adds no safety.As per coding guidelines, “Use explicit return types for public APIs and complex functions in TypeScript” and “Use type assertions sparingly.”
Also applies to: 181-183
🤖 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/useJobBuilder.ts` around lines 51 - 55, Define a JobBuilderReturn interface for the hook’s returned shape, annotate useJobBuilder with that explicit return type, and remove the unnecessary type assertion from the returned form value. Update the corresponding return statement while preserving the existing hook behavior and API.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/ModelConfigPanel/index.tsx`:
- Around line 56-59: Update the alias set construction in the ModelConfigPanel
validation flow around validateModelAlias so every peer alias is trimmed before
uniqueness checking. Preserve excluding modelIndex and filtering empty values,
and ensure validateModelAlias receives normalized alias values.
---
Outside diff comments:
In `@web/packages/studio/src/routes/DataDesignerJobBuildRoute/index.tsx`:
- Around line 135-154: The submit flow should not route buildDataDesignerConfig
failures through the mutation-error catch. Move configuration construction
outside the try/catch, or otherwise narrow the catch to createJob.mutateAsync
failures, and surface any configuration-build error directly while preserving
the existing mutation failure handling and navigation behavior.
---
Nitpick comments:
In
`@web/packages/studio/src/components/NewDataDesignerJobForm/usePreview.test.tsx`:
- Around line 36-40: Strengthen the assertion in the previewLogs test for the
config-build error so it checks for the exact original SyntaxError message,
“Unexpected token n in JSON at position 2,” rather than the broad
JSON/error/preview regex. Keep the existing non-empty previewLogs assertion and
verify that the thrown error message is preserved and shown to the user.
- Around line 46-49: Update the streamPreviewMock implementation to replace the
callback type assertion in the args.find call with an inline type predicate that
narrows the matching value to the expected line-handler function, preserving the
existing optional invocation of onLine.
In `@web/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.ts`:
- Around line 51-55: Define a JobBuilderReturn interface for the hook’s returned
shape, annotate useJobBuilder with that explicit return type, and remove the
unnecessary type assertion from the returned form value. Update the
corresponding return statement while preserving the existing hook behavior and
API.
🪄 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: ddb76e80-f5eb-4648-8add-6551328d96ce
📒 Files selected for processing (12)
web/packages/common/src/components/form/ControlledSwitch/index.tsxweb/packages/studio/src/components/ColumnConfigPanel/SeedDatasetConfig.tsxweb/packages/studio/src/components/ColumnConfigPanel/index.tsxweb/packages/studio/src/components/ModelConfigPanel/index.tsxweb/packages/studio/src/components/NewDataDesignerJobForm/usePreview.test.tsxweb/packages/studio/src/components/NewDataDesignerJobForm/usePreview.tsweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderCanvas.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderConfigPane.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderPalette.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/BuilderToolbar.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/index.tsxweb/packages/studio/src/routes/DataDesignerJobBuildRoute/useJobBuilder.ts
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
1 participant
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.
This PR is a performance improvement for the DD build route by using React Hook Form for state management. By adopting this, we can cut down on unnecessary rerenders. It also helps with state access and form components.
Summary by CodeRabbit