fix(extensions): honor pickerIntent on ExtensionNode string params - #258
Open
Souptik96 wants to merge 1 commit into
Open
fix(extensions): honor pickerIntent on ExtensionNode string params#258Souptik96 wants to merge 1 commit into
Souptik96 wants to merge 1 commit into
Conversation
ExtensionNode's string-param browse button called window.electron.fs.selectDirectory() unconditionally, so a manifest asking for a file dialog with pickerIntent: "image" still got a folder dialog. The Generate panel's copy of the same control had the identical bug. Route both through a shared resolver that maps a param's pickerIntent to one of the dialogs the preload API already exposes (image, mesh, text), falling back to the folder picker when the field is unset or holds a value this build does not know -- so existing manifests are unaffected. The button glyph and its accessible name now follow the resolved intent. Refs lightningpixel#155
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
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.
Fixes #155.
This implements the fix exactly as proposed in the issue, which @Lorchie already endorsed ("+1 on the fix as proposed"): route the string param's browse button by
pickerIntentinstead of always callingselectDirectory(), and swap the button glyph to match.Root cause
ParamControlinsrc/areas/workflows/nodes/ExtensionNode.tsxrendered everytype: "string"param with a browse button whose handler was hardcoded:Nothing consulted the param definition, so
pickerIntent: "image"was silently dropped and an extension author could not get a file dialog.ParamFieldinsrc/areas/generate/components/WorkflowPanel.tsxis a second copy of the same control (it renders the sameParamSchemain the Generate panel) and had the identical bug, so fixing only the workflow canvas would have left the Generate tab still opening a folder picker.One thing the issue expected turned out to be unnecessary: the image dialog is already exposed through preload as
window.electron.fs.selectImage()(electron/preload/electron-api.ts), backed byfs:selectImageinipc-handlers.ts. So this PR adds no new IPC surface at all.Fix
src/shared/utils/paramPicker.ts—resolvePickerIntent(param)maps a param to one offolder | image | mesh | text, andopenParamPicker(param, fs)calls the matching dialog that preload already provides (selectImage,selectMeshFile,selectTextFile,selectDirectory).pickerIntentresolves to'folder', and so does an unrecognised value — a manifest written for a future intent degrades to today's behavior on an older build instead of breaking the button. Every existing manifest is unaffected.openParamPicker. The glyph comes from a newPickerIcon(reusing the existing image / mesh / text node glyphs so the icons match the rest of the app), and the button finally has atitle/aria-label— it previously had no accessible name at all.ParamSchemagainspickerIntent?: PickerIntent.Two small judgement calls, both easy to drop if you'd rather not carry them:
picker_intentis accepted as an alias. Every other multi-word key inParamSchemais snake_case (show_if,dir_from,param_defaults), so manifest authors following the existing convention will plausibly writepicker_intent. Happy to remove it and keep camelCase only.filtersare not wired. The issue's example includesfilters, but honoring arbitrary filters needs a newfs:selectFile(filters)IPC handler, which felt out of scope here.pickerIntent: "image"uses the existing dialog'spng/jpg/jpeg/webpfilter, which covers the reported case. Glad to follow up if you want generic filters.Verification
New
src/shared/utils/paramPicker.test.mjs(esbuild-bundle +node:test, same harness asformat.test.mjs/nodeBehaviors.test.mjs). It passes a fakefsthat records which dialog was opened, so it asserts on the actual routing rather than on a flag —pickerIntent: "image"must callselectImageand must not callselectDirectory. It also pins the fallback cases and greps both call sites to make sure neither re-introduces a directselectDirectory()call.Negative control — with the pre-fix behavior restored (call sites reverted and
openParamPickerforced back toselectDirectory()), 4 of the 8 new tests fail, including the#155image case and both call-site checks:With the fix: 8/8 pass.
Also run on this branch:
node:testsuite — 37/37 TS tests, 48/48.mjstests (40 pre-existing + 8 new).npx eslinton every changed file — clean.npx tsc --noEmit -p tsconfig.web.json— zero errors in any file this PR touches. (The pre-existing errors inassetLibrary*andworkflowRunStore.tsare unchanged and untouched by this PR.)npx electron-vite build— succeeds. Chunk layout is unchanged: the shared Generate/Workflows chunk is the same chunk as before, just renamed by Vite (preflight-*.js484.22 kB →paramPicker-*.js486.22 kB), so this is not a code-splitting regression.Not verified, disclosed for transparency:
fs:selectImageshows an image dialog rests on the existing handler (unchanged by this PR) plus the routing test, not on a live run.electron/main/extension-install-recovery.test.mjstest 18 ("registration transaction state never writes through a symlinked backup") fails on my Windows machine withEPERM ... symlink— creating symlinks needs elevation there. It fails identically on a cleandevcheckout and touches no code in this PR.npm testalso runstest:py, andnpm run test:nodeneeds Node >= 22.6 for--experimental-strip-types; I ran the TS half under Node 22.22 and the.mjshalf under Node 20.18.