fix(send): refresh held tickets and compare the reviewed composition for All (#542)
A review held longer than the host's 30 s ticket lifetime would have refused the first node while sending the rest. Every node now takes a fresh ticket except the first of a pass taken in the same click, and that ticket must show the reviewed Before texts and composition. The revision is checked until something has been posted. Node review sections are labelled by their h4 heading.
This commit is contained in:
+9
-6
@@ -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 "#<id> <title>", 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
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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\.$/);
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 */ }
|
||||
|
||||
Reference in New Issue
Block a user