fix(desktop): use path.sep in migrateWorkspace nested-path guard#453
fix(desktop): use path.sep in migrateWorkspace nested-path guard#453HiddenPuppy wants to merge 5 commits intoclawwork-ai:mainfrom
Conversation
Fixes clawwork-ai#397 handleSave, handleTest, and handlePairingRetry all followed the pattern: setLoading(true) → await ipc() → setLoading(false) If the IPC call threw (network error, preload exception, serialization failure), the loading state was never reset and the button stayed stuck on a greyed-out spinner. Now each handler wraps the IPC call in try/finally so the loading flag always resets, and catches errors with console.error + toast.error.
Fixes clawwork-ai#398 handleChangeWorkspace called setChangingWorkspace(true) → await IPC → setChangingWorkspace(false) without try/catch. If the IPC threw (workspace migration exception, invalid path, disk error), the button stayed stuck spinning until Settings was closed. Now both browseWorkspace and changeWorkspace are wrapped: - browseWorkspace failure: logs + toast.error + early return - changeWorkspace failure: logs + toast.error with error detail + finally resets loading state Also wraps browseWorkspace for consistency, since it is also an IPC call that can fail.
Fixes clawwork-ai#399 handleToggleEnabled and handleInstall both used the pattern: setXxx((prev) => new Set(prev).add(key)) → await window.clawwork.xxx(...) → setXxx((prev) => remove key) If the IPC call threw, the key was never removed from the toggling/installing set and the button stayed spinning forever. Now both handlers wrap the IPC call in try/finally so the set is always cleaned up. Errors are caught, logged to console, and surfaced via toast.
…dater Fixes clawwork-ai#400 handleNotificationToggle was computing the new state inline inside the updateSettings argument: { notifications: { ...notifyState, [key]: value } }. While not as severe as a setState updater, this pattern is still fragile because it relies on the captured notifyState closure. If React re-renders or batches updates, the stale closure could overwrite concurrent changes. Now the new state is computed into a local variable first, then passed to updateSettings. This is cleaner, easier to read, and avoids any potential issues with stale closures or double-invocation in Strict Mode.
Replaces hardcoded '/' with 'sep' from the path module so the nested-path check works correctly on Windows. - Fixes clawwork-ai#382 - Adds unit tests for migrateWorkspace and initWorkspace
|
Hi @HiddenPuppy, DetailsInstructions for interacting with me using comments are available here. |
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 improves the robustness of the desktop application by addressing cross-platform path handling issues and enhancing error management in the UI layer. By replacing hardcoded path separators and implementing structured error handling for asynchronous tasks, the application becomes more resilient to environment-specific pathing and unexpected runtime failures. Highlights
Ignored Files
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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of the settings sections by implementing try-catch-finally blocks for asynchronous operations, ensuring that loading states are correctly reset and errors are properly logged and displayed. It also updates workspace migration to use platform-specific path separators and introduces a new test suite for workspace initialization. Feedback was provided regarding the path comparison logic in the workspace migration, specifically to handle filesystem roots where trailing separators might cause incorrect prefix matching.
| const resolvedOld = resolve(oldPath); | ||
| const resolvedNew = resolve(newPath); | ||
| if (resolvedNew.startsWith(resolvedOld + '/') || resolvedNew === resolvedOld) { | ||
| if (resolvedNew.startsWith(resolvedOld + sep) || resolvedNew === resolvedOld) { |
There was a problem hiding this comment.
The current path comparison logic fails when the workspace is located at the root of a drive or filesystem (e.g., C:\ on Windows or / on Linux). In such cases, path.resolve returns a path that already ends with a separator. Appending another sep creates a prefix like C:\\ or //, which causes startsWith to fail for subdirectories (e.g., C:\Users does not start with C:\\). Using a conditional check for the trailing separator ensures the prefix is correctly formed for both root and non-root paths. Additionally, note that startsWith is case-sensitive, which might be an issue on Windows if paths are provided with inconsistent casing.
| if (resolvedNew.startsWith(resolvedOld + sep) || resolvedNew === resolvedOld) { | |
| if (resolvedNew.startsWith(resolvedOld.endsWith(sep) ? resolvedOld : resolvedOld + sep) || resolvedNew === resolvedOld) { |
1 participant
Replaces hardcoded '/' with
sepfrom the path module so the nested-path check works correctly on Windows.migrateWorkspaceandinitWorkspace