From fe338c5b09bc2c84d635ca7d178d525616cbddb6 Mon Sep 17 00:00:00 2001 From: "Dr.Lt.Data" Date: Sun, 1 Nov 2026 02:15:08 +0900 Subject: [PATCH] Fix legacy SAM save and input handling --- docs/E2E_TEST_STRATEGY.md | 20 ++++- js/impact-sam-editor.js | 10 +-- modules/impact/impact_server.py | 3 +- tests/README.md | 9 +- tests/e2e_ui.py | 3 +- tests/legacy_sam.test.mjs | 96 +++++++++++++++++++++ tests/test_e2e_legacy_sam.py | 143 ++++++++++++++++++++++++++----- tests/test_e2e_widget_catalog.py | 4 +- 8 files changed, 254 insertions(+), 34 deletions(-) create mode 100644 tests/legacy_sam.test.mjs diff --git a/docs/E2E_TEST_STRATEGY.md b/docs/E2E_TEST_STRATEGY.md index ba05d1b3..cec811e6 100644 --- a/docs/E2E_TEST_STRATEGY.md +++ b/docs/E2E_TEST_STRATEGY.md @@ -129,14 +129,32 @@ python -m pytest tests/test_e2e_legacy_sam.py -v ``` Install the model configured by `sam_editor_model` on the isolated server. -The detection tests skip only when `IMPACT_E2E_SAM_IMAGE` is unset. They forward +Detection tests skip when `IMPACT_E2E_SAM_IMAGE` is unset; named-restoration +cases also skip on older frontends without that setting. Other native reloads +use positional restoration on those frontends. The tests forward the real detector response without substituting a mask, compare saved outputs and browser previews, and reload native exports in positional and named modes. The second edit starts from the first saved temporary image and checks that two clicks produce exactly one positive and one negative point. The separate menu test checks repeated opening and native loading without running detection. +Detection starts from both an uploaded input and a SaveImage output in a nested +subfolder, with both native restoration modes. Event checks cover keyboard +shortcuts, cancellation without changing the image widget, resizing and opening +a different image. Invalid type/path requests are rejected twice, between valid +preparation requests, so a rejected request cannot be cached as successful. No Vue MaskEditor extension API is required. +For the frontend 1.42.15 / backend 0.19.3 compatibility comparison, select +`-k 'not rejects_invalid'`. Those three API checks target the current backend's +path resolver; the older-pair comparison exercises the legacy UI and image flow. + +`node --experimental-vm-modules --test tests/legacy_sam.test.mjs` checks that +resizing redraws only the current image after reopening and that mask upload +sets the selected image reference without changing another image. It loads the shipped +SAM module with controlled UI exports and canvas/event objects, exercising both +global UI exports and the dynamic UI-module fallback. This checks those import +contracts; it does not substitute for browser testing an older frontend release. + The all-node catalog requires **Full Cache mode** because the trusted snapshot includes wildcard picker values displaying `Select Wildcard 🟢 Full Cache`. In the test copy's `impact-pack.ini`, set `wildcard_cache_limit_mb` above the diff --git a/js/impact-sam-editor.js b/js/impact-sam-editor.js index 845325e3..551e3864 100644 --- a/js/impact-sam-editor.js +++ b/js/impact-sam-editor.js @@ -65,8 +65,8 @@ async function uploadMask(filepath, formData) { ComfyApp.clipspace.imgs[ComfyApp.clipspace['selectedIndex']] = new Image(); ComfyApp.clipspace.imgs[ComfyApp.clipspace['selectedIndex']].src = `view?filename=${filepath.filename}&type=${filepath.type}`; - if(ComfyApp.clipspace.images) - ComfyApp.clipspace.images[ComfyApp.clipspace['selectedIndex']] = filepath; + ComfyApp.clipspace.images ??= []; + ComfyApp.clipspace.images[ComfyApp.clipspace['selectedIndex']] = filepath; ComfyApp.clipspace_invalidate_handler?.(); } @@ -421,6 +421,7 @@ class ImpactSamEditorDialog extends ComfyDialog { static handleKeyDown(event) { const self = ImpactSamEditorDialog.instance; + const brush = self.brush; if (event.key === '=') { // positive brush.style.backgroundColor = "blue"; brush.style.outline = "2px solid pink"; @@ -588,14 +589,11 @@ class ImpactSamEditorDialog extends ComfyDialog { "type": "temp", }; - if(ComfyApp.clipspace.images) - ComfyApp.clipspace.images[0] = item; - if(ComfyApp.clipspace.widgets) { const index = ComfyApp.clipspace.widgets.findIndex(obj => obj.name === 'image'); if(index >= 0) - ComfyApp.clipspace.widgets[index].value = `${filename} [temp]`; + ComfyApp.clipspace.widgets[index].value = item; } const dataURL = save_canvas.toDataURL(); diff --git a/modules/impact/impact_server.py b/modules/impact/impact_server.py index b22f6048..ab704edb 100644 --- a/modules/impact/impact_server.py +++ b/modules/impact/impact_server.py @@ -67,8 +67,6 @@ async def sam_prepare(request): # already loaded: skip -- prevent redundant loading return web.Response(status=200) - last_prepare_data = data - model_name = 'sam_vit_b_01ec64.pth' if data['sam_model_name'] == 'auto': model_name = impact.config.get_config()['sam_editor_model'] @@ -95,6 +93,7 @@ async def sam_prepare(request): except ValueError: return web.Response(status=400) + last_prepare_data = data thread = threading.Thread(target=async_prepare_sam, args=(model_path, filename,)) thread.start() diff --git a/tests/README.md b/tests/README.md index 0cb2619e..2f1cd422 100644 --- a/tests/README.md +++ b/tests/README.md @@ -36,7 +36,14 @@ a running test server and are separate from the curl-based API integration tests and Clipspace button without importing the removed `ClipspaceDialog` module. Set `IMPACT_E2E_SAM_IMAGE` and install the configured SAM model to include real detection, repeated saving/pasting, RGB preservation, and native workflow reload -in both widget restoration modes. +in both widget restoration modes, starting from input and nested output images. +The suite also checks keyboard shortcuts, cancellation, resizing, changing the +image, and repeated invalid-path rejection. Run +`node --experimental-vm-modules --test tests/legacy_sam.test.mjs` for current-image +resizing and saved image references with controlled browser objects, including +global and fallback UI exports. +Older frontends without named widget restoration skip the two named detection +cases; their other native reloads use positional restoration. With Playwright and Chrome installed, `python3 -m pytest tests/test_e2e_ui.py` also checks failure cleanup and JavaScript errors during artifact saving using intercepted browser requests, without a server. diff --git a/tests/e2e_ui.py b/tests/e2e_ui.py index c77278b6..c4522b6d 100644 --- a/tests/e2e_ui.py +++ b/tests/e2e_ui.py @@ -46,7 +46,8 @@ class ComfyUI: self.page.mouse.dblclick(x, y, delay=100) search = self.page.get_by_placeholder("Add a node...", exact=True) search.fill(node_type) - label = self.page.locator("span.truncate").filter(has_text=re.compile(rf"^\s*{re.escape(title)}\s*$")) + label = self.page.locator("span.truncate, .option-container .font-semibold > span").filter( + has_text=re.compile(rf"^\s*{re.escape(title)}\s*$")) self.page.get_by_role("option").filter(has=label).click() expect(search).not_to_be_visible() self.page.get_by_role("dialog").wait_for(state="hidden") diff --git a/tests/legacy_sam.test.mjs b/tests/legacy_sam.test.mjs new file mode 100644 index 00000000..945317dd --- /dev/null +++ b/tests/legacy_sam.test.mjs @@ -0,0 +1,96 @@ +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import test from 'node:test'; +import vm from 'node:vm'; + +const source = readFileSync(new URL('../js/impact-sam-editor.js', import.meta.url), 'utf8'); + +async function loadSam(legacyUI = false) { + const window = new EventTarget(); + Object.assign(window, { innerWidth: 1600, innerHeight: 1000 }); + const ui = { ComfyDialog: class {}, $el: () => {} }; + if (!legacyUI) window.comfyAPI = { ui }; + const registered = []; + const context = vm.createContext({ + window, Event, URL, + Image: class { + set src(value) { + this.url = String(value); + [this.width, this.height] = this.url.includes('portrait') ? [700, 1300] : [1400, 800]; + } + get src() { return this.url; } + }, + }); + const ComfyApp = {}; + const imports = { + '../../scripts/app.js': { app: { registerExtension: extension => registered.push(extension) }, ComfyApp }, + '../../scripts/api.js': { api: { fetchApi: async () => ({ ok: true }) } }, + '../../scripts/ui.js': ui, + }; + const requests = []; + async function load(specifier) { + requests.push(specifier); + assert.ok(imports[specifier], `Unexpected import: ${specifier}`); + const values = imports[specifier]; + const module = new vm.SyntheticModule(Object.keys(values), function () { + for (const [key, value] of Object.entries(values)) this.setExport(key, value); + }, { context }); + await module.link(load); + await module.evaluate(); + return module; + } + const module = new vm.SourceTextModule(`${source}\nexport { ImpactSamEditorDialog, uploadMask };`, { + context, importModuleDynamically: load, + }); + await module.link(load); + await module.evaluate(); + assert.deepEqual(registered.map(extension => extension.name), ['Comfy.Impact.SAMEditor']); + assert.equal(requests.includes('../../scripts/ui.js'), legacyUI); + return { module, window, ComfyApp }; +} + +for (const legacyUI of [false, true]) { + test(`resize redraws only the current image (${legacyUI ? 'UI module fallback' : 'global UI exports'})`, async () => { + const { module, window } = await loadSam(legacyUI); + const draws = []; + const imageCanvas = { + width: 0, height: 0, offsetTop: 0, offsetLeft: 0, + get clientWidth() { return this.width; }, + getContext: () => ({ drawImage: image => draws.push(image.src) }), + }; + const dialog = Object.create(module.namespace.ImpactSamEditorDialog.prototype); + Object.assign(dialog, { + maskCanvas: { style: {} }, maskCtx: {}, + pointsCanvas: { style: {} }, pointsCtx: { canvas: {} }, + prompt_points: [], saveButton: {}, + }); + for (const name of ['landscape', 'portrait']) { + dialog.setImages(`http://localhost/${name}.png`, imageCanvas, dialog.pointsCanvas); + dialog.image.onload(); + } + draws.length = 0; + window.innerWidth = 1100; + window.dispatchEvent(new Event('resize')); + assert.equal(draws.length, 1, 'Resize redrew images from previous editing sessions'); + assert.match(draws[0], /portrait\.png/); + }); +} + +for (const selectedIndex of [0, 1]) { + test(`upload retains image references (selected ${selectedIndex}, metadata ${selectedIndex ? 'present' : 'absent'})`, async () => { + const { module, ComfyApp } = await loadSam(); + const original = { filename: 'original.png', subfolder: '', type: 'input' }; + const otherImage = { src: 'http://localhost/other.png' }; + ComfyApp.clipspace = { selectedIndex, imgs: [otherImage, otherImage] }; + if (selectedIndex) ComfyApp.clipspace.images = [original, original]; + let refreshes = 0; + ComfyApp.clipspace_invalidate_handler = () => { refreshes++; }; + const saved = { filename: 'saved.png', subfolder: '', type: 'temp' }; + await module.namespace.uploadMask(saved, {}); + assert.deepEqual(ComfyApp.clipspace.images?.[selectedIndex], saved, + 'Saved image metadata is required when the core refreshes node previews'); + assert.equal(ComfyApp.clipspace.imgs[1 - selectedIndex], otherImage); + if (selectedIndex) assert.equal(ComfyApp.clipspace.images[0], original); + assert.equal(refreshes, 1); + }); +} diff --git a/tests/test_e2e_legacy_sam.py b/tests/test_e2e_legacy_sam.py index 5c7d1dad..642b0562 100644 --- a/tests/test_e2e_legacy_sam.py +++ b/tests/test_e2e_legacy_sam.py @@ -45,21 +45,28 @@ def node_menu(browser, node_id, name): def source_image(browser, path): source = browser.add_node("Load Image", "LoadImage", 220, 200) + upload_image(browser, source, path) + return source + + +def upload_image(browser, source, path): button = browser.app.evaluate("""(app, id) => app.graph.getNodeById(id).widgets.find(widget => widget.type === 'button').name """, source) with browser.page.expect_file_chooser() as chooser: browser.page.mouse.click(*browser.point("LoadImage", widget=button)) - chooser.value.set_files(path) - browser.page.wait_for_function("""(app) => { + with browser.page.expect_response(lambda response: response.url.endswith("/upload/image")) as uploaded: + chooser.value.set_files(path) + assert uploaded.value.ok + filename = uploaded.value.json()["name"] + browser.page.wait_for_function("""({app, filename}) => { const image = app.graph._nodes.find(node => node.type === 'LoadImage').imgs?.[0]; - return image?.complete && image.naturalWidth > 0; - }""", arg=browser.app) - return source + return image?.complete && image.naturalWidth > 0 && new URL(image.src).searchParams.get('filename') === filename; + }""", arg={"app": browser.app, "filename": filename}) def clipspace_menu(browser): - browser.page.get_by_test_id("comfy-menu-button").click() + browser.page.locator('[data-testid="comfy-menu-button"], .comfy-menu-button-wrapper').click() browser.page.get_by_role("menuitem", name="Edit", exact=True).hover() browser.page.get_by_role("menuitem", name="Clipspace", exact=True).click() @@ -81,8 +88,15 @@ def widgets(browser): }))""") +def supports_named_restore(browser): + return browser.app.evaluate("app => 'Comfy.Workflow.NamedValuesRestore' in app.ui.settings.settingsLookup") + + def reload_native(browser, artifacts, named): - setting(browser, "Restore widget values", "Comfy.Workflow.NamedValuesRestore", named) + if supports_named_restore(browser): + setting(browser, "Restore widget values", "Comfy.Workflow.NamedValuesRestore", named) + else: + assert not named, "This frontend only provides positional widget restoration" before = widgets(browser) path = artifacts / "edited-workflow.json" export_workflow(browser, path) @@ -105,6 +119,9 @@ def test_legacy_sam_context_menu_and_clipspace_reopen(ui): expect(empty_dialog).to_be_visible() expect(browser.page.get_by_role("button", name="Impact SAM Detector", exact=True)).not_to_be_visible() empty_dialog.get_by_role("button", name="Close", exact=True).click() + expect(empty_dialog).not_to_be_visible() + browser.page.mouse.click(600, 400) + expect(browser.page.get_by_role("menuitem", name="Edit", exact=True)).not_to_be_visible() path = artifacts / "source.png" Image.new("RGB", (256, 256), (64, 128, 192)).save(path) source = source_image(browser, path) @@ -121,28 +138,107 @@ def test_legacy_sam_context_menu_and_clipspace_reopen(ui): expect(browser.page.locator("#pointsCanvas")).to_be_visible() browser.page.get_by_role("button", name="Cancel", exact=True).click() content.get_by_role("button", name="Close", exact=True).click() - for named in (False, True): + for named in ((False, True) if supports_named_restore(browser) else (False,)): reload_native(browser, artifacts, named) +def test_legacy_sam_keyboard_cancel_resize_and_change_image(ui): + browser, artifacts = ui + assert_sam_loaded(browser) + first = artifacts / "landscape.png" + second = artifacts / "portrait.png" + Image.new("RGB", (1400, 800), (64, 128, 192)).save(first) + Image.new("RGB", (700, 1300), (192, 64, 128)).save(second) + source = source_image(browser, first) + for path, size, color in [(first, (1400, 800), [64, 128, 192]), + (second, (700, 1300), [192, 64, 128])]: + if path == second: + upload_image(browser, source, path) + before = widgets(browser) + for _ in range(2): + node_menu(browser, source, "Open in SAM Detector") + canvas = browser.page.locator("#pointsCanvas") + expect(canvas).to_be_visible() + expect(browser.page.get_by_role("button", name="Save to node", exact=True)).to_be_enabled() + assert canvas.evaluate("canvas => [...canvas.getContext('2d').getImageData(0, 0, canvas.width, canvas.height).data].every(value => value === 0)"), "Reopening retained prompt points" + brush = browser.page.locator("#sam-brush") + browser.page.keyboard.press("-") + expect(brush).to_have_css("background-color", "rgb(255, 0, 0)") + browser.page.keyboard.press("=") + expect(brush).to_have_css("background-color", "rgb(0, 0, 255)") + for width, height in [(1100, 900), (2200, 1300)]: + browser.page.set_viewport_size({"width": width, "height": height}) + browser.page.wait_for_function("""({width, height}) => { + const image = document.getElementById('imageCanvas'); + return image.width === width - 250 && image.height === height - 200; + }""", arg={"width": width, "height": height}) + box = canvas.bounding_box() + assert box["width"] / box["height"] == pytest.approx(size[0] / size[1], abs=0.005) + pixel = browser.page.locator("#imageCanvas").evaluate("canvas => [...canvas.getContext('2d').getImageData(10, 10, 1, 1).data].slice(0, 3)") + assert pixel == color, "Resize drew a previously edited image" + canvas.click(position={"x": 60, "y": 60}) + assert canvas.evaluate("canvas => canvas.getContext('2d').getImageData(60, 60, 1, 1).data[3]") > 0 + browser.page.get_by_role("button", name="Cancel", exact=True).click() + expect(canvas).not_to_be_visible() + assert widgets(browser) == before, "Cancel changed the source image" + browser.page.keyboard.press("-") + expect(brush).to_have_css("background-color", "rgb(0, 0, 255)") + reload_native(browser, artifacts, path == second and supports_named_restore(browser)) + + +@pytest.mark.parametrize("invalid", [ + {"type": "unknown"}, + {"filename": "../outside.png"}, + {"subfolder": "../../outside"}, +], ids=["unknown-type", "filename-escape", "subfolder-escape"]) +def test_legacy_sam_rejects_invalid_path_on_retry(ui, invalid): + browser, artifacts = ui + assert_sam_loaded(browser) + path = artifacts / "valid-source.png" + Image.new("RGB", (256, 256), (64, 128, 192)).save(path) + source = source_image(browser, path) + with browser.page.expect_response(lambda response: response.url.endswith("/sam/prepare")) as prepared: + node_menu(browser, source, "Open in SAM Detector") + assert prepared.value.status == 200, "Valid SAM preparation must succeed before testing rejection" + valid = prepared.value.request.post_data_json + browser.page.get_by_role("button", name="Cancel", exact=True).click() + data = dict(valid) + data.update(invalid) + for _ in range(2): + response = browser.page.request.post(f"{browser.base_url}/sam/prepare", data=data) + assert response.status == 400, "Rejected SAM preparation was cached as successful" + response = browser.page.request.post(f"{browser.base_url}/sam/prepare", data=valid) + assert response.status == 200, "Rejected paths prevented valid preparation" + + @pytest.mark.skipif(not os.environ.get("IMPACT_E2E_SAM_IMAGE"), reason="Set IMPACT_E2E_SAM_IMAGE and install the configured SAM model") @pytest.mark.parametrize("named", [False, True], ids=["positional", "named"]) -def test_legacy_sam_detect_save_and_native_reload(ui, named): +@pytest.mark.parametrize("origin", ["input", "output_subfolder"]) +def test_legacy_sam_detect_save_and_native_reload(ui, named, origin): browser, artifacts = ui assert_sam_loaded(browser) + if named and not supports_named_restore(browser): + pytest.skip("This frontend does not provide named widget restoration") source = source_image(browser, os.environ["IMPACT_E2E_SAM_IMAGE"]) - output = browser.add_node("Preview Image", "PreviewImage", 750, 200) + output_type, mask_type = ("PreviewImage", "SaveImage") if origin == "input" else ("SaveImage", "PreviewImage") + titles = {"PreviewImage": "Preview Image", "SaveImage": "Save Image"} + output = browser.add_node(titles[output_type], output_type, 750, 200) convert = browser.add_node("Convert Mask to Image", "MaskToImage", 220, 800) - mask_output = browser.add_node("Save Image", "SaveImage", 750, 800) - browser.connect("LoadImage", "PreviewImage") + mask_output = browser.add_node(titles[mask_type], mask_type, 750, 800) + browser.connect("LoadImage", output_type) browser.page.mouse.move(*browser.point("LoadImage", slot=1)) browser.page.mouse.down() browser.page.mouse.move(*browser.point("MaskToImage", is_input=True), steps=12) browser.page.mouse.up() browser.page.wait_for_function("""(app) => app.graph._nodes.find(node => node.type === 'MaskToImage').inputs[0].link !== null""", arg=browser.app) - browser.connect("MaskToImage", "SaveImage") + browser.connect("MaskToImage", mask_type) + if origin == "output_subfolder": + browser.page.mouse.dblclick(*browser.point("SaveImage", widget="filename_prefix"), delay=100) + editor = browser.page.locator("input.value:visible, textarea.value:visible") + editor.fill("impact-legacy-sam/nested/source") + editor.press("Enter") original = browser.images(browser.run())[output][0] responses = [] @@ -154,15 +250,20 @@ def test_legacy_sam_detect_save_and_native_reload(ui, named): browser.page.route("**/sam/detect", capture) previous = None for index in range(2): - if index == 0: - node_menu(browser, source, "Open in SAM Detector") - else: - node_menu(browser, source, "Copy (Clipspace)") - content = open_clipspace(browser) - content.get_by_role("button", name="Impact SAM Detector", exact=True).click() + direct = index == 0 and origin == "input" + with browser.page.expect_request(lambda request: request.url.endswith("/sam/prepare")) as prepared: + if direct: + node_menu(browser, source, "Open in SAM Detector") + else: + node_menu(browser, output if index == 0 else source, "Copy (Clipspace)") + content = open_clipspace(browser) + content.get_by_role("button", name="Impact SAM Detector", exact=True).click() + if index == 0 and origin == "output_subfolder": + assert prepared.value.post_data_json["type"] == "output" + assert prepared.value.post_data_json["subfolder"] == "impact-legacy-sam/nested" canvas = browser.page.locator("#pointsCanvas") expect(canvas).to_be_visible() - save = browser.page.get_by_role("button", name="Save to node" if index == 0 else "Save", exact=True) + save = browser.page.get_by_role("button", name="Save to node" if direct else "Save", exact=True) expect(save).to_be_enabled() box = canvas.bounding_box() browser.page.mouse.click(box["x"] + box["width"] * 0.5, box["y"] + box["height"] * (0.5 if index == 0 else 0.25)) @@ -185,7 +286,7 @@ def test_legacy_sam_detect_save_and_native_reload(ui, named): save.click() assert uploaded.value.ok expect(canvas).not_to_be_visible() - if index: + if not direct: browser.page.wait_for_function("""() => { const image = document.getElementById('clipspace_preview'); return image.complete && image.naturalWidth > 0 && diff --git a/tests/test_e2e_widget_catalog.py b/tests/test_e2e_widget_catalog.py index c9fc86fe..46397cd4 100644 --- a/tests/test_e2e_widget_catalog.py +++ b/tests/test_e2e_widget_catalog.py @@ -112,8 +112,8 @@ def widget_state(ui, node_id): def export_workflow(ui, path): page = ui.page - page.get_by_role("button", name="Graph mode, workflow actions", exact=True).click() - page.get_by_role("menuitem", name="Export", exact=True).click() + page.locator('button[aria-label$="workflow actions" i]').click() + page.get_by_text("Export", exact=True).filter(visible=True).click() dialog = page.get_by_role("dialog") dialog.get_by_role("textbox").fill(path.stem) with page.expect_download() as download: