Feat: allow user to choose which directory to save Dyad apps in#2875
Feat: allow user to choose which directory to save Dyad apps in#2875wwwillchen merged 39 commits intodyad-sh:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances user flexibility by allowing them to define a custom base directory for their Dyad applications. This change addresses the need for personalized storage locations, moving beyond the previously fixed default folder. The implementation carefully handles the transition for existing applications by employing symlinks, which maintain accessibility without forcing a data migration. Furthermore, all app-related file system operations have been updated to intelligently navigate these symlinks, ensuring that the application behaves predictably and reliably with both new and existing app installations. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Greptile SummaryThis PR adds a General Settings option that lets users choose a custom top-level directory for storing new Dyad apps instead of the default Key design decisions:
Issues found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as CustomAppsFolderSelector
participant IPC as IPC (custom_apps_folder_handlers)
participant Paths as paths.ts (cache)
participant DB as SQLite DB
participant FS as Filesystem
participant Settings as user-settings.json
UI->>IPC: selectCustomAppsFolder()
IPC->>FS: dialog.showOpenDialog()
FS-->>IPC: { filePaths, canceled }
IPC->>FS: isDirectoryAccessible(dirPath)
IPC-->>UI: { path, canceled }
UI->>IPC: setCustomAppsFolder(newPath)
IPC->>Paths: invalidateDyadAppsBaseDirectoryCache()
IPC->>Paths: getDyadAppsBaseDirectory() → prevPath
Paths->>Settings: readSettings().customAppsFolder
Settings-->>Paths: cached & returned
IPC->>FS: isDirectoryAccessible(newPath)
Note over IPC,DB: Only if newPath !== prevPath
IPC->>DB: transaction: convert relative → absolute paths
DB-->>IPC: done
IPC->>Settings: writeSettings({ customAppsFolder })
IPC->>Paths: invalidateDyadAppsBaseDirectoryCache()
UI->>IPC: getCustomAppsFolder()
IPC->>Paths: invalidateDyadAppsBaseDirectoryCache()
IPC->>Paths: getDyadAppsBaseDirectory()
Paths->>Settings: readSettings().customAppsFolder
Settings-->>Paths: value (null = default)
IPC->>FS: isDirectoryAccessible(directory)
IPC->>Paths: getCustomFolderCache() → null means default
IPC-->>UI: { path, isPathAvailable, isPathDefault }
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/lib/schemas.ts
Line: 340
Comment:
**Empty string bypasses path resolution**
`z.string().optional().nullable()` accepts `""` as a valid value. If a user manually edits their settings file to set `customAppsFolder: ""`, `getDyadAppsBaseDirectory()` will return `""` (empty string is not nullish, so the `??` chain short-circuits there). `path.join("", "my-app")` then resolves to just `"my-app"` — a path relative to the process's working directory — silently storing and looking for apps in an unpredictable location.
Adding a minimum length constraint prevents this without affecting normal usage:
```suggestion
customAppsFolder: z.string().min(1).optional().nullable(),
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/ipc/handlers/app_handlers.ts
Line: 1454-1457
Comment:
**`renameApp` missing accessibility check for the new path**
`createApp`, `copyApp`, `cloneRepo`, and `import` all call `isAppLocationAccessible` before performing write operations to the custom folder. `renameApp` does not. When `app.path` is relative (the normal case for apps that haven't been migrated to an absolute path), `newAppPath = getDyadAppPath(appPath)` resolves inside the current custom folder. If that folder is inaccessible the file move will fail with a generic `"Failed to move app files"` error, giving the user no indication that the root cause is the custom folder setting.
Adding the same guard used elsewhere would produce a consistent, actionable error message:
```ts
if (!isAppLocationAccessible(newAppPath)) {
throw new Error(
`The path ${newAppPath} is inaccessible. Please check your custom apps folder setting.`,
);
}
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "fix: prevent path co..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
hi @RyanGroch - thanks for the PR. yeah the existing apps makes this issue tricky (why is we haven't tackled it yet :) agree with the concern about bulk moving files, this could take a very long time since some people have dozens of apps and each app can be a gb+. what about this: before we let the user change this base directory, we should iterate through all the apps and ensure the path is absolute. if it's already absolute (e.g. import without copy into dyad-apps), leave it alone. if it's a relative path (which assumes it's relative, now, to having an e2e test for this would be good as this is a pretty subtle feature. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Missing default value for customAppsFolder in DEFAULT_SETTINGS (src/main/settings.ts:23-47)
Per the mandatory rule in rules/adding-settings.md, when adding a new setting, step 2 requires adding a default value in DEFAULT_SETTINGS in src/main/settings.ts. The customAppsFolder field was added to UserSettingsSchema (src/lib/schemas.ts:340) and to the search index, but no corresponding entry was added to DEFAULT_SETTINGS in src/main/settings.ts:23-47. While the field is optional().nullable() so it won't cause a runtime error, it violates the explicit rule for adding settings.
View 17 additional findings in Devin Review.
| if (!isAppLocationAccessible(newAppPath)) { | ||
| throw new Error( | ||
| `The path ${newAppPath} is inaccessible. Please check your custom apps folder setting.`, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
The isAppLocationAccessible(...) guards are my effort to account for cases where the user's custom folder is unavailable (e.g. maybe on a separate drive that's not plugged in).
I didn't add these guards to every app handler; I focused on adding them in places where the custom apps folder is most likely to be the cause of failure. This primarily includes handlers that create apps (createApp, copyApp, import-app, cloneRepoFromUrl). The main appeal here is having a specific error message for the user, so I wasn't concerned about putting guards in places where the error message might not fit.
| export const SelectCustomAppsFolderResultSchema = z.object({ | ||
| path: z.string().nullable(), | ||
| canceled: z.boolean(), | ||
| }); |
There was a problem hiding this comment.
SelectAppLocationResultSchema wasn't being used. There's an identical Zod schema in src/ipc/types/app.ts, and that one actually gets used. I've repurposed this one.
| } | ||
|
|
||
| fs.mkdirSync(getUserDataPath(), { recursive: true }); | ||
| fs.mkdirSync(getDyadAppPath("."), { recursive: true }); |
There was a problem hiding this comment.
Previously, we were creating the dyad-apps directory for the first time in the database initialization. I moved it into a function called resolveDefaultDyadAppsDirectory in src/paths/paths.ts so that it gets created the first time that Dyad tries to access it.
The main reason for this is that we probably don't want dyad-apps to get created if the user has chosen a different directory.
| // Cached result of getDyadAppsBaseDirectory | ||
| let cachedBaseDirectory: string | null = null; | ||
| let cachedCustomFolderSetting: string | null | undefined; |
There was a problem hiding this comment.
I've changed getDyadAppsBaseDirectory so that it now relies on a settings read. However, to avoid the potentially expensive settings read on every call, I also added a cache for the result here.
I'm actually a bit torn on whether the cache is necessary or a premature optimization (AI reviews asked for it), but I don't think it hurts.
There was a problem hiding this comment.
generally, you don't need to worry about caching read settings since it's very fast (it should be a small file), but seems OK
|
Apologies for taking so long on this; I ended changing my approach a few times (in part to move away from the symlink approach as requested), so the PR is quite different from when I first opened it. There are still two things that I'm unsure of:
Please let me know if there's anything else you'd like changed. Thanks! |
|
@azizmejri1 could you take a look at this PR first and respond to some of the comments? thanks |
Sure , I'll take a look |
I think the comment is meant for modifying existing fields , adding new ones is fine. |
azizmejri1
left a comment
azizmejri1
left a comment
There was a problem hiding this comment.
@RyanGroch @wwwillchen Overall this looks good , just left a minor comment regarding a misleading error message.
github-actions
bot
added
the
needs-human:review-issue
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Missing default value for customAppsFolder in DEFAULT_SETTINGS (src/main/settings.ts:23-47)
Per the mandatory rule in rules/adding-settings.md, when adding a new setting, step 2 requires adding a default value in DEFAULT_SETTINGS in src/main/settings.ts. The customAppsFolder field was added to UserSettingsSchema (src/lib/schemas.ts:340) and to the search index, but no corresponding entry was added to DEFAULT_SETTINGS in src/main/settings.ts:23-47. While the field is optional().nullable() so it won't cause a runtime error, it violates the explicit rule for adding settings.
View 17 additional findings in Devin Review.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87546e3e24
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const resolvedAppPath = path.isAbsolute(appPath) | ||
| ? appPath | ||
| : path.join(basePath, appPath); | ||
| await fsPromises.rm(resolvedAppPath, { |
There was a problem hiding this comment.
resetAll now recursively deletes every apps.path value from the database, including absolute paths (fsPromises.rm(resolvedAppPath, { recursive: true, force: true })). That is unsafe because Dyad can store user-owned absolute paths (for example, local-folder imports with skipCopy=true keep sourcePath instead of copying), so a reset can wipe unrelated project directories outside any Dyad-managed apps root. This is a regression from the previous behavior and can cause irreversible data loss for users who imported existing folders in-place.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I agree that this could happen. The warning message on resetAll is pretty explicit though, so I'm not sure how many users would unintentionally lose apps because of this.
It would also probably require a change to the database schema to determine which apps were imported in place and are therefore exempt from deletion. I'd prefer not to make a change like that in this PR. We could treat all absolute paths as exempt, but after this PR that could include a lot of apps.
I'll leave this unresolved because it's worth seeing.
|
@azizmejri1 Thanks for the review. I appreciate the note on the error message; that's been fixed.
Good to know. The default value for @wwwillchen By the way, don't worry too much about responding to my comments/questions. I leave them mostly for context or to call attention to particular things. Unless there's a change I should make, it's totally fine to ignore or resolve them. |
wwwillchen
left a comment
wwwillchen
left a comment
There was a problem hiding this comment.
thanks @RyanGroch! merging this
🎭 Playwright Test Results❌ Some tests failed
Summary: 811 passed, 7 failed, 5 flaky, 254 skipped Failed Tests🍎 macOS
🪟 Windows
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/queued_message.spec.ts \
e2e-tests/setup_flow.spec.ts
|
4 participants
Closes #399. Adds a setting (under "General Settings") to select a custom directory to store new apps in, replacing the default
dyad-appsfolder.In order to make sure that users don't lose access to older apps, I opted for creating symlinks inside the new folder to the old app locations. I went with symlinking because moving every single app could be an expensive operation depending on how many apps there are, and the user might not want that. However, this is inconsistent with the import apps and move folder features (i.e. #2000), which copy the apps instead of symlinking.
I'm happy to change the approach on request. Some options I've been thinking about:
Also, since I'm tentatively adding in the use of symlinks, I modified the move/copy/delete app features so that they always target the true location of the app, not the symlink.
I can also write tests if desired.