feat(studio): Transform File Modal with preview schema output - #1032
Open
steramae-nvidia wants to merge 1 commit into
Open
feat(studio): Transform File Modal with preview schema output#1032steramae-nvidia wants to merge 1 commit into
steramae-nvidia wants to merge 1 commit into
Conversation
Signed-off-by: Sean Teramae <steramae@nvidia.com>
Contributor
Author
|
This change is part of the following stack: Change managed by git-spice. |
Contributor
📝 WalkthroughWalkthroughThe PR extracts reusable mapping rows, adds dataset-aware file action handling, and replaces transform schema output with a generated source-to-transformed preview. ChangesStudio mapping and transform workflow
Sequence Diagram(s)sequenceDiagram
participant User
participant TransformFileModal
participant useTransformPreview
participant PreviewOutputPanel
User->>TransformFileModal: open transform modal
TransformFileModal->>useTransformPreview: pass file content and mappings
useTransformPreview->>TransformFileModal: return source and transformed row
TransformFileModal->>PreviewOutputPanel: render preview values
User->>PreviewOutputPanel: navigate rows or copy output
Possibly related PRs
Suggested labels: 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: 4
🧹 Nitpick comments (2)
web/packages/common/src/components/form/MappingFields/types.ts (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType-only imports use value import syntax in both new files. Both files import symbols that appear only in type positions.
web/packages/common/src/components/form/MappingFields/types.ts#L13-L14: convert theControlledComboboxandControlledTextInputimports toimport type.web/packages/common/src/components/form/MappingFields/MappingRow.tsx#L23-L23: convert theControlandFieldValuesimport toimport type.As per coding guidelines: "Use
import typefor type-only imports in TypeScript".🤖 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/common/src/components/form/MappingFields/types.ts` around lines 13 - 14, Update web/packages/common/src/components/form/MappingFields/types.ts lines 13-14 to use type-only imports for ControlledCombobox and ControlledTextInput, and update web/packages/common/src/components/form/MappingFields/MappingRow.tsx line 23 to use a type-only import for Control and FieldValues; no other changes are needed.Source: Coding guidelines
web/packages/common/src/components/form/MappingFields/index.tsx (1)
194-201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the four assertions.
{}is already assignable toPartial<...>, so??infers the correct type. TypeNO_OVERRIDESonce instead.♻️ Proposed change
- const keyComboboxProps = (attributes?.keyCombobox ?? - NO_OVERRIDES) as Partial<KeyValueComboboxPassthrough>; - const valueComboboxProps = (attributes?.valueCombobox ?? - NO_OVERRIDES) as Partial<KeyValueComboboxPassthrough>; - const keyTextInputProps = (attributes?.keyTextInput ?? - NO_OVERRIDES) as Partial<KeyValueTextInputPassthrough>; - const valueTextInputProps = (attributes?.valueTextInput ?? - NO_OVERRIDES) as Partial<KeyValueTextInputPassthrough>; + const keyComboboxProps = attributes?.keyCombobox ?? NO_OVERRIDES; + const valueComboboxProps = attributes?.valueCombobox ?? NO_OVERRIDES; + const keyTextInputProps = attributes?.keyTextInput ?? NO_OVERRIDES; + const valueTextInputProps = attributes?.valueTextInput ?? NO_OVERRIDES;With line 67:
const NO_OVERRIDES: Readonly<Record<string, never>> = {};As per coding guidelines: "Use type assertions sparingly. Prefer type guards and narrowing in TypeScript".
🤖 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/common/src/components/form/MappingFields/index.tsx` around lines 194 - 201, Remove the four type assertions from the keyComboboxProps, valueComboboxProps, keyTextInputProps, and valueTextInputProps initializations. Type NO_OVERRIDES once as Readonly<Record<string, never>> near its declaration, allowing each nullish-coalescing expression to infer the appropriate Partial passthrough type.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/FilesTable/TransformFileModal/index.tsx`:
- Around line 64-65: Update the fileType derivation near resolvedFilepath so
filenames without a dot produce an empty file type instead of the full filename;
preserve extracting the final suffix for paths with extensions, ensuring
parseFileContent does not misclassify extensionless names based on substring
matches.
In
`@web/packages/studio/src/components/FilesTable/TransformFileModal/TransformPreview.tsx`:
- Around line 4-8: Use type-only imports for the specified symbols: in
web/packages/studio/src/components/FilesTable/TransformFileModal/TransformPreview.tsx
lines 4-8, import TransformFileFormFields, FC, and Control with import type; in
web/packages/studio/src/components/FilesTable/TransformFileModal/index.tsx lines
15-18, mark TransformFileFormFields as type while keeping transformFileSchema a
value import; and on line 25, mark ComponentProps and FC as type while keeping
useMemo as a value import.
In
`@web/packages/studio/src/components/FilesTable/TransformFileModal/useTransformPreview.ts`:
- Around line 21-46: Update the nested-path construction in the mapping loop so
each intermediate segment is verified to be a non-null object before assigning
into it; replace any existing primitive or null value with a fresh object before
reassigning current. Preserve existing object branches and ensure conflicting
mappings such as “user” followed by “user.name” complete without throwing.
In `@web/packages/studio/src/components/PreviewOutputPanel/index.tsx`:
- Around line 51-60: Add an aria-expanded attribute to the collapse toggle
Button, deriving its boolean value from the existing collapsed state so expanded
and collapsed states are accurately exposed to screen readers. Keep the current
onClick handler, icons, and aria-label unchanged.
---
Nitpick comments:
In `@web/packages/common/src/components/form/MappingFields/index.tsx`:
- Around line 194-201: Remove the four type assertions from the
keyComboboxProps, valueComboboxProps, keyTextInputProps, and valueTextInputProps
initializations. Type NO_OVERRIDES once as Readonly<Record<string, never>> near
its declaration, allowing each nullish-coalescing expression to infer the
appropriate Partial passthrough type.
In `@web/packages/common/src/components/form/MappingFields/types.ts`:
- Around line 13-14: Update
web/packages/common/src/components/form/MappingFields/types.ts lines 13-14 to
use type-only imports for ControlledCombobox and ControlledTextInput, and update
web/packages/common/src/components/form/MappingFields/MappingRow.tsx line 23 to
use a type-only import for Control and FieldValues; 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: 9791b903-8fcb-4256-bc16-d8344b32b4a4
📒 Files selected for processing (9)
web/packages/common/src/components/form/MappingFields/MappingRow.tsxweb/packages/common/src/components/form/MappingFields/index.tsxweb/packages/common/src/components/form/MappingFields/types.tsweb/packages/studio/src/components/FilesTable/FileQuickActions/index.tsxweb/packages/studio/src/components/FilesTable/RenameFileModal/index.tsxweb/packages/studio/src/components/FilesTable/TransformFileModal/TransformPreview.tsxweb/packages/studio/src/components/FilesTable/TransformFileModal/index.tsxweb/packages/studio/src/components/FilesTable/TransformFileModal/useTransformPreview.tsweb/packages/studio/src/components/PreviewOutputPanel/index.tsx
Comment on lines
+64
to
+65
| const resolvedFilepath = filepath ?? ''; | ||
| const fileType = resolvedFilepath.split('.').at(-1) ?? ''; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle filepaths without an extension.
resolvedFilepath.split('.').at(-1) returns the full filename when there is no dot in the name (for example, "README" yields "README" instead of ""). Downstream, fileType is checked with fileType?.includes('csv') in parseFileContent; a filename that happens to contain the substring "csv" without an extension would be misclassified as CSV.
🩹 Proposed fix
- const resolvedFilepath = filepath ?? '';
- const fileType = resolvedFilepath.split('.').at(-1) ?? '';
+ const resolvedFilepath = filepath ?? '';
+ const extensionParts = resolvedFilepath.split('.');
+ const fileType = extensionParts.length > 1 ? (extensionParts.at(-1) ?? '') : '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const resolvedFilepath = filepath ?? ''; | |
| const fileType = resolvedFilepath.split('.').at(-1) ?? ''; | |
| const resolvedFilepath = filepath ?? ''; | |
| const extensionParts = resolvedFilepath.split('.'); | |
| const fileType = extensionParts.length > 1 ? (extensionParts.at(-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/FilesTable/TransformFileModal/index.tsx`
around lines 64 - 65, Update the fileType derivation near resolvedFilepath so
filenames without a dot produce an empty file type instead of the full filename;
preserve extracting the final suffix for paths with extensions, ensuring
parseFileContent does not misclassify extensionless names based on substring
matches.
Comment on lines
+4
to
+8
| import { TransformFileFormFields } from '@studio/components/FilesTable/TransformFileModal/types'; | ||
| import { useTransformPreview } from '@studio/components/FilesTable/TransformFileModal/useTransformPreview'; | ||
| import { PreviewOutputPanel } from '@studio/components/PreviewOutputPanel'; | ||
| import { FC, useMemo } from 'react'; | ||
| import { Control, useWatch } from 'react-hook-form'; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use import type for type-only symbols. Both files import type-only symbols as regular imports; the sibling file useTransformPreview.ts already does this correctly for the same symbol.
web/packages/studio/src/components/FilesTable/TransformFileModal/TransformPreview.tsx#L4-L8: markTransformFileFormFields(line 4, only used inControl<TransformFileFormFields>),FC(line 7), andControl(line 8) asimport type.web/packages/studio/src/components/FilesTable/TransformFileModal/index.tsx#L15-L18: markTransformFileFormFieldsastypewithin the combined import;transformFileSchemastays a value import.web/packages/studio/src/components/FilesTable/TransformFileModal/index.tsx#L25-L25: markComponentPropsandFCastype;useMemostays a value import.
🔧 Proposed fixes
--- a/web/packages/studio/src/components/FilesTable/TransformFileModal/TransformPreview.tsx
-import { TransformFileFormFields } from '`@studio/components/FilesTable/TransformFileModal/types`';
+import type { TransformFileFormFields } from '`@studio/components/FilesTable/TransformFileModal/types`';
import { useTransformPreview } from '`@studio/components/FilesTable/TransformFileModal/useTransformPreview`';
import { PreviewOutputPanel } from '`@studio/components/PreviewOutputPanel`';
-import { FC, useMemo } from 'react';
-import { Control, useWatch } from 'react-hook-form';
+import { useMemo, type FC } from 'react';
+import { useWatch, type Control } from 'react-hook-form';--- a/web/packages/studio/src/components/FilesTable/TransformFileModal/index.tsx
import {
- TransformFileFormFields,
+ type TransformFileFormFields,
transformFileSchema,
} from '`@studio/components/FilesTable/TransformFileModal/types`';
...
-import { ComponentProps, FC, useMemo } from 'react';
+import { useMemo, type ComponentProps, type FC } from 'react';Based on coding guidelines: "Use import type for type-only imports in TypeScript."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { TransformFileFormFields } from '@studio/components/FilesTable/TransformFileModal/types'; | |
| import { useTransformPreview } from '@studio/components/FilesTable/TransformFileModal/useTransformPreview'; | |
| import { PreviewOutputPanel } from '@studio/components/PreviewOutputPanel'; | |
| import { FC, useMemo } from 'react'; | |
| import { Control, useWatch } from 'react-hook-form'; | |
| import type { TransformFileFormFields } from '`@studio/components/FilesTable/TransformFileModal/types`'; | |
| import { useTransformPreview } from '`@studio/components/FilesTable/TransformFileModal/useTransformPreview`'; | |
| import { PreviewOutputPanel } from '`@studio/components/PreviewOutputPanel`'; | |
| import { useMemo, type FC } from 'react'; | |
| import { useWatch, type Control } from 'react-hook-form'; |
Suggested change
| import { TransformFileFormFields } from '@studio/components/FilesTable/TransformFileModal/types'; | |
| import { useTransformPreview } from '@studio/components/FilesTable/TransformFileModal/useTransformPreview'; | |
| import { PreviewOutputPanel } from '@studio/components/PreviewOutputPanel'; | |
| import { FC, useMemo } from 'react'; | |
| import { Control, useWatch } from 'react-hook-form'; | |
| import { | |
| type TransformFileFormFields, | |
| transformFileSchema, | |
| } from '`@studio/components/FilesTable/TransformFileModal/types`'; |
Suggested change
| import { TransformFileFormFields } from '@studio/components/FilesTable/TransformFileModal/types'; | |
| import { useTransformPreview } from '@studio/components/FilesTable/TransformFileModal/useTransformPreview'; | |
| import { PreviewOutputPanel } from '@studio/components/PreviewOutputPanel'; | |
| import { FC, useMemo } from 'react'; | |
| import { Control, useWatch } from 'react-hook-form'; | |
| import { useMemo, type ComponentProps, type FC } from 'react'; |
📍 Affects 2 files
web/packages/studio/src/components/FilesTable/TransformFileModal/TransformPreview.tsx#L4-L8(this comment)web/packages/studio/src/components/FilesTable/TransformFileModal/index.tsx#L15-L18web/packages/studio/src/components/FilesTable/TransformFileModal/index.tsx#L25-L25
🤖 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/FilesTable/TransformFileModal/TransformPreview.tsx`
around lines 4 - 8, Use type-only imports for the specified symbols: in
web/packages/studio/src/components/FilesTable/TransformFileModal/TransformPreview.tsx
lines 4-8, import TransformFileFormFields, FC, and Control with import type; in
web/packages/studio/src/components/FilesTable/TransformFileModal/index.tsx lines
15-18, mark TransformFileFormFields as type while keeping transformFileSchema a
value import; and on line 25, mark ComponentProps and FC as type while keeping
useMemo as a value import.
Source: Coding guidelines
Comment on lines
+21
to
+46
| for (const { key, value } of mappings) { | ||
| if (!key.trim()) continue; | ||
|
|
||
| const template = Handlebars.compile(value ?? ''); | ||
| const keyParts = key.split('.'); | ||
| let current = newRow; | ||
|
|
||
| for (let i = 0; i < keyParts.length - 1; i++) { | ||
| const part = keyParts[i]; | ||
| if (!(part in current)) current[part] = {}; | ||
| current = current[part] as Record<string, unknown>; | ||
| } | ||
|
|
||
| const lastPart = keyParts[keyParts.length - 1]; | ||
| const compiledValue = template(processedRow); | ||
|
|
||
| try { | ||
| if (compiledValue.trim().startsWith('[') || compiledValue.trim().startsWith('{')) { | ||
| current[lastPart] = JSON.parse(compiledValue); | ||
| } else { | ||
| current[lastPart] = compiledValue; | ||
| } | ||
| } catch { | ||
| current[lastPart] = compiledValue; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fix crash on conflicting nested mapping keys.
The loop reuses current[part] as the next nesting target without checking its type:
if (!(part in current)) current[part] = {};
current = current[part] as Record<string, unknown>;If an earlier mapping already assigned current[part] to a non-object value (e.g. mapping key "user" set newRow.user to a string), then a later mapping key "user.name" reuses that string as current. Module code runs in strict mode, so current[lastPart] = ... on a primitive throws TypeError. This throw happens inside the afterRow useMemo, during render, and can crash the preview.
Two mapping rows with keys "user" and "user.name" are both valid free-text input in the mapping UI, so this is reachable, not theoretical.
🛠️ Proposed fix
for (let i = 0; i < keyParts.length - 1; i++) {
const part = keyParts[i];
- if (!(part in current)) current[part] = {};
+ const existing = current[part];
+ if (typeof existing !== 'object' || existing === null || Array.isArray(existing)) {
+ current[part] = {};
+ }
current = current[part] as Record<string, unknown>;
}🤖 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/FilesTable/TransformFileModal/useTransformPreview.ts`
around lines 21 - 46, Update the nested-path construction in the mapping loop so
each intermediate segment is verified to be a non-null object before assigning
into it; replace any existing primitive or null value with a fresh object before
reassigning current. Preserve existing object branches and ensure conflicting
mappings such as “user” followed by “user.name” complete without throwing.
Comment on lines
+51
to
+60
| <Button | ||
| size="small" | ||
| type="button" | ||
| kind="tertiary" | ||
| onClick={() => setCollapsed(!collapsed)} | ||
| className="shrink-0 text-secondary transition-colors hover:text-primary" | ||
| aria-label={collapsed ? 'Expand preview' : 'Collapse preview'} | ||
| > | ||
| {collapsed ? <ChevronRight size={14} /> : <ChevronDown size={14} />} | ||
| </Button> |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add aria-expanded to the collapse toggle.
The button toggles collapsed and sets aria-label, but it does not expose aria-expanded. Screen readers cannot tell whether the panel is expanded or collapsed.
♿ Proposed fix
<Button
size="small"
type="button"
kind="tertiary"
onClick={() => setCollapsed(!collapsed)}
className="shrink-0 text-secondary transition-colors hover:text-primary"
aria-label={collapsed ? 'Expand preview' : 'Collapse preview'}
+ aria-expanded={!collapsed}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Button | |
| size="small" | |
| type="button" | |
| kind="tertiary" | |
| onClick={() => setCollapsed(!collapsed)} | |
| className="shrink-0 text-secondary transition-colors hover:text-primary" | |
| aria-label={collapsed ? 'Expand preview' : 'Collapse preview'} | |
| > | |
| {collapsed ? <ChevronRight size={14} /> : <ChevronDown size={14} />} | |
| </Button> | |
| <Button | |
| size="small" | |
| type="button" | |
| kind="tertiary" | |
| onClick={() => setCollapsed(!collapsed)} | |
| className="shrink-0 text-secondary transition-colors hover:text-primary" | |
| aria-label={collapsed ? 'Expand preview' : 'Collapse preview'} | |
| aria-expanded={!collapsed} | |
| > | |
| {collapsed ? <ChevronRight size={14} /> : <ChevronDown size={14} />} | |
| </Button> |
🤖 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/PreviewOutputPanel/index.tsx` around lines
51 - 60, Add an aria-expanded attribute to the collapse toggle Button, deriving
its boolean value from the existing collapsed state so expanded and collapsed
states are accurately exposed to screen readers. Keep the current onClick
handler, icons, and aria-label unchanged.
Contributor
|
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.
Signed-off-by: Sean Teramae steramae@nvidia.com
Summary by CodeRabbit