[ENGG-4591]: Onboarding revamp changes #270
Conversation
WalkthroughAdds workspace management for local sync: imports Electron shell and an IPC handler to open paths in the system file explorer; introduces renderer constants for default workspace naming; exposes three IPC methods (createDefaultWorkspace, openExistingLocalWorkspace, checkIsWorkspacePathAvailable) from the fs-manager builder; implements many new fs-utils helpers and public functions to detect, validate, create, and open local workspaces; updates common path-name logic; extends types with a new ErrorCode member and optional metadata on FileSystemError. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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.
| 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.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/actions/local-sync/fs-utils.ts (1)
678-699: Also guard against an existing target workspace folder.
The availability check only inspects ancestors; an existing target folder can be reused and later clobbered.🛠️ Suggested fix
const workspaceFolderPath = appendPath(sanitizedWorkspacePath, name); - const folderCreationResult = await createFolder( - createFsResource({ - rootPath: sanitizedWorkspacePath, - path: workspaceFolderPath, - type: "folder", - }), - { - createWithElevatedAccess: true, - } - ); + const workspaceFolderResource = createFsResource({ + rootPath: sanitizedWorkspacePath, + path: workspaceFolderPath, + type: "folder", + }); + const alreadyExists = await getIfFolderExists(workspaceFolderResource); + if (alreadyExists) { + return { + type: "error", + error: { + message: "Workspace folder already exists!", + fileType: FileTypeEnum.UNKNOWN, + path: workspaceFolderPath, + code: ErrorCode.EntityAlreadyExists, + }, + }; + } + const folderCreationResult = await createFolder(workspaceFolderResource, { + createWithElevatedAccess: true, + errorIfExist: true, + });
🤖 Fix all issues with AI agents
In `@src/renderer/actions/local-sync/fs-utils.ts`:
- Around line 760-780: When doesWorkspaceFolderExists is true, ensure the folder
actually contains the workspace CONFIG_FILE before calling
addWorkspaceToGlobalConfig: update the logic around
getWorkspaceIdFromConfig(workspaceFolderPath) to explicitly check for presence
and validity of CONFIG_FILE (or use an existing helper that verifies workspace
config) and only return success from addWorkspaceToGlobalConfig when the config
is present and valid; otherwise either create/initialize the CONFIG_FILE or
return an error (use ErrorCode.EntityAlreadyExists or a new code) instead of
silently registering a non-valid default workspace. Reference functions/vars:
doesWorkspaceFolderExists, getWorkspaceIdFromConfig, CONFIG_FILE,
addWorkspaceToGlobalConfig, DEFAULT_WORKSPACE_NAME, workspaceFolderPath.
RuntimeTerror10
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/actions/local-sync/common-utils.ts (1)
33-55:⚠️ Potential issue | 🔴 CriticalFix Map iteration for exposed workspace paths.
Map.prototype.keys()returns an iterator, so.keys().forEach(...)will throw whenexposedWorkspacePathsis provided. Iterate the keys withfor...of(orArray.from) to avoid a runtime TypeError.✅ Proposed fix
- params.exposedWorkspacePaths?.keys().forEach(path => normalizedRootPaths.add(getNormalizedPath(path))); + for (const p of params.exposedWorkspacePaths?.keys() ?? []) { + normalizedRootPaths.add(getNormalizedPath(p)); + }
🤖 Fix all issues with AI agents
In `@src/renderer/actions/local-sync/fs-utils.ts`:
- Around line 103-141: hasWorkspaceConfigInAncestors and
checkIsWorkspacePathAvailable call sanitizePath which is currently a no-op,
causing incorrect ancestor detection for dialog paths with quotes, URI prefixes,
mixed separators or trailing slashes; implement sanitizePath to: trim
whitespace, strip surrounding quotes, remove URI schemes like "file://", convert
all separators to platform-normalized separators (use path.normalize/replace
backslashes on Windows), remove trailing path separators, and resolve/normalize
to an absolute path (e.g., via path.resolve) so functions like
hasWorkspaceConfigInAncestors and checkIsWorkspacePathAvailable reliably detect
ancestor CONFIG_FILE presence; update or add tests to cover quoted paths,
file:// URIs, mixed separators, and trailing separators.
- Around line 884-894: The code currently wraps an error from
addWorkspaceToGlobalConfig in createFileSystemError (using
FileTypeEnum.UNKNOWN), which loses the original ErrorCode and metadata; instead,
detect when result.type === "error" and return the original result directly
(i.e., return result) rather than calling createFileSystemError so
addWorkspaceToGlobalConfig's error code and metadata are preserved; ensure the
surrounding function's return type accepts that result shape.
- Around line 742-746: Replace the use of homedir()/Documents when building
rqDirectoryPath with the app's userData path: stop joining homedir() and
"Documents" and instead obtain the app path via app.getPath("userData") in the
main process and return it to the renderer over IPC; update the code that
constructs rqDirectoryPath (which currently uses path.join(homedir(),
"Documents", LOCAL_WORKSPACES_DIRECTORY_NAME)) to use the userData path plus
LOCAL_WORKSPACES_DIRECTORY_NAME, and add or call an IPC method (e.g.,
getUserDataPath) in the main process to expose app.getPath("userData") to the
renderer.
| async function hasWorkspaceConfigInAncestors( | ||
| dirPath: string | ||
| ): Promise<boolean> { | ||
| let currentDir = sanitizePath(dirPath); | ||
|
|
||
| // eslint-disable-next-line no-constant-condition | ||
| while (true) { | ||
| const configPath = appendPath(currentDir, CONFIG_FILE); | ||
|
|
||
| const doesConfigFileExist = await getIfFileExists(createFsResource({ | ||
| rootPath: currentDir, | ||
| path: configPath, | ||
| type: "file", | ||
| })); | ||
|
|
||
|
|
||
| if (doesConfigFileExist) { | ||
| return true; | ||
| } | ||
|
|
||
| const parentDir = path.dirname(currentDir); | ||
| if (parentDir === currentDir) { | ||
| break; | ||
| } | ||
|
|
||
| currentDir = parentDir; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| export async function checkIsWorkspacePathAvailable(workspacePath: string): Promise<boolean> { | ||
| const sanitizedWorkspacePath = sanitizePath(workspacePath); | ||
| const hasWorkspaceConfig = await hasWorkspaceConfigInAncestors( | ||
| sanitizedWorkspacePath | ||
| ); | ||
|
|
||
| return !hasWorkspaceConfig; | ||
| } |
There was a problem hiding this comment.
Implement sanitizePath before relying on it in workspace checks.
hasWorkspaceConfigInAncestors / checkIsWorkspacePathAvailable now depend on sanitizePath, but it’s still a no‑op despite the TODO about Electron path delimiters. If dialog paths include quotes/URI prefixes or trailing separators, ancestor detection can fail and incorrectly allow nested workspaces or report a path as “available.”
If you can confirm the exact delimiter behavior, I can propose a concrete sanitization implementation and add tests.
🤖 Prompt for AI Agents
In `@src/renderer/actions/local-sync/fs-utils.ts` around lines 103 - 141,
hasWorkspaceConfigInAncestors and checkIsWorkspacePathAvailable call
sanitizePath which is currently a no-op, causing incorrect ancestor detection
for dialog paths with quotes, URI prefixes, mixed separators or trailing
slashes; implement sanitizePath to: trim whitespace, strip surrounding quotes,
remove URI schemes like "file://", convert all separators to platform-normalized
separators (use path.normalize/replace backslashes on Windows), remove trailing
path separators, and resolve/normalize to an absolute path (e.g., via
path.resolve) so functions like hasWorkspaceConfigInAncestors and
checkIsWorkspacePathAvailable reliably detect ancestor CONFIG_FILE presence;
update or add tests to cover quoted paths, file:// URIs, mixed separators, and
trailing separators.
| const rqDirectoryPath = path.join( | ||
| homedir(), | ||
| "Documents", | ||
| LOCAL_WORKSPACES_DIRECTORY_NAME | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Electron app.getPath("documents") recommended approach user Documents directory
💡 Result:
Use app.getPath('documents') only as a hint for the user’s “My Documents” location (e.g., as the initial folder in a Save/Open dialog), not as the place where your app silently stores its own data. Electron’s docs explicitly position userData as the conventional place for app data/config, and documents as the user’s documents folder. [1]
Recommended pattern (user-facing export/save): prompt the user, using defaultPath seeded from documents. [2]
const { app, dialog } = require('electron');
const path = require('node:path');
async function exportFile() {
const { canceled, filePath } = await dialog.showSaveDialog({
defaultPath: path.join(app.getPath('documents'), 'MyApp Export.txt'),
});
if (canceled || !filePath) return;
// write to filePath
}Recommended pattern (app-internal storage): store under app.getPath('userData') (or a subfolder), not under Documents. [1]
Sources: [1] Electron app.getPath() docs, [2] Electron dialog.showSaveDialog() docs.
🏁 Script executed:
# First, let's examine the actual file and its context
cat -n src/renderer/actions/local-sync/fs-utils.ts | sed -n '735,755p'Repository: requestly/requestly-desktop-app
Length of output: 685
🏁 Script executed:
# Check if this file is renderer process code and how it accesses Electron APIs
head -50 src/renderer/actions/local-sync/fs-utils.tsRepository: requestly/requestly-desktop-app
Length of output: 1066
🏁 Script executed:
# Search for any existing usage of app.getPath in the codebase
rg "app\.getPath" --type ts --type tsxRepository: requestly/requestly-desktop-app
Length of output: 102
🏁 Script executed:
# Search for homedir usage to understand the pattern
rg "homedir\(\)" --type ts --type tsx -B 2 -A 2Repository: requestly/requestly-desktop-app
Length of output: 102
🏁 Script executed:
# Check the LOCAL_WORKSPACES_DIRECTORY_NAME definition
rg "LOCAL_WORKSPACES_DIRECTORY_NAME" --type ts -B 2 -A 2Repository: requestly/requestly-desktop-app
Length of output: 1047
🏁 Script executed:
# Search for IPC usage related to paths in the renderer
rg "ipcRenderer\." src/renderer --type ts | head -20Repository: requestly/requestly-desktop-app
Length of output: 738
🏁 Script executed:
# Check if userData is used elsewhere for similar storage
rg "userData" --type tsRepository: requestly/requestly-desktop-app
Length of output: 147
🏁 Script executed:
# Look for IPC handlers that might get Electron paths
rg "getPath|app\.getPath" src/main --type ts -B 2 -A 2Repository: requestly/requestly-desktop-app
Length of output: 57
Use app.getPath("userData") instead of homedir()/Documents for app-internal storage.
homedir()/Documents is problematic on Windows (OneDrive redirection, localization, custom paths). However, since the Requestly workspace directory is app-internal storage—not user documents—use app.getPath("userData") instead of app.getPath("documents"). The latter is only for user-facing Save/Open dialogs. Access this from the renderer via IPC if needed.
🤖 Prompt for AI Agents
In `@src/renderer/actions/local-sync/fs-utils.ts` around lines 742 - 746, Replace
the use of homedir()/Documents when building rqDirectoryPath with the app's
userData path: stop joining homedir() and "Documents" and instead obtain the app
path via app.getPath("userData") in the main process and return it to the
renderer over IPC; update the code that constructs rqDirectoryPath (which
currently uses path.join(homedir(), "Documents",
LOCAL_WORKSPACES_DIRECTORY_NAME)) to use the userData path plus
LOCAL_WORKSPACES_DIRECTORY_NAME, and add or call an IPC method (e.g.,
getUserDataPath) in the main process to expose app.getPath("userData") to the
renderer.
| const result = await addWorkspaceToGlobalConfig({ | ||
| name: getNameOfResource(workspaceFolderResource), | ||
| path: workspaceRootPath, | ||
| }); | ||
| if (result.type === "error") { | ||
| return createFileSystemError( | ||
| result.error, | ||
| workspaceRootPath, | ||
| FileTypeEnum.UNKNOWN | ||
| ); | ||
| } |
There was a problem hiding this comment.
Preserve original error codes from addWorkspaceToGlobalConfig.
Wrapping the error with createFileSystemError downgrades specific ErrorCodes to UNKNOWN. Return the original error result to keep its code and metadata intact.
✅ Proposed fix
- if (result.type === "error") {
- return createFileSystemError(
- result.error,
- workspaceRootPath,
- FileTypeEnum.UNKNOWN
- );
- }
+ if (result.type === "error") {
+ return result;
+ }📝 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.
| const result = await addWorkspaceToGlobalConfig({ | |
| name: getNameOfResource(workspaceFolderResource), | |
| path: workspaceRootPath, | |
| }); | |
| if (result.type === "error") { | |
| return createFileSystemError( | |
| result.error, | |
| workspaceRootPath, | |
| FileTypeEnum.UNKNOWN | |
| ); | |
| } | |
| const result = await addWorkspaceToGlobalConfig({ | |
| name: getNameOfResource(workspaceFolderResource), | |
| path: workspaceRootPath, | |
| }); | |
| if (result.type === "error") { | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In `@src/renderer/actions/local-sync/fs-utils.ts` around lines 884 - 894, The code
currently wraps an error from addWorkspaceToGlobalConfig in
createFileSystemError (using FileTypeEnum.UNKNOWN), which loses the original
ErrorCode and metadata; instead, detect when result.type === "error" and return
the original result directly (i.e., return result) rather than calling
createFileSystemError so addWorkspaceToGlobalConfig's error code and metadata
are preserved; ensure the surrounding function's return type accepts that result
shape.
Changes included:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.