Comfy-Org/desktop

Fork 0

mirror of https://github.com/Comfy-Org/desktop.git synced 2026-11-04 23:47:43 +00:00

[GH-ISSUE #1405] SHOW_DIRECTORY_PICKER and preload typing don’t reflect cancellation / undefined result #400

New issue

Closed

opened 2026-10-31 22:12:32 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-31 22:12:32 +00:00

Owner

Originally created by @benceruleanlu on GitHub (Nov 19, 2025).
Original GitHub issue: https://github.com/Comfy-Org/desktop/issues/1405

Summary

The SHOW_DIRECTORY_PICKER IPC handler can return undefined when the user cancels the dialog, but the preload typing exposes it as Promise<string>. Most current call sites defensively check if (result), so runtime behavior is fine today, but the type mismatch is misleading and could lead to future misuse.

Current behavior

In src/handlers/pathHandlers.ts:

ipcMain.handle(IPC_CHANNELS.SHOW_DIRECTORY_PICKER, async (): Promise<string> => {
  const result = await dialog.showOpenDialog({
    properties: ['openDirectory'],
  });
  return result.filePaths[0];
});

When the user cancels the dialog, result.canceled will be true and result.filePaths will be an empty array, so filePaths[0] is undefined.

In src/preload.ts:

showDirectoryPicker: (): Promise<string> => {
  return ipcRenderer.invoke(IPC_CHANNELS.SHOW_DIRECTORY_PICKER);
},

So the preload API claims the function always resolves to a string, but the implementation actually returns string | undefined.

Frontend usage

In frontend-2/apps/desktop-ui/src/components/install/InstallLocationPicker.vue and MigrationPicker.vue:

const browsePath = async () => {
  try {
    const result = await electron.showDirectoryPicker();
    if (result) {
      installPath.value = result;
      await validatePath(result);
    }
  } catch (error) {
    pathError.value = t('install.failedToSelectDirectory');
  }
};

The components already guard with if (result) so they behave correctly at runtime when the user cancels.

Issues

The mismatch between the declared return type (Promise<string>) and possible values (string | undefined) is confusing and can lead to:
- Consumers assuming the value is always a string and skipping the cancellation check.
- TypeScript not flagging that omission, because the type definition is too strict.

Expected behavior

The preload typing should accurately reflect that the result may be undefined when the user cancels.
Optionally, the main-process handler could explicitly encode cancelation (e.g., { path?: string; canceled: boolean }) to make usage and typing even clearer.

Possible improvements

Adjust the preload type to Promise<string | undefined>

In src/preload.ts, change:

showDirectoryPicker: (): Promise<string> => {
  return ipcRenderer.invoke(IPC_CHANNELS.SHOW_DIRECTORY_PICKER);
},

to:

showDirectoryPicker: (): Promise<string | undefined> => {
  return ipcRenderer.invoke(IPC_CHANNELS.SHOW_DIRECTORY_PICKER);
},

This matches the current behavior of the handler and incentivizes callers to handle the undefined case.

Optionally, make cancelation explicit
- Change the handler to return an object, e.g. { path?: string; canceled: boolean }, and update the preload type accordingly.
- This is a larger API change but can make intent clearer.
Add a small unit test for cancelation
- In tests/unit/handlers/pathHandler.test.ts, add a case where dialog.showOpenDialog returns { filePaths: [], canceled: true } and assert that the handler resolves to undefined.

Impact

Zero runtime impact today (callers already guard for falsy values) but the current type contract is inaccurate and brittle.
Fixing the typing reduces the chance of future call sites using the API incorrectly.

Suggested work

Update the preload type to Promise<string | undefined>.
Add a unit test around the cancelation behavior.
Optionally discuss whether a more structured return type would be preferred long-term.

┆Issue is synchronized with this Notion page by Unito

Originally created by @benceruleanlu on GitHub (Nov 19, 2025). Original GitHub issue: https://github.com/Comfy-Org/desktop/issues/1405 ### Summary The `SHOW_DIRECTORY_PICKER` IPC handler can return `undefined` when the user cancels the dialog, but the preload typing exposes it as `Promise<string>`. Most current call sites defensively check `if (result)`, so runtime behavior is fine today, but the type mismatch is misleading and could lead to future misuse. ### Current behavior In `src/handlers/pathHandlers.ts`: ```ts ipcMain.handle(IPC_CHANNELS.SHOW_DIRECTORY_PICKER, async (): Promise<string> => { const result = await dialog.showOpenDialog({ properties: ['openDirectory'], }); return result.filePaths[0]; }); ``` - When the user cancels the dialog, `result.canceled` will be `true` and `result.filePaths` will be an empty array, so `filePaths[0]` is `undefined`. In `src/preload.ts`: ```ts showDirectoryPicker: (): Promise<string> => { return ipcRenderer.invoke(IPC_CHANNELS.SHOW_DIRECTORY_PICKER); }, ``` So the preload API claims the function always resolves to a `string`, but the implementation actually returns `string | undefined`. ### Frontend usage In `frontend-2/apps/desktop-ui/src/components/install/InstallLocationPicker.vue` and `MigrationPicker.vue`: ```ts const browsePath = async () => { try { const result = await electron.showDirectoryPicker(); if (result) { installPath.value = result; await validatePath(result); } } catch (error) { pathError.value = t('install.failedToSelectDirectory'); } }; ``` The components already guard with `if (result)` so they behave correctly at runtime when the user cancels. ### Issues - The mismatch between the declared return type (`Promise<string>`) and possible values (`string | undefined`) is confusing and can lead to: - Consumers assuming the value is always a string and skipping the cancellation check. - TypeScript not flagging that omission, because the type definition is too strict. ### Expected behavior - The preload typing should accurately reflect that the result may be `undefined` when the user cancels. - Optionally, the main-process handler could explicitly encode cancelation (e.g., `{ path?: string; canceled: boolean }`) to make usage and typing even clearer. ### Possible improvements 1. **Adjust the preload type to `Promise<string | undefined>`** - In `src/preload.ts`, change: ```ts showDirectoryPicker: (): Promise<string> => { return ipcRenderer.invoke(IPC_CHANNELS.SHOW_DIRECTORY_PICKER); }, ``` to: ```ts showDirectoryPicker: (): Promise<string | undefined> => { return ipcRenderer.invoke(IPC_CHANNELS.SHOW_DIRECTORY_PICKER); }, ``` - This matches the current behavior of the handler and incentivizes callers to handle the `undefined` case. 2. **Optionally, make cancelation explicit** - Change the handler to return an object, e.g. `{ path?: string; canceled: boolean }`, and update the preload type accordingly. - This is a larger API change but can make intent clearer. 3. **Add a small unit test for cancelation** - In `tests/unit/handlers/pathHandler.test.ts`, add a case where `dialog.showOpenDialog` returns `{ filePaths: [], canceled: true }` and assert that the handler resolves to `undefined`. ### Impact - Zero runtime impact today (callers already guard for falsy values) but the current type contract is inaccurate and brittle. - Fixing the typing reduces the chance of future call sites using the API incorrectly. ### Suggested work - Update the preload type to `Promise<string | undefined>`. - Add a unit test around the cancelation behavior. - Optionally discuss whether a more structured return type would be preferred long-term. ┆Issue is synchronized with this [Notion page](https://www.notion.so/Issue-1405-SHOW_DIRECTORY_PICKER-and-preload-typing-don-t-reflect-cancellation-undefined-result-2b06d73d365081a7be98d4a99abc05e4) by [Unito](https://www.unito.io)

bsakaguchi

2026-10-31 22:12:32 +00:00

closed this issue
added the
enhancement

area: desktop-app

component: installer

component: preload
labels

bsakaguchi commented

2026-10-31 22:12:33 +00:00

Author

Owner

@dosubot[bot] commented on GitHub (Nov 19, 2025):

You're right about the type mismatch: the SHOW_DIRECTORY_PICKER handler can return undefined on cancellation, but the preload and IPC channel typings currently claim it always returns a string. This could lead to future misuse if callers skip the cancellation check.

To fix this, update the return type in both the preload API and the IPC channel definition to Promise<string | undefined> and string | undefined, respectively. This matches the actual behavior and encourages correct handling. There's already a broader effort in the repo to address similar IPC type mismatches and ensure type safety across the IPC boundary (see here, and here).

There's also a gap in unit test coverage: the current tests only check the happy path (directory selected), not the cancellation case. Adding a test where dialog.showOpenDialog returns { filePaths: [], canceled: true } and asserting that the handler returns undefined would document and verify the intended behavior (see current test).

If you want to make cancellation even more explicit, you could return an object like { path?: string; canceled: boolean } from the handler and update the typing throughout, but this would be a larger API change.

In summary: update the typings to reflect the real return type, add a cancellation test, and optionally consider a more explicit return structure if you want to make intent even clearer. This will make the API safer and more maintainable.

To reply, just mention @dosu.

Share context across your team and agents. Try Dosu.

@dosubot[bot] commented on GitHub (Nov 19, 2025):  You're right about the type mismatch: the SHOW_DIRECTORY_PICKER handler can return undefined on cancellation, but the preload and IPC channel typings currently claim it always returns a string. This could lead to future misuse if callers skip the cancellation check. To fix this, update the return type in both the preload API and the IPC channel definition to Promise<string | undefined> and string | undefined, respectively. This matches the actual behavior and encourages correct handling. There's already a broader effort in the repo to address similar IPC type mismatches and ensure type safety across the IPC boundary ([see here](https://github.com/Comfy-Org/desktop/issues/1367), [and here](https://github.com/Comfy-Org/desktop/pull/1366)). There's also a gap in unit test coverage: the current tests only check the happy path (directory selected), not the cancellation case. Adding a test where dialog.showOpenDialog returns { filePaths: [], canceled: true } and asserting that the handler returns undefined would document and verify the intended behavior ([see current test](https://github.com/Comfy-Org/desktop/blob/35d386c3dd74c79951c79a5105df07a5f28b0c91/tests/unit/handlers/pathHandler.test.ts#L453-L471)). If you want to make cancellation even more explicit, you could return an object like { path?: string; canceled: boolean } from the handler and update the typing throughout, but this would be a larger API change. In summary: update the typings to reflect the real return type, add a cancellation test, and optionally consider a more explicit return structure if you want to make intent even clearer. This will make the API safer and more maintainable.  *To reply, just mention [@dosu](https://go.dosu.dev/dosubot?utm_source=github&utm_medium=bot-comment&utm_campaign=github-comment-footer-20260415&utm_content=reply-with-mention&utm_term=Comfy-Org%2Fdesktop).* --- Share context across your team and agents. Try [Dosu](https://dosu.dev?utm_source=github&utm_medium=bot-comment&utm_campaign=github-comment-footer-20260415&utm_content=knowledge-infrastructure-tagline&utm_term=Comfy-Org%2Fdesktop). [![Leave Feedback](https://img.shields.io/badge/Leave%20Feedback-555555?style=flat)](https://app.dosu.dev/response-feedback?utm_source=github&utm_medium=bot-comment&utm_campaign=github-comment-footer-20260415&utm_content=knowledge-infrastructure-feedback&utm_term=Comfy-Org%2Fdesktop#message_id=d3cf21ab-6da0-426a-9949-2a5e0744926a) [![Learn about desktop with Dosu](https://img.shields.io/badge/Learn%20about%20desktop%20with%20Dosu-2f7b3f?style=flat&logo=data%3Aimage%2Fsvg%2Bxml%3Bbase64%2CPHN2ZyB3aWR0aD0iODYiIGhlaWdodD0iODkiIHZpZXdCb3g9IjAgMCA4NiA4OSIgZmlsbD0ibm9uZSIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIj48cGF0aCBkPSJNNS4yOTIzNiAxMi43OTI4TDE3Ljc1OTMgNi42ODE4OFY3Mi41NjY3TDUuMjkyMzYgODQuMDYxOFYxMi43OTI4WiIgZmlsbD0iI0I0QkI5MSIvPjxwYXRoIGQ9Ik0xOC4yNTc1IDczLjExOTZMNTkuMTMyOSA3Mi43NDhMNTEuNzAxMSA4Mi40MDk1TDI5LjAzMzggODYuMjkxTDYuMjM5NjIgODUuMTU1NEwxOC4yNTc1IDczLjExOTZaIiBmaWxsPSIjNzc4NTYxIi8%2BPHBhdGggZD0iTTE3LjQ5MTYgMy43MzYzM0wzLjU4NTU3IDEyLjcwOTlWODMuNTc5MkMzLjU4NTU3IDg0Ljc1NDIgNC45ODU2MyA4NS4zNjUyIDUuODQ3MDUgODQuNTY2TDE5LjYyOTYgNzEuNzgwMSIgc3Ryb2tlPSJibGFjayIgc3Ryb2tlLXdpZHRoPSI2LjQyODQ0IiBzdHJva2UtbGluZWNhcD0icm91bmQiLz48bWFzayBpZD0iZG9zdS1kLWN1dG91dCIgZmlsbD0id2hpdGUiPjxwYXRoIGZpbGwtcnVsZT0iZXZlbm9kZCIgY2xpcC1ydWxlPSJldmVub2RkIiBkPSJNNDAuNzA0IDAuNTE4MDY2SDE3LjA0MzlWNzYuMjIyMUg0MC43MDRINDIuNTgwNUg0Ny44MDEzQzY4LjcwNjQgNzYuMjIyMSA4NS42NTMzIDU5LjI3NTIgODUuNjUzMyAzOC4zNzAxQzg1LjY1MzMgMTcuNDY1IDY4LjcwNjMgMC41MTgwNjYgNDcuODAxMyAwLjUxODA2Nkg0Mi41ODA1SDQwLjcwNFoiLz48L21hc2s%2BPHBhdGggZmlsbC1ydWxlPSJldmVub2RkIiBjbGlwLXJ1bGU9ImV2ZW5vZGQiIGQ9Ik00MC43MDQgMC41MTgwNjZIMTcuMDQzOVY3Ni4yMjIxSDQwLjcwNEg0Mi41ODA1SDQ3LjgwMTNDNjguNzA2NCA3Ni4yMjIxIDg1LjY1MzMgNTkuMjc1MiA4NS42NTMzIDM4LjM3MDFDODUuNjUzMyAxNy40NjUgNjguNzA2MyAwLjUxODA2NiA0Ny44MDEzIDAuNTE4MDY2SDQyLjU4MDVINDAuNzA0WiIgZmlsbD0iI0YzRjZGMSIvPjxwYXRoIGQ9Ik0xNy4wNDM5IDAuNTE4MDY2Vi02LjU3OTE5SDkuOTQ2NjlWMC41MTgwNjZIMTcuMDQzOVpNMTcuMDQzOSA3Ni4yMjIxSDkuOTQ2NjlWODMuMzE5NEgxNy4wNDM5Vjc2LjIyMjFaTTE3LjA0MzkgNy42MTUzMkg0MC43MDRWLTYuNTc5MTlIMTcuMDQzOVY3LjYxNTMyWk0yNC4xNDEyIDc2LjIyMjFWMC41MTgwNjZIOS45NDY2OVY3Ni4yMjIxSDI0LjE0MTJaTTQwLjcwNCA2OS4xMjQ5SDE3LjA0MzlWODMuMzE5NEg0MC43MDRWNjkuMTI0OVpNNDIuNTgwNSA2OS4xMjQ5SDQwLjcwNFY4My4zMTk0SDQyLjU4MDVWNjkuMTI0OVpNNDcuODAxMyA2OS4xMjQ5SDQyLjU4MDVWODMuMzE5NEg0Ny44MDEzVjY5LjEyNDlaTTc4LjU1NiAzOC4zNzAxQzc4LjU1NiA1NS4zNTU1IDY0Ljc4NjcgNjkuMTI0OSA0Ny44MDEzIDY5LjEyNDlWODMuMzE5NEM3Mi42MjYxIDgzLjMxOTQgOTIuNzUwNSA2My4xOTQ5IDkyLjc1MDUgMzguMzcwMUg3OC41NTZaTTQ3LjgwMTMgNy42MTUzMkM2NC43ODY2IDcuNjE1MzIgNzguNTU2IDIxLjM4NDcgNzguNTU2IDM4LjM3MDFIOTIuNzUwNUM5Mi43NTA1IDEzLjU0NTMgNzIuNjI2IC02LjU3OTE5IDQ3LjgwMTMgLTYuNTc5MTlWNy42MTUzMlpNNDIuNTgwNSA3LjYxNTMySDQ3LjgwMTNWLTYuNTc5MTlINDIuNTgwNVY3LjYxNTMyWk00MC43MDQgNy42MTUzMkg0Mi41ODA1Vi02LjU3OTE5SDQwLjcwNFY3LjYxNTMyWiIgZmlsbD0iYmxhY2siIG1hc2s9InVybCgjZG9zdS1kLWN1dG91dCkiLz48cGF0aCBkPSJNNjguOTIxNSAzNi4wMTM1QzY4LjkyMTUgMzYuMDEzNSA2NS43MzY5IDQ5LjQ3MzggNTEuNDIzMSA0OS40NzM4QzM3LjEwOTMgNDkuNDczOCAzMi41Nzg3IDM3LjM1OTYgMzIuNTc4NyAzNi4wMTM1IiBzdHJva2U9ImJsYWNrIiBzdHJva2Utd2lkdGg9IjcuNjkxNjEiIHN0cm9rZS1saW5lY2FwPSJyb3VuZCIgc3Ryb2tlLWxpbmVqb2luPSJyb3VuZCIvPjxwYXRoIGQ9Ik0wLjM0ODYzMyA4NS40OTQ2QzAuMzQ4NjMzIDg1LjQ5NDYgMjkuNDg1NiA4NS44MzA5IDM0LjgwOSA4NS42OThDNDQuODMzNyA4NS40NDc3IDUxLjI4NzIgODQuNDAyIDU3LjUyNjkgNzguOTcyNEM2Mi44MTI5IDc0LjM3MjcgNzUuMTM0MiA1OS42ODM2IDc1LjEzNDIgNTkuNjgzNiIgc3Ryb2tlPSJibGFjayIgc3Ryb2tlLXdpZHRoPSI2LjE2NDgyIi8%2BPC9zdmc%2B)](https://github.dosu.com/Comfy-Org/desktop?utm_source=github&utm_medium=bot-comment&utm_campaign=github-comment-footer-20260415&utm_content=knowledge-infrastructure-learn-repo&utm_term=Comfy-Org%2Fdesktop) [![Add Dosu to your team](https://img.shields.io/badge/Add%20Dosu%20to%20your%20team-1f6feb?style=flat)](https://app.dosu.dev/signup?referrer=openSource&source=github-footer&utm_source=github&utm_medium=bot-comment&utm_campaign=github-comment-footer-20260415&utm_content=knowledge-infrastructure-add-team&utm_term=Comfy-Org%2Fdesktop)

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/desktop#400

No description provided.

Rows
Columns