-
Notifications
You must be signed in to change notification settings - Fork 673
fix: keep Preview as Text rendering when the output payload has no text #14073
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3f3f58f
15d861a
04f1668
3ba36a0
cee098a
bbc6628
39e9bfe
8cb195c
3b876b7
7b0ab98
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,7 +55,10 @@ test.describe('Preview as Text node', () => { | |
| await comfyPage.menu.topbar.newWorkflowButton.click() | ||
| await comfyPage.searchBoxV2.addNode('Preview as Text') | ||
| const node = await comfyPage.vueNodes.getFixtureByTitle('Preview as Text') | ||
| const preview = node.root.locator('textarea') | ||
| const preview = comfyPage.vueNodes.getWidgetByName( | ||
| 'Preview as Text', | ||
| 'preview_text' | ||
| ) | ||
|
|
||
| await test.step('node previews execution result', async () => { | ||
| const id = await comfyPage.vueNodes.getNodeIdByTitle('Preview as Text') | ||
|
|
@@ -75,4 +78,59 @@ test.describe('Preview as Text node', () => { | |
| ) | ||
| } | ||
| ) | ||
|
|
||
| wstest( | ||
| 'renders the payloads users reported blank', | ||
| { tag: '@vue-nodes' }, | ||
| async ({ comfyPage, getWebSocket }) => { | ||
| const execution = new ExecutionHelper(comfyPage, await getWebSocket()) | ||
|
|
||
| await comfyPage.menu.topbar.newWorkflowButton.click() | ||
| await comfyPage.searchBoxV2.addNode('Preview as Text') | ||
| const preview = comfyPage.vueNodes.getWidgetByName( | ||
| 'Preview as Text', | ||
| 'preview_text' | ||
| ) | ||
| const id = await comfyPage.vueNodes.getNodeIdByTitle('Preview as Text') | ||
| const jobId = '' | ||
|
|
||
| const payloads = [ | ||
| ['compact JSON from an LLM node', '{"name":"Comfy","emoji":"🌟"}'], | ||
| ['JSON array', '[{"a": 1}, {"b": 2}]'], | ||
| ['markdown-fenced JSON', '```json\n{"name":"Comfy"}\n```'], | ||
| ['non-ASCII text', '你好,世界。'], | ||
| ['prompt with a trailing space', '"A red car" is a great prompt. '] | ||
| ] as const | ||
|
|
||
| for (const [label, text] of payloads) { | ||
| await test.step(label, async () => { | ||
| execution.executed(jobId, id, { text: [text] }) | ||
| await expect(preview).toHaveValue(text) | ||
| }) | ||
| } | ||
|
|
||
| await test.step('numeric output from Get Video Components', async () => { | ||
| execution.executed(jobId, id, { text: 23.976 }) | ||
| await expect(preview).toHaveValue('23.976') | ||
| }) | ||
|
|
||
| await test.step('null text does not wedge the widget', async () => { | ||
| // The shape the Cloud backend produced when it misclassified the text | ||
| // as a filename and dropped it from the payload (BE-3601). | ||
| execution.executed(jobId, id, { text: [null] }) | ||
| await expect(preview).toHaveValue('') | ||
|
|
||
| execution.executed(jobId, id, { text: ['recovered'] }) | ||
| await expect(preview).toHaveValue('recovered') | ||
| }) | ||
|
Comment on lines
+105
to
+125
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: every frame this spec sends yields the same assertion result against the pre-fix implementation, so it cannot detect the bug in its own commit.
The failure that needed fixing is Two ways to give the spec teeth, both covering ground the unit tests cannot reach:
Either way the six-row loop could collapse to one representative payload — those characters are already covered at the unit layer, and six browser round-trips is a lot for one identity path.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. Added a dedicated step in 91c26ea that sends |
||
|
|
||
| await test.step('output with no text key does not wedge the widget', async () => { | ||
| execution.executed(jobId, id, {}) | ||
| await expect(preview).toHaveValue('') | ||
|
|
||
| execution.executed(jobId, id, { text: ['recovered again'] }) | ||
| await expect(preview).toHaveValue('recovered again') | ||
| }) | ||
| } | ||
| ) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import type { LGraphNode } from '@/lib/litegraph/src/LGraphNode' | ||
| import { resolveNodeRootGraphId } from '@/lib/litegraph/src/litegraph' | ||
| import WidgetTextPreview from '@/renderer/extensions/vueNodes/widgets/components/WidgetTextPreview.vue' | ||
| import type { NodeExecutionOutput } from '@/schemas/apiSchema' | ||
| import type { CustomInputSpec } from '@/schemas/nodeDef/nodeDefSchemaV2' | ||
| import { app } from '@/scripts/app' | ||
| import { ComponentWidgetImpl, addWidget } from '@/scripts/domWidget' | ||
|
|
@@ -66,13 +67,19 @@ export function addTextPreviewWidgets(node: LGraphNode) { | |
| modeWidget.serialize = false | ||
| } | ||
|
|
||
| function toPreviewText(text: unknown): string { | ||
| if (text == null) return '' | ||
| if (Array.isArray(text)) | ||
| return text.filter((part) => part != null).join('\n\n') | ||
| return String(text) | ||
| } | ||
|
Comment on lines
+70
to
+75
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: two of the four branches are no-ops, and dropping them makes the contract easier to read.
function toPreviewText(text: unknown): string {
if (text == null) return ''
if (Array.isArray(text)) return text.filter((part) => part != null).join('\n\n')
return String(text)
}I diffed both versions over 19 inputs — nullish, Side benefit:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Applied in 731afa2. Dropped the |
||
|
|
||
| export function updateTextPreviewWidgets( | ||
| node: LGraphNode, | ||
| message: { text?: string | string[] } | ||
| message: NodeExecutionOutput | null | undefined | ||
| ) { | ||
| const preview = node.widgets?.find((w) => w.name === PREVIEW_WIDGET_NAME) | ||
| if (!preview) return | ||
|
|
||
| const text = message.text ?? '' | ||
| preview.value = Array.isArray(text) ? text.join('\n\n') : text | ||
| preview.value = toPreviewText(message?.text) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooooh, I like this