CSOM-X Working Branch#2028
Conversation
| if (segCount === 0) return []; | ||
|
|
||
| // dp[dir] = best cost so far, exiting in `dir` | ||
| let dp: [number, number] = [Infinity, Infinity]; |
6e7eec5 to
7f54b42
Compare
There was a problem hiding this comment.
V1 Regression Risk Review — File-level annotations for changes outside the V2 scope.
See full analysis: V1 changes tracker
| @@ -1 +1,15 @@ | |||
| export const deepClone = <T>(obj: T): T => structuredClone(obj); | |||
There was a problem hiding this comment.
Risk: HIGH
What & Why: Replaced structuredClone with JSON.parse(JSON.stringify()) + MobX toJS() unwrapping. V2 uses MobX Keystone for state management; structuredClone throws on MobX observables because they contain non-serializable internal properties.
Risks: 17 V1 callers affected (history manager, autosave, debounced state, node duplication, flex node updates, task replacement, etc.). JSON.parse/stringify loses Date objects, Map, Set, undefined values, Infinity, NaN, and functions that structuredClone preserves. If any V1 code path stores these types in component specs, cloning silently corrupts data. Adds MobX as a runtime dependency of a core utility.
What if reverted? V2 MobX store operations (clone-on-write for undo, autosave snapshots) would throw at runtime. V1 reverts to safe structuredClone semantics.
| @@ -137,7 +137,7 @@ interface ContainerSpec { | |||
| interface ContainerImplementation { | |||
There was a problem hiding this comment.
Risk: MEDIUM
What & Why: Exported three previously-private types (ImplementationType, PredicateType, ExecutionOptionsSpec). Changed isGraphImplementation to accept undefined and return false instead of assuming the argument is always defined.
Risks: isGraphImplementation(undefined) now returns false silently instead of throwing. If V1 code passed undefined due to a bug, it was caught immediately before; now it proceeds with a falsy branch, potentially masking logic errors.
What if reverted? V2 model layer loses type imports. Any V2 code calling isGraphImplementation with a potentially-undefined value would need its own guard.
| @@ -5,6 +5,7 @@ import { ToastContainer } from "react-toastify"; | |||
| import { useDocumentTitle } from "@/hooks/useDocumentTitle"; | |||
There was a problem hiding this comment.
Risk: MEDIUM
What & Why: Wrapped the entire layout tree in <PipelineStorageProvider>, which is a V2 service for folder-based pipeline storage (IndexedDB + local filesystem drivers).
Risks: Adds a new React context provider to the V1 root. If PipelineStorageProvider triggers IndexedDB initialization, background fetches, or event listeners on mount, these run on every V1 page load.
What if reverted? V2 pipeline folders, file menu "Save As", and folder breadcrumbs all lose their storage context and break.
| @@ -17,19 +17,7 @@ export const ExportPipelineButton = () => { | |||
| }, [componentSpec]); | |||
There was a problem hiding this comment.
Risk: LOW
What & Why: Extracted file-download logic into a standalone exportPipeline() function. Existing button uses it.
Risks: Pure refactoring. Same behavior. exportPipeline is also consumed by V2 fileMenu.actions.ts.
What if reverted? V2 file menu export breaks. V1 unaffected if inline logic restored.
| @@ -142,7 +142,7 @@ const DropdownMenuRadioItem = forwardRef< | |||
| > | |||
There was a problem hiding this comment.
Risk: LOW
What & Why: Changed Icon size from null to "sm" in radio items. Added pl-2 padding to DropdownMenuShortcut.
Risks: Visual-only. Affects all dropdown menus globally (radio indicator size, shortcut label spacing).
What if reverted? Dropdown radio indicators and shortcut spacing revert.
| @@ -1,5 +1,5 @@ | |||
| import { cva, type VariantProps } from "class-variance-authority"; | |||
There was a problem hiding this comment.
Risk: LOW
What & Why: Added LucideProps spread to allow passing additional SVG attributes (e.g., strokeWidth, color).
Risks: Additive. Existing usages unaffected (extra props just pass through).
What if reverted? V2 icon customization (custom stroke widths, colors) stops working.
| @@ -7,12 +7,14 @@ interface ResizeHandleProps { | |||
| minWidth?: number; | |||
There was a problem hiding this comment.
Risk: LOW
What & Why: Added optional onDoubleClick prop to VerticalResizeHandle.
Risks: No behavior change when not provided. Used by V2 floating windows for double-click-to-reset-width.
What if reverted? V2 resize handles lose double-click reset.
cbb8625 to
74b6dde
Compare
🎩 PreviewA preview build has been created at: |
74b6dde to
c9a8ef6
Compare
| @@ -200,3 +200,39 @@ code { | |||
| display: none; /* Chrome, Safari, Opera */ | |||
| } | |||
| } | |||
|
|
|||
| /* Dialog stack transition animations */ | |||
There was a problem hiding this comment.
Addition to make dialog transitions smooth
| @@ -544,8 +544,9 @@ export const isContainerImplementation = ( | |||
| ): implementation is ContainerImplementation => "container" in implementation; | |||
|
|
|||
| export const isGraphImplementation = ( | |||
| implementation: ImplementationType, | |||
| ): implementation is GraphImplementation => "graph" in implementation; | |||
| implementation: ImplementationType | undefined, | |||
There was a problem hiding this comment.
Widening typing to accept undefined
c9a8ef6 to
554786f
Compare
|
|
||
| const DEFAULT_PAGE_SIZE = 10; | ||
|
|
||
| function usePagination<T>( |
There was a problem hiding this comment.
Extracted into separate file to be reused by multiple components
| </TableBody> | ||
| </Table> | ||
|
|
||
| {totalPages > 1 && ( |
There was a problem hiding this comment.
Extracted into PaginationControls
| "mobx": "^6.15.0", | ||
| "mobx-keystone": "^1.20.0", | ||
| "mobx-react-lite": "^4.1.1", | ||
| "prism-react-renderer": "^2.4.1", |
| onDragStart={(e: DragEvent<HTMLDivElement>) => { | ||
| if (!dragData) return; | ||
| e.dataTransfer.setData("application/x-folder-move", dragData); | ||
| e.dataTransfer.effectAllowed = "move"; | ||
| if (dragItemCount && dragItemCount > 1) { | ||
| const ghost = document.createElement("div"); | ||
| ghost.style.cssText = | ||
| "position:fixed;top:-1000px;left:-1000px;padding:6px 12px;border-radius:6px;font-size:14px;font-weight:500;color:white;background:#0f172a;box-shadow:0 4px 12px rgba(0,0,0,0.15);white-space:nowrap;"; | ||
| ghost.textContent = `${dragItemCount} items`; | ||
| document.body.appendChild(ghost); | ||
| e.dataTransfer.setDragImage(ghost, 0, 0); | ||
| requestAnimationFrame(() => ghost.remove()); | ||
| } | ||
| onDragStateChange?.(true); | ||
| }} | ||
| onDragEnd={() => onDragStateChange?.(false)} | ||
| className={dragData ? "cursor-grab" : undefined} | ||
| > |
There was a problem hiding this comment.
Can this be moved to a handleDragStart variable?
| if (dragItemCount && dragItemCount > 1) { | ||
| const ghost = document.createElement("div"); | ||
| ghost.style.cssText = | ||
| "position:fixed;top:-1000px;left:-1000px;padding:6px 12px;border-radius:6px;font-size:14px;font-weight:500;color:white;background:#0f172a;box-shadow:0 4px 12px rgba(0,0,0,0.15);white-space:nowrap;"; |
There was a problem hiding this comment.
does it make sense for this to be fixed at -1000, -1000?
| import NewPipelineButton from "../shared/NewPipelineButton"; | ||
|
|
||
| const AppMenu = () => { | ||
| const DefaultAppMenu = () => { |
There was a problem hiding this comment.
Worth renaming or marking as AppMenuV1?
| if (pathname.startsWith(RUN_V2_BASE_PATH)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I assume this is future-proofing? I'm not aware of a runs v2
| ); | ||
| }; | ||
|
|
||
| const RUN_V2_BASE_PATH = "/run-v2"; |
There was a problem hiding this comment.
This should be derived from the route defined in the router
| const showMoreButton = componentSpec?.name ? ( | ||
| <RecentExecutionsButton pipelineName={componentSpec.name} /> | ||
| ) : null; |
There was a problem hiding this comment.
| const showMoreButton = componentSpec?.name ? ( | |
| <RecentExecutionsButton pipelineName={componentSpec.name} /> | |
| ) : null; | |
| const showMoreButton = !!componentSpec?.name && | |
| <RecentExecutionsButton pipelineName={componentSpec.name} /> |
| <InlineStack | ||
| align="space-between" | ||
| gap="0" | ||
| className="px-4 py-2 bg-gray-50 border-b w-full z-1" |
| > | ||
| {isRoot ? ( | ||
| <InlineStack align="start" gap="1"> | ||
| <Home className="w-3 h-3" /> |
| <BreadcrumbPage> | ||
| {isRoot ? ( | ||
| <InlineStack align="start" gap="1"> | ||
| <Home className="w-3 h-3" /> |
| </BreadcrumbList> | ||
| </Breadcrumb> | ||
|
|
||
| <div className="text-xs text-gray-500"> |
|
|
||
| if (input.type) result.type = input.type; | ||
| if (input.description) result.description = input.description; | ||
| if (input.defaultValue) result.default = input.defaultValue; |
There was a problem hiding this comment.
🤖 This is an AI-generated code review comment.
Falsy default value silently dropped during serialization
if (input.defaultValue) result.default = input.defaultValue;InputSpec.default is typed as string | undefined, so "" (empty string) is a structurally valid default value. This falsy check drops it silently — a pipeline with default: "" would lose that field on roundtrip.
Fix:
if (input.defaultValue !== undefined) result.default = input.defaultValue;| } | ||
|
|
||
| export function isInputRequired(input: InputSpec): boolean { | ||
| return !input.optional && !input.default; |
There was a problem hiding this comment.
🤖 This is an AI-generated code review comment.
isInputRequired misclassifies empty-string default as "no default"
export function isInputRequired(input: InputSpec): boolean {
return !input.optional && !input.default;
}InputSpec.default is string | undefined. The falsy check !input.default returns true for an empty string (""), which is a valid default — so an input with default: "" would incorrectly generate a MISSING_REQUIRED_INPUT validation error.
Fix:
return !input.optional && input.default === undefined;| } | ||
|
|
||
| dialog.reject(createCancellationError()); | ||
| onClose(dialog.id); |
There was a problem hiding this comment.
🤖 This is an AI-generated code review comment.
Renderer calls reject then resolve on the same promise
In handleOpenChange and dialogProps.cancel:
dialog.reject(createCancellationError());
onClose(dialog.id); // internally calls resolver.resolve() — no-op but misleadingonClose is the provider's close(), which calls resolver.resolve(result) before cleaning up. Since the promise is already settled (rejected), the resolve is a no-op — so it doesn't break anything. But the renderer is bypassing the provider's cancel abstraction by calling reject directly. This makes the code harder to reason about and means the renderer has implicit knowledge of the provider's internals.
Suggested approach: pass an onCancel prop (backed by the provider's cancel()) to DialogRenderer, so the renderer doesn't need to call reject directly.
| } | ||
| } | ||
| previousStackLength.current = stack.length; | ||
| }, [disabled, stack.length, navigate, searchParams, stack, closingDialogIds]); |
There was a problem hiding this comment.
🤖 This is an AI-generated code review comment.
Redundant stack.length dep alongside stack
}, [disabled, stack.length, navigate, searchParams, stack, closingDialogIds]);stack as a dependency already covers changes to stack.length. The duplicate is unnecessary.
Fix: remove stack.length from the array.
|
Clicking on a handle now highlights the prop in the properties panel. Previously it would highlight all of the edges it was connected to. |
camielvs
left a comment
There was a problem hiding this comment.
Initial thoughts and comments throughout the code. UX stuff has been noted in Slack thread.
Some notable items to highlight:
- Pack & unpack subgraph is buggy
- Undo/redo needs improvement
- Pipeline validation is returning false positives and the availability of the submission functionality is not clear
- Some top nav buttons don't appear to do anything when clicked
- We need to provide a couple of default window layouts, and the first time the v2 editor is loaded it should have one of these already applied (ideally one that is similar to the existing v1 layout)
- Selection state seems to persist even after deleting a node or otherwise visually deselecting it
- Window behaviour can feel a bit awkward when not docked
- Pipeline renaming does not work
| * Uses ResizeObserver to detect content size changes and applies CSS transitions. | ||
| * During shrink transitions, overflow is visible to prevent content clipping. | ||
| */ | ||
| export function AnimatedHeight({ |
There was a problem hiding this comment.
I see this is only used inside the new dialog component. For exactly which scenario does it apply?
| ["task-component-ref-bar"]: { | ||
| name: "Task Component Ref Bar", | ||
| description: | ||
| "When enabled, the Component Ref Bar is shown in the Task Details panel moving some of the actions to the Actions section.", | ||
| default: true, | ||
| category: "setting", | ||
| }, |
There was a problem hiding this comment.
I'm not sure I understand what this is actually referring to
There was a problem hiding this comment.
After playing with things I see what this setting is changing. imo we don't need this setting; it should be default "true" behaviour. in "false" mode it just doesn't work well with the rest of the new UX
| ["snap-properties-to-node"]: { | ||
| name: "Position Properties near selected node", | ||
| description: | ||
| "When enabled, the floating Properties window is automatically positioned next to the selected node.", | ||
| default: false, | ||
| category: "setting", | ||
| }, |
There was a problem hiding this comment.
Not sure if intended but this setting appears to not work with multiselect
There was a problem hiding this comment.
There are some bugs somewhere in here:
- tasks moved into a subgraph seem to loose their annotations (or at least cache setting)
- multiple inputs with the same name across different tasks are not handled correctly - we end up with multiple input nodes with the same name, some of which will not be connected to anything. we should be checking the original connections to see what is connected to what and then determine from there exactly what inputs we need to create and link up
- positioning of generated input or output nodes in the subgraph could be improved
There was a problem hiding this comment.
I don't recall seeing Google Drive anywhere during my adventures. Where/what is this used for?
|
|
||
| // ad-hoc solution to display "Link" action | ||
| const taskNode = { | ||
| nodeId: taskName, |
There was a problem hiding this comment.
isn't this the exact kind of scenario we're trying to fix with the rewrite?
| <Button | ||
| onClick={handleOpenLogs} | ||
| variant="outline" | ||
| size="sm" | ||
| className="absolute -z-1 -top-8 right-0" | ||
| > | ||
| <Icon name="ScrollText" size="xs" /> | ||
| Open Logs | ||
| </Button> |
There was a problem hiding this comment.
That's a PoC - not a final product, a demo that we can use different components in Editor and RunView to cater the best ux we can craft.
|
Validation, from a subgraph, doesn't get surfaced to the top and you can submit a broken pipeline if there are validation errors in subgraphs. the Pipeline structure window picks up the validation but the validation under the details as well as the submit run button do not. |
b436d06 to
0bd9008
Compare
f4f4cb1 to
71c859e
Compare








Description
Related Issue and Pull requests
Type of Change
Checklist
Screenshots (if applicable)
Test Instructions
Additional Comments