fix(assets): show text outputs instead of dropping jobs with a text-shaped preview#13292
Open
DeniDoman wants to merge 2 commits into
Open
fix(assets): show text outputs instead of dropping jobs with a text-shaped preview#13292DeniDoman wants to merge 2 commits into
DeniDoman wants to merge 2 commits into
Conversation
…t dropped
The jobs list synthesizes a node's `text` array from a `preview_output`
object ({content}), but parseTextOutput only accepted raw strings and
discarded the object — leaving the job with no previewable output, so it
was dropped from the Jobs and Generated panels. Read the content from
both the string (detail/subfeed) and object (list) shapes, and un-skip
the realistic text-preview test.
dosubot
Bot
added
the
size:M
|
✅ All contributors have signed the CLA. Thank you! This PR is ready to be merged. |
🎭 Playwright: ⏳ Running...🎨 Storybook: 🚧 Building... |
Contributor
📝 WalkthroughWalkthroughText outputs from node output maps are now parsed into Text Output Parsing and Preview Priority
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
Contributor
Author
|
I have read and agree to the Contributor License Agreement |
comfy-legal
added a commit
to Comfy-Org/comfy-cla
that referenced
this pull request
Jun 29, 2026
![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: 2
🤖 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 `@src/stores/resultItemParsing.test.ts`:
- Around line 116-147: Add a regression test in resultItemParsing.test.ts for
the direct single-string text shape in parseNodeOutput; the current cases cover
arrays and synthesized preview_output objects, but not text: string. Extend the
existing parseNodeOutput expectations with a case like the other it blocks,
asserting a plain string becomes a single text result item with the correct
content, nodeId, and generated filename so the supported
NodeExecutionOutput['text'] shape stays covered.
In `@src/stores/resultItemParsing.ts`:
- Around line 76-83: Raw string text outputs are being filtered out in
resultItemParsing because the Object.entries(...).filter(...) step only accepts
array values, so parseTextOutput never sees NodeExecutionOutput['text'] when it
is a plain string. Update the logic in resultItemParsing (the flatMap branch
around parseTextOutput and ResultItemImpl creation) to explicitly handle the
'text' mediaType before the array check, accepting both string and array forms
for text while keeping the existing array-only handling for other media types.
🪄 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 Plus
Run ID: 2402a178-6df2-4aad-8a45-f1cdaf137e66
📒 Files selected for processing (5)
src/platform/remote/comfyui/jobs/jobTypes.tssrc/stores/queueStore.test.tssrc/stores/queueStore.tssrc/stores/resultItemParsing.test.tssrc/stores/resultItemParsing.ts
Comment on lines
+116
to
+147
| it('parses array-shaped text outputs into text result items', () => { | ||
| const output = makeOutput({ | ||
| images: [{ filename: 'img.png', subfolder: '', type: 'output' }], | ||
| text: ['first', 'second'] | ||
| }) | ||
|
|
||
| const result = parseNodeOutput('6', output) | ||
|
|
||
| expect(result).toHaveLength(3) | ||
|
|
||
| const textItems = result.filter((item) => item.isText) | ||
| expect(textItems).toHaveLength(2) | ||
| expect(textItems.map((item) => item.content)).toEqual(['first', 'second']) | ||
| expect(textItems[0].nodeId).toBe('6') | ||
| expect(textItems[0].filename).toBe('6-text-0.txt') | ||
| }) | ||
|
|
||
| it('parses object-shaped text entries from a synthesized preview_output', () => { | ||
| const output = makeOutput({ | ||
| text: [ | ||
| { nodeId: '6', mediaType: 'text', content: 'from preview_output' } | ||
| ] as unknown as NodeExecutionOutput['text'] | ||
| }) | ||
|
|
||
| const result = parseNodeOutput('6', output) | ||
|
|
||
| expect(result).toHaveLength(1) | ||
| expect(result[0].isText).toBe(true) | ||
| expect(result[0].content).toBe('from preview_output') | ||
| expect(result[0].filename).toBe('6-text-0.txt') | ||
| }) | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the single-string text case here too.
These tests cover arrays and synthesized preview objects, but the upstream
contract still allows text: string. A direct text: 'hello' regression test
would lock that supported shape in and catch the current parser gap.
Suggested test
+ it('parses single-string text outputs', () => {
+ const result = parseNodeOutput('6', makeOutput({ text: 'hello' }))
+
+ expect(result).toHaveLength(1)
+ expect(result[0].isText).toBe(true)
+ expect(result[0].content).toBe('hello')
+ expect(result[0].filename).toBe('6-text-0.txt')
+ })📝 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
| it('parses array-shaped text outputs into text result items', () => { | |
| const output = makeOutput({ | |
| images: [{ filename: 'img.png', subfolder: '', type: 'output' }], | |
| text: ['first', 'second'] | |
| }) | |
| const result = parseNodeOutput('6', output) | |
| expect(result).toHaveLength(3) | |
| const textItems = result.filter((item) => item.isText) | |
| expect(textItems).toHaveLength(2) | |
| expect(textItems.map((item) => item.content)).toEqual(['first', 'second']) | |
| expect(textItems[0].nodeId).toBe('6') | |
| expect(textItems[0].filename).toBe('6-text-0.txt') | |
| }) | |
| it('parses object-shaped text entries from a synthesized preview_output', () => { | |
| const output = makeOutput({ | |
| text: [ | |
| { nodeId: '6', mediaType: 'text', content: 'from preview_output' } | |
| ] as unknown as NodeExecutionOutput['text'] | |
| }) | |
| const result = parseNodeOutput('6', output) | |
| expect(result).toHaveLength(1) | |
| expect(result[0].isText).toBe(true) | |
| expect(result[0].content).toBe('from preview_output') | |
| expect(result[0].filename).toBe('6-text-0.txt') | |
| }) | |
| it('parses array-shaped text outputs into text result items', () => { | |
| const output = makeOutput({ | |
| images: [{ filename: 'img.png', subfolder: '', type: 'output' }], | |
| text: ['first', 'second'] | |
| }) | |
| const result = parseNodeOutput('6', output) | |
| expect(result).toHaveLength(3) | |
| const textItems = result.filter((item) => item.isText) | |
| expect(textItems).toHaveLength(2) | |
| expect(textItems.map((item) => item.content)).toEqual(['first', 'second']) | |
| expect(textItems[0].nodeId).toBe('6') | |
| expect(textItems[0].filename).toBe('6-text-0.txt') | |
| }) | |
| it('parses single-string text outputs', () => { | |
| const result = parseNodeOutput('6', makeOutput({ text: 'hello' })) | |
| expect(result).toHaveLength(1) | |
| expect(result[0].isText).toBe(true) | |
| expect(result[0].content).toBe('hello') | |
| expect(result[0].filename).toBe('6-text-0.txt') | |
| }) | |
| it('parses object-shaped text entries from a synthesized preview_output', () => { | |
| const output = makeOutput({ | |
| text: [ | |
| { nodeId: '6', mediaType: 'text', content: 'from preview_output' } | |
| ] as unknown as NodeExecutionOutput['text'] | |
| }) | |
| const result = parseNodeOutput('6', output) | |
| expect(result).toHaveLength(1) | |
| expect(result[0].isText).toBe(true) | |
| expect(result[0].content).toBe('from preview_output') | |
| expect(result[0].filename).toBe('6-text-0.txt') | |
| }) |
🤖 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 `@src/stores/resultItemParsing.test.ts` around lines 116 - 147, Add a
regression test in resultItemParsing.test.ts for the direct single-string text
shape in parseNodeOutput; the current cases cover arrays and synthesized
preview_output objects, but not text: string. Extend the existing
parseNodeOutput expectations with a case like the other it blocks, asserting a
plain string becomes a single text result item with the correct content, nodeId,
and generated filename so the supported NodeExecutionOutput['text'] shape stays
covered.
Comment on lines
76
to
+83
| return Object.entries(nodeOutput) | ||
| .filter(([key, value]) => !METADATA_KEYS.has(key) && Array.isArray(value)) | ||
| .flatMap(([mediaType, items]) => | ||
| (items as unknown[]) | ||
| .filter(isResultItem) | ||
| .map((item) => new ResultItemImpl({ ...item, mediaType, nodeId })) | ||
| mediaType === 'text' | ||
| ? parseTextOutput(nodeId, items as unknown[]) | ||
| : (items as unknown[]) | ||
| .filter(isResultItem) | ||
| .map((item) => new ResultItemImpl({ ...item, mediaType, nodeId })) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Raw text: string outputs are still skipped.
NodeExecutionOutput['text'] still allows a plain string in
src/schemas/apiSchema.ts, but Line 77 only keeps array-valued entries. That
means a response like { text: 'hello' } never reaches parseTextOutput, so
single-string text outputs still disappear.
Suggested fix
export function parseNodeOutput(
nodeId: string | number,
nodeOutput: NodeExecutionOutput | null | undefined
): ResultItemImpl[] {
if (!nodeOutput) return []
return Object.entries(nodeOutput)
- .filter(([key, value]) => !METADATA_KEYS.has(key) && Array.isArray(value))
- .flatMap(([mediaType, items]) =>
- mediaType === 'text'
- ? parseTextOutput(nodeId, items as unknown[])
- : (items as unknown[])
+ .filter(([key]) => !METADATA_KEYS.has(key))
+ .flatMap(([mediaType, value]) =>
+ mediaType === 'text'
+ ? parseTextOutput(
+ nodeId,
+ Array.isArray(value) ? value : value === undefined ? [] : [value]
+ )
+ : Array.isArray(value)
+ ? (value as unknown[])
.filter(isResultItem)
.map((item) => new ResultItemImpl({ ...item, mediaType, nodeId }))
+ : []
)
}📝 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
| return Object.entries(nodeOutput) | |
| .filter(([key, value]) => !METADATA_KEYS.has(key) && Array.isArray(value)) | |
| .flatMap(([mediaType, items]) => | |
| (items as unknown[]) | |
| .filter(isResultItem) | |
| .map((item) => new ResultItemImpl({ ...item, mediaType, nodeId })) | |
| mediaType === 'text' | |
| ? parseTextOutput(nodeId, items as unknown[]) | |
| : (items as unknown[]) | |
| .filter(isResultItem) | |
| .map((item) => new ResultItemImpl({ ...item, mediaType, nodeId })) | |
| return Object.entries(nodeOutput) | |
| .filter(([key]) => !METADATA_KEYS.has(key)) | |
| .flatMap(([mediaType, value]) => | |
| mediaType === 'text' | |
| ? parseTextOutput( | |
| nodeId, | |
| Array.isArray(value) ? value : value === undefined ? [] : [value] | |
| ) | |
| : Array.isArray(value) | |
| ? (value as unknown[]) | |
| .filter(isResultItem) | |
| .map((item) => new ResultItemImpl({ ...item, mediaType, nodeId })) | |
| : [] | |
| ) |
🤖 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 `@src/stores/resultItemParsing.ts` around lines 76 - 83, Raw string text
outputs are being filtered out in resultItemParsing because the
Object.entries(...).filter(...) step only accepts array values, so
parseTextOutput never sees NodeExecutionOutput['text'] when it is a plain
string. Update the logic in resultItemParsing (the flatMap branch around
parseTextOutput and ResultItemImpl creation) to explicitly handle the 'text'
mediaType before the array check, accepting both string and array forms for text
while keeping the existing array-only handling for other media types.
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.
Addresses Comfy-Org/ComfyUI#14680