diff --git a/TESTING.md b/TESTING.md index ecf3d2d8..194a5710 100644 --- a/TESTING.md +++ b/TESTING.md @@ -4364,12 +4364,15 @@ the per-section hand-off panel, was retired by S12 (#329): every section is nati stay counted in the discovery sentence. One window only. Preview stays optional (D175): Preview changes shows one `section.prompt-sender-node` per node, headed "# ", in send order (node id, numeric). Apply previews every node first and writes nothing if any refuses ("#<id> <title>: <reason> - Nothing was sent."). Then it sends one node after another: the first with its reviewed ticket, each later - node with a fresh ticket, because our own write may move the graph version; that fresh preview skips the - discovery revision check but is refused if the node's text differs from the reviewed "Before". Refusals - are best effort: "Sent to 2 of 3 nodes in <window>. #12 <title>: <host's refusal>." A lost reply stops - the run, names that node as unknown and lists the nodes not sent. Closing the dialog stops after the - current node. Unit tests in `tests/js/prompt-sender.test.js`, a fake three-node host in + Nothing was sent."). Then it sends one node after another. Only the first node of a pass taken in that + click keeps its ticket; every other node, and every node of a held Preview (host tickets expire after + 30 s), takes a fresh ticket that is refused unless its Before texts and composition equal the reviewed + ones. That fresh preview checks the discovered revision until something has been posted, and not after, + since our own write may move the graph version. Refusals are best effort: "Sent to 2 of 3 nodes in + <window>. #12 <title>: <host's refusal>."; Apply stays available and a retry sends to every node again + (rewriting the same text). A lost reply stops the run, names that node as unknown and lists the nodes + not sent. Closing the dialog stops after the current node. The review sections are labelled by their + `h4` node heading. Unit tests in `tests/js/prompt-sender.test.js`, a fake three-node host in `tests/browser/prompt-inspector.spec.mjs`, real Classic and Nodes 2.0 widgets in `tests/comfyui/prompt-sender.spec.mjs`. - **D106** (Integrations, S10, Q8): the send bridge always asks for the ComfyUI window diff --git a/tests/browser/prompt-inspector.spec.mjs b/tests/browser/prompt-inspector.spec.mjs index 5d7725b2..53d9e74e 100644 --- a/tests/browser/prompt-inspector.spec.mjs +++ b/tests/browser/prompt-inspector.spec.mjs @@ -345,7 +345,7 @@ test('All eligible nodes previews each node, sends in node id order and names a await page.screenshot({path:testInfo.outputPath('send-all-option.png')}); await sender.getByRole('button',{name:'Preview changes',exact:true}).click(); const reviews=sender.locator('section.prompt-sender-node'); - await expect(reviews.locator('> h3')).toHaveText(['#3 Prompt Manager (+/-)','#7 Edited in ComfyUI','#12 Prompt Manager (+/-)']); + await expect(reviews.locator('> h4')).toHaveText(['#3 Prompt Manager (+/-)','#7 Edited in ComfyUI','#12 Prompt Manager (+/-)']); await expect(sender.getByRole('region',{name:'#12 Prompt Manager (+/-)',exact:true})).toContainText('Existing 12'); expect(await host.evaluate(()=>window.applied)).toEqual([]); await sender.getByRole('button',{name:'Apply to ComfyUI',exact:true}).click(); diff --git a/tests/comfyui/prompt-sender.spec.mjs b/tests/comfyui/prompt-sender.spec.mjs index d1c342b6..1ba71bdd 100644 --- a/tests/comfyui/prompt-sender.spec.mjs +++ b/tests/comfyui/prompt-sender.spec.mjs @@ -151,7 +151,7 @@ test('native image inspector previews and sends a pair into real visible widgets await expect(sender.getByRole('status')).toContainText('1 other Prompt Manager node cannot receive this prompt.'); await targets.selectOption({label:'All 2 eligible nodes'}); await sender.getByRole('button',{name:'Preview changes',exact:true}).click(); - await expect(sender.locator('section.prompt-sender-node > h3')).toHaveText([new RegExp(`^#${id} `),new RegExp(`^#${twinId} `)]); + await expect(sender.locator('section.prompt-sender-node > h4')).toHaveText([new RegExp(`^#${id} `),new RegExp(`^#${twinId} `)]); await apply.click(); // The tab title gains ComfyUI's unsaved mark once the workflow changed, so the window is matched loosely. await expect(sender.getByRole('status')).toHaveText(/^Sent to 2 of 2 nodes in .*ComfyUI\.$/); diff --git a/tests/js/prompt-sender.test.js b/tests/js/prompt-sender.test.js index c6b81c05..3e74eb8c 100644 --- a/tests/js/prompt-sender.test.js +++ b/tests/js/prompt-sender.test.js @@ -315,7 +315,7 @@ test('a node whose text changed after the preview pass is refused, not overwritt assert.deepEqual(s.calls.filter(call=>call[0]==='apply').map(call=>call[1]),['2','9']); assert.match(s.session.state.message,/^Sent to 2 of 3 nodes in Flow - ComfyUI\. #10 Node 10: Its prompt changed since the preview\./);s.session.dispose(); }); -test('Preview with All shows every node in order, and Apply sends the reviewed first ticket',async()=>{ +test('Preview with All shows every node in order, and Apply sends fresh tickets that match the review',async()=>{ const {createSendSession,ALL_NODES}=await source;const s=await allSession(createSendSession); s.session.selectTarget(ALL_NODES);await s.session.preview(); assert.equal(s.session.state.phase,'ready');assert.equal(s.session.state.preview,null); @@ -323,8 +323,9 @@ test('Preview with All shows every node in order, and Apply sends the reviewed f assert.match(s.session.state.message,/Sending to all 3 eligible nodes in Flow - ComfyUI\. Review the complete changes, then apply\./); const first=s.session.state.previews[0].preview.ticket; await s.session.apply(); - assert.deepEqual(order(s.calls),['preview 2','preview 9','preview 10','apply 2','preview 9','apply 9','preview 10','apply 10']); - assert.equal(s.calls.find(call=>call[0]==='apply')[2],first); + // A held review may have outlived the host's 30 s tickets, so even the first node takes a fresh one. + assert.deepEqual(order(s.calls),['preview 2','preview 9','preview 10','preview 2','apply 2','preview 9','apply 9','preview 10','apply 10']); + assert.notEqual(s.calls.find(call=>call[0]==='apply')[2],first);assert.equal(s.session.state.phase,'success'); s.session.selectTarget('2');assert.deepEqual(s.session.state.previews,[]);s.session.dispose(); }); test('the dialog offers All for two or more nodes and reviews each node under its name',async()=>{ @@ -343,3 +344,22 @@ test('the dialog offers All for two or more nodes and reviews each node under it assert.ok(dom.find(dialog,node=>node.textContent==='Sent to 2 of 2 nodes in Flow - ComfyUI.')); } finally {scope.dispose();dom.restore();} }); +test('an expired review still sends every node, and a held review checks the discovered revision first',async()=>{ + const {createSendSession,ALL_NODES}=await source;let reviewing=true; + // The host refuses every ticket taken during the review, as after its 30 s expiry. + const s=await allSession(createSendSession,{preview:(id,calls)=>({ticket:`t-${id}-${reviewing?'old':calls.length}`,target:{id},fields:[{role:'positive',before:'old',after:'new'}]}),apply:id=>({targetId:id,values:{positive:'new'}})}); + const realApply=s.client.apply;s.client.apply=async(host,ticket,options)=>{if(ticket.endsWith('-old'))throw Error('Preview expired or unavailable; preview again');return realApply(host,ticket,options);}; + s.session.selectTarget(ALL_NODES);await s.session.preview();reviewing=false;await s.session.apply(); + assert.equal(s.session.state.message,'Sent to 3 of 3 nodes in Flow - ComfyUI.');s.session.dispose(); + const moved=await allSession(createSendSession,{revision:'1:4',preview:(id,calls)=>({ticket:`t-${id}`,revision:calls.length>3?'1:5':'1:4',target:{id},fields:[{role:'positive',before:'old',after:'new'}]})}); + moved.session.selectTarget(ALL_NODES);await moved.session.preview();assert.equal(moved.session.state.phase,'ready');await moved.session.apply(); + assert.match(moved.session.state.message,/^Sent to 0 of 3 nodes in Flow - ComfyUI\. #2 Node 2: The workflow changed since this window was found/); + assert.equal(moved.calls.filter(call=>call[0]==='apply').length,0);moved.session.dispose(); +}); +test('a changed prepend or append after the review refuses that node',async()=>{ + const {createSendSession,ALL_NODES}=await source;let edited=false; + const s=await allSession(createSendSession,{preview:id=>({ticket:`t-${id}`,target:{id},fields:[{role:'positive',before:'old',after:'new'}],composition:{prepend:id==='9'&&edited?'typed':'trigger',append:'',before:'trigger old',after:'trigger new'}}),apply:id=>{edited=true;return{targetId:id,values:{positive:'new'}};}}); + s.session.selectTarget(ALL_NODES);await s.session.apply(); + assert.deepEqual(s.calls.filter(call=>call[0]==='apply').map(call=>call[1]),['2','10']); + assert.match(s.session.state.message,/^Sent to 2 of 3 nodes in Flow - ComfyUI\. #9 Node 9: Its prompt changed since the preview\./);s.session.dispose(); +}); diff --git a/web/js/workspace/prompt-sender.mjs b/web/js/workspace/prompt-sender.mjs index 609e3c93..196ed480 100644 --- a/web/js/workspace/prompt-sender.mjs +++ b/web/js/workspace/prompt-sender.mjs @@ -77,6 +77,8 @@ export function createSendSession({client, roles:rawRoles, onChange}) { function checkAcknowledgement(acknowledgement,targetId) { if(acknowledgement?.targetId!==targetId || !acknowledgement.values || Object.keys(acknowledgement.values).length!==Object.keys(roles).length || Object.keys(roles).some(role=>!Object.hasOwn(acknowledgement.values,role) || acknowledgement.values[role]!==roles[role])) throw Error('ComfyUI returned an unexpected acknowledgement.'); } + const reviewedText = change => JSON.stringify([change.fields.map(field=>[field.role,field.before]).sort(),change.composition ?? null]); + const sameChanges = (reviewed,fresh) => reviewedText(reviewed)===reviewedText(fresh); /** Q542-2 C: every node of "All" previews before anything is written; the first refusal names its node. */ async function previewEach(request,hostId,targets) { const reviewed=[]; @@ -92,21 +94,25 @@ export function createSendSession({client, roles:rawRoles, onChange}) { } return reviewed; } - /** Sends the reviewed nodes one after another (each host apply checks its own preview), best effort past refusals. */ - async function applyEach(request,hostId,reviewed,where) { - const sent=[],refused=[]; + /** + * Sends the reviewed nodes one after another, best effort past refusals. Only the first node of a pass taken in this + * click keeps its ticket: a held review may have expired at the host (30 s), and our own writes may make later + * tickets stale, so every other node takes a fresh ticket that must show exactly the reviewed text. + */ + async function applyEach(request,hostId,reviewed,where,held) { + const sent=[],refused=[];let written=false; for (const [index,{target,preview}] of reviewed.entries()) { let posted=false; try { let ticket=preview.ticket; - if (index>0) { - // A ticket taken before our earlier writes may be stale, so take a fresh one, but only for the text reviewed. - const fresh=await fetchPreview(request,hostId,target,false); + if (index>0 || held) { + // Until something was posted the revision is still the discovered one; after that our own write may move it. + const fresh=await fetchPreview(request,hostId,target,!written); if (!fresh) return; - if (fresh.fields.some(field=>preview.fields.find(item=>item.role===field.role)?.before!==field.before)) throw Error('Its prompt changed since the preview. Preview again.'); + if (!sameChanges(preview,fresh)) throw Error('Its prompt changed since the preview. Preview again.'); ticket=fresh.ticket; } - posted=true; + posted=written=true; const acknowledgement=await client.apply(hostId,ticket,{signal:request.signal}); if (!current(request)) return; checkAcknowledgement(acknowledgement,target.id); @@ -185,7 +191,7 @@ export function createSendSession({client, roles:rawRoles, onChange}) { let reviewed; try { reviewed=held?.length ? held : await previewEach(request,hostId,targets); } catch(error) { if(current(request)) emit({phase:'error',message:error.message}); return; } - if (reviewed) await applyEach(request,hostId,reviewed,windowLabel); + if (reviewed) await applyEach(request,hostId,reviewed,windowLabel,Boolean(held?.length)); return; } const [target]=targets; @@ -250,7 +256,7 @@ export function createPromptSender({scope,baseUrl=window.ApiClient.resolveUrl('/ inspect.disabled=true;apply.disabled=true; const actions=element('footer',undefined,'prompt-sender-actions');actions.append(find,inspect,apply); dialog.append(header,description,controls,status,preview,actions);document.body.append(dialog); - let session,closed=false; + let session,closed=false,headingIds=0; function dispose() { if(closed) return;closed=true;local.dispose();session?.dispose(); if(dialog.open) dialog.close();dialog.remove(); @@ -268,7 +274,7 @@ export function createPromptSender({scope,baseUrl=window.ApiClient.resolveUrl('/ select.replaceChildren(...[["",placeholder],...entries].map(([value,label])=>{const option=element('option',label);option.value=value;return option;}));select.value=value; } function textBlock(parent,label,value,level=4) { - parent.append(element(`h${level}`,label));const text=element('pre');text.tabIndex=0;text.append(element('code',value === '' ? '(empty)' : value));parent.append(text); + parent.append(element(`h${Math.min(level,6)}`,label));const text=element('pre');text.tabIndex=0;text.append(element('code',value === '' ? '(empty)' : value));parent.append(text); } function render(state) { if(closed) return; @@ -291,10 +297,11 @@ export function createPromptSender({scope,baseUrl=window.ApiClient.resolveUrl('/ } if(state.previews?.length) { preview.append(element('h3','Review changes')); - // One section per node of "All", in the order they will be sent. + // One section per node of "All", in the order they will be sent, named by its heading. for(const item of state.previews) { - const node=element('section',undefined,'prompt-sender-node');node.setAttribute('aria-label',item.label); - node.append(element('h3',item.label));changes(node,item.preview,4);preview.append(node); + const node=element('section',undefined,'prompt-sender-node');const heading=element('h4',item.label); + heading.id=`prompt-sender-node-${++headingIds}`;node.setAttribute('aria-labelledby',heading.id); + node.append(heading);changes(node,item.preview,5);preview.append(node); } } } diff --git a/web/js/workspace/workspace.css b/web/js/workspace/workspace.css index 9dfce5b4..05b569be 100644 --- a/web/js/workspace/workspace.css +++ b/web/js/workspace/workspace.css @@ -418,7 +418,7 @@ pre.prompt-text { font: inherit; font-size: 12px; line-height: 1.75; margin-bloc .prompt-sender-field :where(pre, textarea), .prompt-sender-composition { white-space: pre-wrap; overflow-wrap: anywhere; font: inherit; font-size: 12px; line-height: 1.65; max-inline-size: 100%; /* D48 */ } /* D191 (#542): each node of "All eligible nodes" reviews under its own heading. */ .prompt-sender-node { border-block-start: 1px solid var(--ca-line); padding-block-start: 14px; margin-block: 18px; min-inline-size: 0; /* D191 */ } -.prompt-sender-node > h3 { font-size: 15px; /* D191 */ } +.prompt-sender-node > h4 { font-size: 15px; /* D191 */ } .prompt-sender-field textarea { inline-size: 100%; min-block-size: 100px; resize: vertical; /* D48 */ } .prompt-sender-composition { padding: 12px; background: var(--ca-field); border-radius: 6px; /* D48 */ } .prompt-sender-actions { justify-content: flex-end; margin-block-start: 18px; /* D48 */ }