Skip to content

fix(extensions): honor pickerIntent on ExtensionNode string params - #258

Open
Souptik96 wants to merge 1 commit into
lightningpixel:devfrom
Souptik96:fix/extension-node-picker-intent
Open

fix(extensions): honor pickerIntent on ExtensionNode string params#258
Souptik96 wants to merge 1 commit into
lightningpixel:devfrom
Souptik96:fix/extension-node-picker-intent

Conversation

@Souptik96

Copy link
Copy Markdown

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 pickerIntent instead of always calling selectDirectory(), and swap the button glyph to match.

Root cause

ParamControl in src/areas/workflows/nodes/ExtensionNode.tsx rendered every type: "string" param with a browse button whose handler was hardcoded:

const p = await window.electron.fs.selectDirectory()

Nothing consulted the param definition, so pickerIntent: "image" was silently dropped and an extension author could not get a file dialog. ParamField in src/areas/generate/components/WorkflowPanel.tsx is a second copy of the same control (it renders the same ParamSchema in 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 by fs:selectImage in ipc-handlers.ts. So this PR adds no new IPC surface at all.

Fix

  • New src/shared/utils/paramPicker.tsresolvePickerIntent(param) maps a param to one of folder | image | mesh | text, and openParamPicker(param, fs) calls the matching dialog that preload already provides (selectImage, selectMeshFile, selectTextFile, selectDirectory).
  • Backward compatible by construction: an unset pickerIntent resolves 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.
  • Both call sites now go through openParamPicker. The glyph comes from a new PickerIcon (reusing the existing image / mesh / text node glyphs so the icons match the rest of the app), and the button finally has a title / aria-label — it previously had no accessible name at all.
  • ParamSchema gains pickerIntent?: PickerIntent.

Two small judgement calls, both easy to drop if you'd rather not carry them:

  1. picker_intent is accepted as an alias. Every other multi-word key in ParamSchema is snake_case (show_if, dir_from, param_defaults), so manifest authors following the existing convention will plausibly write picker_intent. Happy to remove it and keep camelCase only.
  2. Per-param filters are not wired. The issue's example includes filters, but honoring arbitrary filters needs a new fs:selectFile(filters) IPC handler, which felt out of scope here. pickerIntent: "image" uses the existing dialog's png/jpg/jpeg/webp filter, 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 as format.test.mjs / nodeBehaviors.test.mjs). It passes a fake fs that records which dialog was opened, so it asserts on the actual routing rather than on a flag — pickerIntent: "image" must call selectImage and must not call selectDirectory. It also pins the fallback cases and greps both call sites to make sure neither re-introduces a direct selectDirectory() call.

Negative control — with the pre-fix behavior restored (call sites reverted and openParamPicker forced back to selectDirectory()), 4 of the 8 new tests fail, including the #155 image case and both call-site checks:

not ok 3 - openParamPicker opens the image dialog for pickerIntent: image — issue #155
not ok 4 - openParamPicker routes mesh/text intents to their existing dialogs
not ok 7 - ExtensionNode.tsx routes its string param browse button through openParamPicker
not ok 8 - WorkflowPanel.tsx routes its string param browse button through openParamPicker
# tests 8 | pass 4 | fail 4

With the fix: 8/8 pass.

Also run on this branch:

  • Full node:test suite — 37/37 TS tests, 48/48 .mjs tests (40 pre-existing + 8 new).
  • npx eslint on every changed file — clean.
  • npx tsc --noEmit -p tsconfig.web.json — zero errors in any file this PR touches. (The pre-existing errors in assetLibrary* and workflowRunStore.ts are 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-*.js 484.22 kB → paramPicker-*.js 486.22 kB), so this is not a code-splitting regression.

Not verified, disclosed for transparency:

  • No manual click-through of the dialogs. I did not launch the packaged Electron app, so the assertion that fs:selectImage shows 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.mjs test 18 ("registration transaction state never writes through a symlinked backup") fails on my Windows machine with EPERM ... symlink — creating symlinks needs elevation there. It fails identically on a clean dev checkout and touches no code in this PR.
  • npm test also runs test:py, and npm run test:node needs Node >= 22.6 for --experimental-strip-types; I ran the TS half under Node 22.22 and the .mjs half under Node 20.18.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant