[ENGG-5139]: Onboarding revamp changes #270
Open
Conversation
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
WalkthroughThis PR introduces local workspace management functionality to the Electron app. Changes include: a new IPC handler to open file explorer paths in the main process; two configuration constants for workspace naming; two new IPC methods exposed for creating and opening local workspaces; multiple helper functions in fs-utils.ts for workspace path detection, validation, and configuration management; and new error codes and metadata support in type definitions. The workspace creation and opening logic includes path validation, ancestor directory traversal, and conflict detection. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/main/events.js`:
- Around line 303-306: The handler registered for "open-path-in-file-explorer"
does not await or validate shell.openPath, so supply a defensive fix in the
ipcMain.handle callback: validate that payload.resourcePath is a non-empty
string, then await shell.openPath(resourcePath); capture its returned string and
if it is non-empty treat that as an error (throw or return an error object) so
the renderer can handle it; otherwise return a success value. Update the handler
for "open-path-in-file-explorer" to perform these checks and return/throw the
appropriate result.
In `@src/renderer/actions/local-sync/fs-utils.ts`:
- Around line 506-521: The function getWorkspaceIdFromConfig currently throws
when parseFile returns an error, breaking the FileSystemResult flow; instead,
handle readResult.type === "error" by returning null (or a FileSystem-style
error wrapper if your surrounding code expects it) and avoid throwing—update the
error branch in getWorkspaceIdFromConfig (which uses createFsResource,
GLOBAL_CONFIG_FOLDER_PATH, parseFile, GlobalConfigRecordFileType) to return null
and optionally log the parse error so createDefaultWorkspace and
openExistingLocalWorkspace can continue when the global config is missing.
- Around line 646-662: The loop in getWorkspacePathFromSelectedPath can never
terminate on Windows because it only checks for "/" as the root; update the
termination logic to detect platform roots by checking when
path.dirname(currentPath) === currentPath or when currentPath equals
path.parse(currentPath).root (use Node's path.parse/currentPath root) so the
while loop exits for Windows roots like "C:\\"; keep using CONFIG_FILE,
appendPath and FsService.lstat as-is and break/return null when you hit the
filesystem root.
- Around line 233-249: The current case-insensitive string comparison using
getNormalizedPath between oldResource.path and newResource.path can miss real
conflicts on case-sensitive filesystems; change the rename/conflict check in the
block using alreadyExists and isSamePath to detect identity via filesystem
metadata: when both paths exist (alreadyExists true), call fs.stat (or lstat)
for oldResource.path and newResource.path and treat them as the same if their
device and inode (stat.dev and stat.ino) match; only fall back to the normalized
path comparison for platforms without inode semantics or when stat fails. Update
references around getIfFileExists/getIfFolderExists, isSamePath,
oldResource/newResource and keep the existing error return
(ErrorCode.EntityAlreadyExists/FileTypeEnum) for genuine conflicts.
Comment on lines
+303
to
+306
| ipcMain.handle("open-path-in-file-explorer", async (event, payload = {}) => { | ||
| const { resourcePath } = payload; | ||
| shell.openPath(resourcePath); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant section
if [ -f "src/main/events.js" ]; then
wc -l src/main/events.js
echo "---"
sed -n '303,310p' src/main/events.js
fiRepository: requestly/requestly-desktop-app
Length of output: 351
🏁 Script executed:
# Search for shell.openPath usage patterns in the codebase
rg "shell\.openPath" -B 3 -A 3Repository: requestly/requestly-desktop-app
Length of output: 385
🏁 Script executed:
# Check for other IPC handlers to see error handling patterns
rg "ipcMain\.handle" -A 5 | head -100Repository: requestly/requestly-desktop-app
Length of output: 5147
🌐 Web query:
shell.openPath electron return value async behavior
💡 Result:
Short answer: shell.openPath is asynchronous — it returns a Promise that resolves to a string: an empty string ("") on success or a non-empty error message on failure. It replaced the older synchronous/boolean shell.openItem API (Electron 9+) and should not block the event loop (there were platform/bug issues historically that were fixed in later releases). (freesoftwaredevlopment.github.io)
Citations:
Handle invalid paths and surface shell.openPath errors.
The shell.openPath call is not awaited, which risks unhandled rejections if resourcePath is missing or invalid. Since shell.openPath is asynchronous and returns either an empty string (success) or an error message (failure), awaiting it and returning the result to the renderer enables proper error handling.
🛠️ Suggested fix
ipcMain.handle("open-path-in-file-explorer", async (event, payload = {}) => {
const { resourcePath } = payload;
- shell.openPath(resourcePath);
+ if (!resourcePath) {
+ return { success: false, error: "resourcePath is required" };
+ }
+ const error = await shell.openPath(resourcePath);
+ return error ? { success: false, error } : { success: true };
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ipcMain.handle("open-path-in-file-explorer", async (event, payload = {}) => { | |
| const { resourcePath } = payload; | |
| shell.openPath(resourcePath); | |
| }); | |
| ipcMain.handle("open-path-in-file-explorer", async (event, payload = {}) => { | |
| const { resourcePath } = payload; | |
| if (!resourcePath) { | |
| return { success: false, error: "resourcePath is required" }; | |
| } | |
| const error = await shell.openPath(resourcePath); | |
| return error ? { success: false, error } : { success: true }; | |
| }); |
🤖 Prompt for AI Agents
In `@src/main/events.js` around lines 303 - 306, The handler registered for
"open-path-in-file-explorer" does not await or validate shell.openPath, so
supply a defensive fix in the ipcMain.handle callback: validate that
payload.resourcePath is a non-empty string, then await
shell.openPath(resourcePath); capture its returned string and if it is non-empty
treat that as an error (throw or return an error object) so the renderer can
handle it; otherwise return a success value. Update the handler for
"open-path-in-file-explorer" to perform these checks and return/throw the
appropriate result.
Comment on lines
+233
to
+249
| if (newResource.type === "folder") { | ||
| return getIfFolderExists(newResource); | ||
| } | ||
| return getIfFileExists(newResource); | ||
| })(); | ||
| const isSamePath = getNormalizedPath(oldResource.path).toLowerCase() === getNormalizedPath(newResource.path).toLowerCase(); | ||
| const isSamePath = | ||
| getNormalizedPath(oldResource.path).toLowerCase() === | ||
| getNormalizedPath(newResource.path).toLowerCase(); | ||
| if (!isSamePath && alreadyExists) { | ||
| return { | ||
| type: 'error', | ||
| type: "error", | ||
| error: { | ||
| message: 'Entity already exists!', | ||
| message: "Entity already exists!", | ||
| fileType: FileTypeEnum.UNKNOWN, | ||
| path: newResource.path, | ||
| code: ErrorCode.EntityAlreadyExists, | ||
| } | ||
| }, |
There was a problem hiding this comment.
Case‑insensitive path compare can overwrite existing files on case‑sensitive FS.
If two paths differ only by case and both exist (possible on Linux), this bypasses the conflict check and allows a clobbering rename.
🛠️ Safer guard using inode/dev identity
- const isSamePath =
- getNormalizedPath(oldResource.path).toLowerCase() ===
- getNormalizedPath(newResource.path).toLowerCase();
- if (!isSamePath && alreadyExists) {
+ if (alreadyExists) {
+ const [oldStat, newStat] = await Promise.all([
+ FsService.lstat(oldResource.path).catch(() => null),
+ FsService.lstat(newResource.path).catch(() => null),
+ ]);
+ const sameInode =
+ oldStat &&
+ newStat &&
+ oldStat.ino === newStat.ino &&
+ oldStat.dev === newStat.dev;
+ if (!sameInode) {
return {
type: "error",
error: {
message: "Entity already exists!",
fileType: FileTypeEnum.UNKNOWN,
path: newResource.path,
code: ErrorCode.EntityAlreadyExists,
},
};
+ }
}🤖 Prompt for AI Agents
In `@src/renderer/actions/local-sync/fs-utils.ts` around lines 233 - 249, The
current case-insensitive string comparison using getNormalizedPath between
oldResource.path and newResource.path can miss real conflicts on case-sensitive
filesystems; change the rename/conflict check in the block using alreadyExists
and isSamePath to detect identity via filesystem metadata: when both paths exist
(alreadyExists true), call fs.stat (or lstat) for oldResource.path and
newResource.path and treat them as the same if their device and inode (stat.dev
and stat.ino) match; only fall back to the normalized path comparison for
platforms without inode semantics or when stat fails. Update references around
getIfFileExists/getIfFolderExists, isSamePath, oldResource/newResource and keep
the existing error return (ErrorCode.EntityAlreadyExists/FileTypeEnum) for
genuine conflicts.
Comment on lines
+506
to
+521
| async function getWorkspaceIdFromConfig( | ||
| workspaceFolderPath: string | null | ||
| ): Promise<string | null> { | ||
| const globalConfigFileResource = createFsResource({ | ||
| rootPath: GLOBAL_CONFIG_FOLDER_PATH, | ||
| path: appendPath(GLOBAL_CONFIG_FOLDER_PATH, GLOBAL_CONFIG_FILE_NAME), | ||
| type: "file", | ||
| }); | ||
| const readResult = await parseFile({ | ||
| resource: globalConfigFileResource, | ||
| fileType: new GlobalConfigRecordFileType(), | ||
| }); | ||
|
|
||
| if (readResult.type === "error") { | ||
| throw new Error("Failed to read global config file"); | ||
| } |
There was a problem hiding this comment.
Avoid throwing in getWorkspaceIdFromConfig; it breaks the FileSystemResult flow.
This throws on read errors, which can make createDefaultWorkspace / openExistingLocalWorkspace fail when the global config is missing.
🛠️ Suggested fix
- if (readResult.type === "error") {
- throw new Error("Failed to read global config file");
- }
+ if (readResult.type === "error") {
+ return null;
+ }🤖 Prompt for AI Agents
In `@src/renderer/actions/local-sync/fs-utils.ts` around lines 506 - 521, The
function getWorkspaceIdFromConfig currently throws when parseFile returns an
error, breaking the FileSystemResult flow; instead, handle readResult.type ===
"error" by returning null (or a FileSystem-style error wrapper if your
surrounding code expects it) and avoid throwing—update the error branch in
getWorkspaceIdFromConfig (which uses createFsResource,
GLOBAL_CONFIG_FOLDER_PATH, parseFile, GlobalConfigRecordFileType) to return null
and optionally log the parse error so createDefaultWorkspace and
openExistingLocalWorkspace can continue when the global config is missing.
Comment on lines
+646
to
+662
| async function getWorkspacePathFromSelectedPath( | ||
| selectedPath: string | ||
| ): Promise<string | null> { | ||
| let currentPath = selectedPath; | ||
| while (currentPath !== "/") { | ||
| const workspacePathExists = await FsService.lstat( | ||
| appendPath(currentPath, CONFIG_FILE) | ||
| ) | ||
| .then((stats) => stats.isFile()) | ||
| .catch(() => false); | ||
| if (workspacePathExists) { | ||
| return currentPath; | ||
| } | ||
| currentPath = path.dirname(currentPath); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Potential infinite loop on Windows roots in getWorkspacePathFromSelectedPath.
The loop only exits at "/", so a Windows root like C:\ will never terminate.
🛠️ Suggested fix
- let currentPath = selectedPath;
- while (currentPath !== "/") {
+ let currentPath = selectedPath;
+ // eslint-disable-next-line no-constant-condition
+ while (true) {
const workspacePathExists = await FsService.lstat(
appendPath(currentPath, CONFIG_FILE)
)
.then((stats) => stats.isFile())
.catch(() => false);
if (workspacePathExists) {
return currentPath;
}
- currentPath = path.dirname(currentPath);
+ const parentPath = path.dirname(currentPath);
+ if (parentPath === currentPath) {
+ break;
+ }
+ currentPath = parentPath;
}
return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function getWorkspacePathFromSelectedPath( | |
| selectedPath: string | |
| ): Promise<string | null> { | |
| let currentPath = selectedPath; | |
| while (currentPath !== "/") { | |
| const workspacePathExists = await FsService.lstat( | |
| appendPath(currentPath, CONFIG_FILE) | |
| ) | |
| .then((stats) => stats.isFile()) | |
| .catch(() => false); | |
| if (workspacePathExists) { | |
| return currentPath; | |
| } | |
| currentPath = path.dirname(currentPath); | |
| } | |
| return null; | |
| } | |
| async function getWorkspacePathFromSelectedPath( | |
| selectedPath: string | |
| ): Promise<string | null> { | |
| let currentPath = selectedPath; | |
| // eslint-disable-next-line no-constant-condition | |
| while (true) { | |
| const workspacePathExists = await FsService.lstat( | |
| appendPath(currentPath, CONFIG_FILE) | |
| ) | |
| .then((stats) => stats.isFile()) | |
| .catch(() => false); | |
| if (workspacePathExists) { | |
| return currentPath; | |
| } | |
| const parentPath = path.dirname(currentPath); | |
| if (parentPath === currentPath) { | |
| break; | |
| } | |
| currentPath = parentPath; | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In `@src/renderer/actions/local-sync/fs-utils.ts` around lines 646 - 662, The loop
in getWorkspacePathFromSelectedPath can never terminate on Windows because it
only checks for "/" as the root; update the termination logic to detect platform
roots by checking when path.dirname(currentPath) === currentPath or when
currentPath equals path.parse(currentPath).root (use Node's
path.parse/currentPath root) so the while loop exits for Windows roots like
"C:\\"; keep using CONFIG_FILE, appendPath and FsService.lstat as-is and
break/return null when you hit the filesystem root.
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.
Changes included:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.