fix(vscode): validate pipe connections and handle invalid pipeline data gracefully#553
Closed
charliegillet wants to merge 1 commit into
Closed
Conversation
…ta gracefully Malformed .pipe files with missing or dangling connection references (input/control entries with no "from" field or referencing non-existent components) caused the canvas UI to crash with an unrecoverable "invalid pipe" error. This adds validation at three layers: the pipeline parser now rejects files with broken connections and shows actionable errors, the graph builder silently skips malformed edges instead of creating undefined source/target pairs, and the canvas loadData is wrapped in try/catch to prevent total UI crashes from unexpected data shapes. Closes rocketride-org#395 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
charliegillet
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
github-actions
Bot
added
module:vscode
Contributor
📝 WalkthroughWalkthroughThese changes add validation and error handling throughout the pipeline processing stack to prevent crashes from malformed data. They validate component connections, catch JSON parsing errors, sanitize node references, and provide clear error messages rather than crashing the UI. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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)
packages/shared-ui/src/modules/flow/util/graph.ts (1)
256-277:⚠️ Potential issue | 🔴 CriticalMalformed
control/inputpayloads can still throw at runtime.Line 256 and Line 275 only gate on
.length; non-arrays with alengthproperty (or null entries inside arrays) can still crash at Line 258 / Line 277 when reading.from. This path should be fully defensive if malformed data is expected.🔧 Proposed defensive guards for array shape and entry shape
- if (data.control?.length) { - data.control.forEach((control: IControlConnection) => { - if (!control.from || !nodeIdSet.has(control.from)) return; + if (Array.isArray(data.control) && data.control.length > 0) { + data.control.forEach((control: IControlConnection | unknown) => { + if (!control || typeof control !== 'object') return; + const c = control as IControlConnection; + if (!c.from || !nodeIdSet.has(c.from)) return; edges.push({ ...DEFAULT_EDGE, id: uuid(), - source: control.from, + source: c.from, target: node.id, - sourceHandle: `invoke-source.${control.classType}`, + sourceHandle: `invoke-source.${c.classType}`, targetHandle: 'invoke-target', }); }); } @@ - if (data.input?.length) { - data.input.forEach((input: IInputConnection) => { - if (!input.from || !nodeIdSet.has(input.from) || !input.lane) return; + if (Array.isArray(data.input) && data.input.length > 0) { + data.input.forEach((input: IInputConnection | unknown) => { + if (!input || typeof input !== 'object') return; + const i = input as IInputConnection; + if (!i.from || !nodeIdSet.has(i.from) || !i.lane) return; edges.push({ ...DEFAULT_EDGE, id: uuid(), - source: input.from, + source: i.from, target: node.id, - sourceHandle: `source-${input.lane}`, - targetHandle: `target-${input.lane}`, + sourceHandle: `source-${i.lane}`, + targetHandle: `target-${i.lane}`, }); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-ui/src/modules/flow/util/graph.ts` around lines 256 - 277, The code currently assumes data.control and data.input are well-formed arrays and that each entry has the expected shape, which can throw when a non-array or malformed entry is present; update the guards to first verify Array.isArray(data.control) and Array.isArray(data.input) before iterating, and inside the loops validate each entry is a non-null object and has the required properties (e.g., for control entries ensure typeof control.from === 'string' && nodeIdSet.has(control.from') and for input entries ensure typeof input.from === 'string' && typeof input.lane !== 'undefined' && nodeIdSet.has(input.from')) before calling edges.push (the code paths using DEFAULT_EDGE, uuid(), sourceHandle/targetHandle) to prevent runtime crashes from malformed payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/vscode/src/shared/util/pipelineParser.ts`:
- Around line 195-219: The input/control validation can throw or misclassify
errors because the code assumes each item is a non-null object and silently
ignores non-array shapes; update the validation in pipelineParser.ts so that
before iterating you verify comp.input and comp.control are arrays (otherwise
push an error like `Component "${compId}" has invalid "input"/"control" shape`),
and inside each forEach first ensure conn is an object (typeof conn === 'object'
&& conn !== null) before reading properties; then validate c.from is a string
and exists in idSet and (for input) validate c.lane is a string, pushing
specific errors referencing compId and connIdx rather than letting access throw
or be reported as generic JSON errors.
---
Outside diff comments:
In `@packages/shared-ui/src/modules/flow/util/graph.ts`:
- Around line 256-277: The code currently assumes data.control and data.input
are well-formed arrays and that each entry has the expected shape, which can
throw when a non-array or malformed entry is present; update the guards to first
verify Array.isArray(data.control) and Array.isArray(data.input) before
iterating, and inside the loops validate each entry is a non-null object and has
the required properties (e.g., for control entries ensure typeof control.from
=== 'string' && nodeIdSet.has(control.from') and for input entries ensure typeof
input.from === 'string' && typeof input.lane !== 'undefined' &&
nodeIdSet.has(input.from')) before calling edges.push (the code paths using
DEFAULT_EDGE, uuid(), sourceHandle/targetHandle) to prevent runtime crashes from
malformed payloads.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e73324a0-02aa-4a61-a75b-39551ef7164b
📒 Files selected for processing (4)
apps/vscode/src/providers/views/PageEditor/PageEditor.tsxapps/vscode/src/shared/util/pipelineParser.tspackages/shared-ui/src/modules/flow/context/FlowGraphContext.tsxpackages/shared-ui/src/modules/flow/util/graph.ts
Comment on lines
+195
to
+219
| if (Array.isArray(comp.input)) { | ||
| comp.input.forEach((conn: unknown, connIdx: number) => { | ||
| const c = conn as Record<string, unknown>; | ||
| if (!c.from || typeof c.from !== 'string') { | ||
| errors.push(`Component "${compId}" input connection at index ${connIdx} is missing a valid "from" field`); | ||
| } else if (!idSet.has(c.from)) { | ||
| errors.push(`Component "${compId}" input connection references non-existent component "${c.from}"`); | ||
| } | ||
| if (!c.lane || typeof c.lane !== 'string') { | ||
| errors.push(`Component "${compId}" input connection at index ${connIdx} is missing a valid "lane" field`); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| // Validate control connections | ||
| if (Array.isArray(comp.control)) { | ||
| comp.control.forEach((conn: unknown, connIdx: number) => { | ||
| const c = conn as Record<string, unknown>; | ||
| if (!c.from || typeof c.from !== 'string') { | ||
| errors.push(`Component "${compId}" control connection at index ${connIdx} is missing a valid "from" field`); | ||
| } else if (!idSet.has(c.from)) { | ||
| errors.push(`Component "${compId}" control connection references non-existent component "${c.from}"`); | ||
| } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Connection validation can still throw on malformed entries and misclassify errors.
Line 197 and Line 212 assume each array item is an object. Payloads like "input":[null] or "control":[42] will throw during validation, then get reported as “Invalid JSON” instead of actionable connection errors. Also, non-array input/control shapes are silently accepted.
🔧 Proposed hardening for shape + entry validation
record.components.forEach((component: unknown, index: number) => {
const comp = component as Record<string, unknown>;
const compId = (comp.id as string) || `index ${index}`;
// Validate input connections
- if (Array.isArray(comp.input)) {
+ if ('input' in comp && !Array.isArray(comp.input)) {
+ errors.push(`Component "${compId}" field "input" must be an array`);
+ } else if (Array.isArray(comp.input)) {
comp.input.forEach((conn: unknown, connIdx: number) => {
- const c = conn as Record<string, unknown>;
+ if (!conn || typeof conn !== 'object') {
+ errors.push(`Component "${compId}" input connection at index ${connIdx} must be an object`);
+ return;
+ }
+ const c = conn as Record<string, unknown>;
if (!c.from || typeof c.from !== 'string') {
errors.push(`Component "${compId}" input connection at index ${connIdx} is missing a valid "from" field`);
} else if (!idSet.has(c.from)) {
errors.push(`Component "${compId}" input connection references non-existent component "${c.from}"`);
}
if (!c.lane || typeof c.lane !== 'string') {
errors.push(`Component "${compId}" input connection at index ${connIdx} is missing a valid "lane" field`);
}
});
}
// Validate control connections
- if (Array.isArray(comp.control)) {
+ if ('control' in comp && !Array.isArray(comp.control)) {
+ errors.push(`Component "${compId}" field "control" must be an array`);
+ } else if (Array.isArray(comp.control)) {
comp.control.forEach((conn: unknown, connIdx: number) => {
- const c = conn as Record<string, unknown>;
+ if (!conn || typeof conn !== 'object') {
+ errors.push(`Component "${compId}" control connection at index ${connIdx} must be an object`);
+ return;
+ }
+ const c = conn as Record<string, unknown>;
if (!c.from || typeof c.from !== 'string') {
errors.push(`Component "${compId}" control connection at index ${connIdx} is missing a valid "from" field`);
} else if (!idSet.has(c.from)) {
errors.push(`Component "${compId}" control connection references non-existent component "${c.from}"`);
}
});
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/vscode/src/shared/util/pipelineParser.ts` around lines 195 - 219, The
input/control validation can throw or misclassify errors because the code
assumes each item is a non-null object and silently ignores non-array shapes;
update the validation in pipelineParser.ts so that before iterating you verify
comp.input and comp.control are arrays (otherwise push an error like `Component
"${compId}" has invalid "input"/"control" shape`), and inside each forEach first
ensure conn is an object (typeof conn === 'object' && conn !== null) before
reading properties; then validate c.from is a string and exists in idSet and
(for input) validate c.lane is a string, pushing specific errors referencing
compId and connIdx rather than letting access throw or be reported as generic
JSON errors.
Collaborator
|
@Rod-Christensen |
Contributor
Author
|
@stepmikhaylov Good point — using the Should we rework this PR to use |
Collaborator
|
Thank you for contribution. Pipeline validation is implemented on Server side and can be leveraged by both Python and TypeScript RocketRide clients with |
10 tasks
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
2 participants
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.
Summary
.pipefiles with brokeninput/controlreferences are rejected with clear, actionable error messages before reaching the canvasundefinedsource/targetloadDatapath and the webview JSON parse in try/catch so unexpected data shapes show an error overlay instead of crashing the UIType
Bug fix
Why this bug happens in this codebase
The pipeline file parser (
PipelineFileParser.validatePipelineStructure) only validated that components haveidandproviderfields, but never checked theinputandcontrolconnection arrays. When a.pipefile contained connections with a missingfromfield, a missinglanefield, or afromvalue referencing a component ID that doesn't exist in the file, the data passed validation and was sent to the canvas. The graph builder (getEdgesFromNodes) then created React Flow edges withundefinedsource or target handles, which caused React Flow to throw an unrecoverable error, crashing the entire canvas UI with a cryptic "invalid pipe" message and no recovery path.What changed
apps/vscode/src/shared/util/pipelineParser.tsinput[].from,input[].lane, andcontrol[].fromfields on each component, including cross-reference checks against the component ID setpackages/shared-ui/src/modules/flow/util/graph.tsgetEdgesFromNodesnow builds a node ID set and skips any connection entry wherefromis missing or references a non-existent nodeapps/vscode/src/providers/views/PageEditor/PageEditor.tsxJSON.parsein the'update'message handler with try/catch, routing parse failures to the file-invalid error overlaypackages/shared-ui/src/modules/flow/context/FlowGraphContext.tsxloadDatain try/catch so any unexpected error clears the canvas instead of crashing itValidation
.pipefile with missingfromin input connection shows clear error in the editor overlay.pipefile referencing a non-existent component ID shows specific error message naming the bad reference.pipefiles continue to load and render normallyHow this could be extended
.pipefiles as a pre-parse stepCloses #395
#Hack-with-bay-2
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes