Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a File Editor feature: backend FS routes (copy, move, download) and Git endpoints (details, enhanced-status); a complete frontend file editor view with CodeMirror editor, file tree, tabs, Git UI, worktree selector, settings, store, routes, and API/Electron bindings. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FE as FileEditorView
participant IPC as Electron IPC / HttpClient
participant Server
participant Store as File Editor Store
participant Editor as CodeEditor
User->>FE: open /file-editor (initialPath)
FE->>IPC: readdir(projectPath)
IPC->>Server: POST /api/fs/readdir
Server-->>IPC: file tree
IPC-->>FE: file tree
FE->>Store: setFileTree
User->>FE: open file
FE->>IPC: readFile(filePath)
IPC->>Server: POST /api/fs/read
Server-->>IPC: file content
IPC-->>FE: content
FE->>Store: openTab(content)
FE->>Editor: render content
User->>Editor: edit -> Cmd+S
Editor->>FE: onSave
FE->>IPC: writeFile(filePath, content)
IPC->>Server: POST /api/fs/write
Server-->>IPC: success
FE->>Store: markTabSaved
sequenceDiagram
participant FE as FileEditorView
participant HttpClient
participant Server
participant FileTree
FE->>HttpClient: git.getEnhancedStatus(projectPath)
HttpClient->>Server: POST /api/git/enhanced-status
Server->>Server: compute git status & diffs
Server-->>HttpClient: enhanced-status result
HttpClient-->>FE: result
FE->>FileTree: update badges
User->>FileTree: context -> Download folder
FileTree->>HttpClient: downloadItem(path)
HttpClient->>Server: POST /api/fs/download
Server->>Server: zip folder (if dir, <=100MB)
Server-->>HttpClient: stream binary
HttpClient-->>User: trigger download
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 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)
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 |
Contributor
Summary of ChangesHello @gsxdsm, 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 introduces a comprehensive in-app file editor, transforming Automaker into a more self-contained development environment. It provides a rich code editing experience with advanced features like multi-tab editing, syntax highlighting, Git integration, and file management capabilities, all accessible directly within the application. This significantly reduces the need to switch to external tools for common development tasks. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
The File Editor feature is a significant addition to the platform, providing a robust in-app editing experience. The implementation of the file tree, CodeMirror integration, and git status indicators is well-structured. However, there are critical security vulnerabilities in the server-side routes where user-provided paths are interpolated into shell commands, creating risks for command injection. Additionally, the file rename logic in the UI is suboptimal and should be refactored to use the newly implemented move API for better performance and correctness, especially for directories. There are also logic issues in the directory copy implementation regarding how existing destinations are handled when overwriting, which currently results in a merge rather than a replacement.
Comment on lines
71
to
76
| await execAsync( | ||
| `cd "${path.dirname(filePath)}" && zip -r "${tmpZipPath}" "${fileName}"`, | ||
| { | ||
| maxBuffer: 50 * 1024 * 1024, | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
Constructing shell commands by interpolating user-provided file paths is a critical security risk that can lead to command injection. A malicious path containing shell metacharacters (e.g., ;, &, |, or $()) could execute arbitrary code on the server. It is highly recommended to use child_process.spawn or child_process.execFile which accept arguments as an array, bypassing the shell entirely.
Comment on lines
63
to
66
| const { stdout: logOutput } = await execAsync( | ||
| `git log -1 --format="%H|%s|%an|%aI" -- "${filePath}"`, | ||
| { cwd: projectPath } | ||
| ); |
Contributor
There was a problem hiding this comment.
The filePath is interpolated directly into a shell command string for git log. This is vulnerable to command injection if the file path contains shell metacharacters. Use execFile or spawn with an arguments array instead of exec with a string template to safely pass user-controlled paths.
| if (statusLine.trim().startsWith('??')) { | ||
| // Untracked file - count all lines as added | ||
| try { | ||
| const { stdout: wcOutput } = await execAsync(`wc -l < "${filePath}"`, { |
Contributor
There was a problem hiding this comment.
Using shell redirection with a user-provided filePath is highly insecure. Instead of spawning a wc -l process, consider reading the file content using secureFs.readFile and counting the lines in memory, or use a safer execution method that avoids shell interpolation.
Comment on lines
663
to
723
| async (oldPath: string, newName: string) => { | ||
| const parentPath = oldPath.substring(0, oldPath.lastIndexOf('/')); | ||
| const newPath = `${parentPath}/${newName}`; | ||
|
|
||
| try { | ||
| const api = getElectronAPI(); | ||
| // Read old content, write to new path, delete old | ||
| const statResult = await api.stat(oldPath); | ||
| const isDir = statResult.success && statResult.stats?.isDirectory; | ||
|
|
||
| if (isDir) { | ||
| // For directories, create new and can't easily move - just refresh | ||
| // In a real implementation, we'd need a rename IPC call | ||
| await api.mkdir(newPath); | ||
| await api.deleteFile(oldPath); | ||
| } else { | ||
| const result = await api.readFile(oldPath); | ||
| if (result.success && result.content !== undefined) { | ||
| await api.writeFile(newPath, result.content); | ||
| await api.deleteFile(oldPath); | ||
|
|
||
| // Update the open tab if it was renamed | ||
| const tab = tabs.find((t) => t.filePath === oldPath); | ||
| if (tab) { | ||
| closeTab(tab.id); | ||
| if (isMobile) { | ||
| handleMobileFileSelect(newPath); | ||
| } else { | ||
| handleFileSelect(newPath); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If the new name starts with a dot, auto-enable hidden files visibility | ||
| // so the renamed file doesn't "disappear" from the tree | ||
| if (newName.startsWith('.')) { | ||
| const { showHiddenFiles } = useFileEditorStore.getState(); | ||
| if (!showHiddenFiles) { | ||
| store.setShowHiddenFiles(true); | ||
| } | ||
| } | ||
|
|
||
| // Preserve expanded folders so the parent directory stays open after rename | ||
| await loadTree(undefined, { preserveExpanded: true }); | ||
| loadGitStatus(); | ||
| } catch (error) { | ||
| logger.error('Failed to rename item:', error); | ||
| } | ||
| }, | ||
| [ | ||
| tabs, | ||
| closeTab, | ||
| handleFileSelect, | ||
| handleMobileFileSelect, | ||
| isMobile, | ||
| loadTree, | ||
| loadGitStatus, | ||
| store, | ||
| ] | ||
| ); |
Contributor
There was a problem hiding this comment.
The current handleRenameItem implementation is inefficient as it performs a rename by reading the entire file content, writing it to a new location, and then deleting the old file. This is slow for large files and incorrect for directories (it only creates an empty directory at the new path without moving its contents). Since a /move endpoint was added to the server and a moveItem method to the API client in this PR, you should use them here. This leverages the atomic fs.rename operation which is significantly faster and correctly handles directory trees.
const handleRenameItem = useCallback(
async (oldPath: string, newName: string) => {
const parentPath = oldPath.substring(0, oldPath.lastIndexOf('/'));
const newPath = `${parentPath}/${newName}`;
try {
const api = getElectronAPI();
const result = await api.moveItem(oldPath, newPath);
if (result.success) {
// Update the open tab if it was renamed
const tab = tabs.find((t) => t.filePath === oldPath);
if (tab) {
closeTab(tab.id);
if (isMobile) {
handleMobileFileSelect(newPath);
} else {
handleFileSelect(newPath);
}
}
// If the new name starts with a dot, auto-enable hidden files visibility
if (newName.startsWith('.')) {
const { showHiddenFiles } = useFileEditorStore.getState();
if (!showHiddenFiles) {
store.setShowHiddenFiles(true);
}
}
await loadTree(undefined, { preserveExpanded: true });
loadGitStatus();
} else {
toast.error('Rename failed', { description: result.error });
}
} catch (error) {
logger.error('Failed to rename item:', error);
}
},
[
tabs,
closeTab,
handleFileSelect,
handleMobileFileSelect,
isMobile,
loadTree,
loadGitStatus,
store,
]
);
Comment on lines
48
to
61
| try { | ||
| await secureFs.stat(destinationPath); | ||
| // Destination exists | ||
| if (!overwrite) { | ||
| res.status(409).json({ | ||
| success: false, | ||
| error: 'Destination already exists', | ||
| exists: true, | ||
| }); | ||
| return; | ||
| } | ||
| } catch { | ||
| // Destination doesn't exist - good to proceed | ||
| } |
Contributor
There was a problem hiding this comment.
When overwrite is true, the current implementation proceeds to copy files into the existing destination directory. This results in a 'merge' of the source and destination contents rather than a clean replacement. To ensure the destination exactly matches the source (standard behavior for an overwrite), the existing destination should be removed first, as is correctly done in the move.ts handler.
Suggested change
| try { | |
| await secureFs.stat(destinationPath); | |
| // Destination exists | |
| if (!overwrite) { | |
| res.status(409).json({ | |
| success: false, | |
| error: 'Destination already exists', | |
| exists: true, | |
| }); | |
| return; | |
| } | |
| } catch { | |
| // Destination doesn't exist - good to proceed | |
| } | |
| // Check if destination already exists | |
| try { | |
| await secureFs.stat(destinationPath); | |
| // Destination exists | |
| if (!overwrite) { | |
| res.status(409).json({ | |
| success: false, | |
| error: 'Destination already exists', | |
| exists: true, | |
| }); | |
| return; | |
| } | |
| // If overwrite is true, remove the existing destination first to avoid merging | |
| await secureFs.rm(destinationPath, { recursive: true }); | |
| } catch { | |
| // Destination doesn't exist - good to proceed | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts-146-146 (1)
146-146:⚠️ Potential issue | 🟡 MinorMisleading comment —
file-editoris unconditionally included, not conditional.The comment says "Terminal and File Editor are conditionally included," but only
terminalis guarded by!hideTerminal. Thefile-editoritem is always pushed intoprojectItems. The comment should reflect the actual logic.✏️ Proposed fix
- // Build project items - Terminal and File Editor are conditionally included + // Build project items - Terminal is conditionally included based on hideTerminal const projectItems: NavItem[] = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts` at line 146, Update the misleading comment above the project items construction in use-navigation.ts: change it to reflect that only the 'terminal' item is conditional (guarded by the hideTerminal flag) while the 'file-editor' item is unconditionally pushed into projectItems; reference the projectItems array and the 'terminal' and 'file-editor' IDs so the comment clearly states the actual behavior.apps/ui/package.json-56-62 (1)
56-62:⚠️ Potential issue | 🟡 MinorAlign
@codemirror/lang-xmlversion pinning with the newly added language packages.
@codemirror/lang-xmlremains pinned at the exact version"6.1.0"while all new@codemirror/lang-*packages (lines 45–55) use caret ranges. CodeMirror 6 packages share internal state objects via@codemirror/state— a version mismatch between language packages can cause silent runtime failures where language extensions stop functioning. Aligning all lang packages to caret ranges ensures the resolver can hoist a consistent version tree.🛡️ Proposed fix
- "@codemirror/lang-xml": "6.1.0", + "@codemirror/lang-xml": "^6.1.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/package.json` around lines 56 - 62, Update the pinned dependency "@codemirror/lang-xml" to use a caret range consistent with the other CodeMirror language packages (e.g., change "6.1.0" to a caret range like "^6.1.0") so the resolver can hoist a single compatible "@codemirror/state" version; locate the entry for "@codemirror/lang-xml" in package.json and adjust its version string accordingly.apps/server/src/routes/fs/routes/copy.ts-47-61 (1)
47-61:⚠️ Potential issue | 🟡 MinorOverwrite for directories merges rather than replaces.
When
overwriteistrueand the destination is an existing directory, the code proceeds to copy into it without first removing the old contents. This results in a merge (old files remain, new files are added/overwritten). The move handler explicitly doessecureFs.rm(destinationPath, { recursive: true })before proceeding. If full replacement is intended here too, add the same removal step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/fs/routes/copy.ts` around lines 47 - 61, The copy handler currently treats overwrite=true as a merge when destinationPath exists; change it to remove the existing destination first (like the move handler) when overwrite is true and the existing destination is a directory: after detecting destination exists via secureFs.stat(destinationPath) and confirming overwrite, call secureFs.rm(destinationPath, { recursive: true }) (handle await and possible errors) before proceeding with the copy operation so the destination is replaced rather than merged.apps/server/src/routes/fs/routes/download.ts-117-131 (1)
117-131:⚠️ Potential issue | 🟡 MinorFile streaming bypasses
secureFslayer.
createReadStream(filePath)at Line 123 uses the rawfsmodule, bypassing whatever path confinement or throttlingsecureFsprovides. WhilesecureFs.stat(filePath)at Line 48 validated the path, a TOCTOU gap exists — the file could be swapped (e.g., via symlink) between the stat and the read.Consider using
secureFs.readFileand piping it, or adding asecureFs.createReadStreamwrapper if streaming is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/fs/routes/download.ts` around lines 117 - 131, The download handler is creating a raw fs stream with createReadStream(filePath) which can bypass secureFs protections and create a TOCTOU race after the earlier secureFs.stat(filePath) check; replace the raw stream with a secureFs-backed read (either call secureFs.readFile(filePath) and pipe the Buffer to res for small/medium files, or add/use a secureFs.createReadStream(filePath) wrapper that opens the file descriptor under secureFs control and returns a stream) and keep the same headers and error handling (update stream.on('error') to log via logError and send the 500 JSON only if !res.headersSent); ensure you remove direct use of createReadStream and reference the existing secureFs.stat/check code paths in the same handler when implementing the change.apps/server/src/routes/git/routes/details.ts-214-231 (1)
214-231:⚠️ Potential issue | 🟡 MinorInner-error fallback returns
success: true— same concern asenhanced-status.ts.Callers cannot distinguish "clean file with no history" from "git command failures." Same recommendation applies: use
success: falseor add awarningfield.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/git/routes/details.ts` around lines 214 - 231, The catch block handling errors in the Git details route currently calls logError(innerError, 'Git details failed') but returns res.json({ success: true, details: { ... }}) which masks failures; change the response to indicate failure (e.g., set success: false) and include an explanatory field (like warning or errorMessage) populated from innerError.message (or a generic message if absent) so callers can distinguish real errors from an empty/clean repo; update the response emitted in the catch where logError is called and keep the same shape for details if needed.apps/server/src/routes/git/routes/enhanced-status.ts-167-170 (1)
167-170:⚠️ Potential issue | 🟡 MinorInner-error fallback returns
success: true— misleading for callers.When git commands fail (e.g.,
projectPathisn't a git repo), the handler responds with{ success: true, branch: '', files: [] }. Callers cannot distinguish "no changed files" from "git commands failed." Consider returningsuccess: falseor including awarningfield so the UI can surface degraded state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/git/routes/enhanced-status.ts` around lines 167 - 170, The catch block in the Git enhanced status handler currently returns a misleading success: true; update the error path in the catch for innerError (where logError(innerError, 'Git enhanced status failed') is called) to respond with success: false and include either a warning or error field (e.g., warning: innerError.message or error: 'Git command failed') so callers can distinguish a failure from an empty result; ensure you still log the full error via logError and choose a suitable HTTP status (e.g., res.status(500).json(...)) or keep 200 if callers rely on JSON-only responses.apps/server/src/routes/git/routes/details.ts-68-74 (1)
68-74:⚠️ Potential issue | 🟡 MinorCommit message parsing breaks on
|in messages.
git log --format="%H|%s|%an|%aI"uses|as delimiter, but commit subjects (%s) can contain|. Use a null byte separator (%x00) instead, and split on\0.Proposed fix
const { stdout: logOutput } = await execAsync( - `git log -1 --format="%H|%s|%an|%aI" -- "${filePath}"`, + `git log -1 --format="%H%x00%s%x00%an%x00%aI" -- "${filePath}"`, { cwd: projectPath } ); if (logOutput.trim()) { - const parts = logOutput.trim().split('|'); + const parts = logOutput.trim().split('\0');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/git/routes/details.ts` around lines 68 - 74, The current parsing of logOutput using '|' breaks when commit subjects contain '|'—update the git log format to use a null separator (%x00) (e.g., "%H%x00%s%x00%an%x00%aI") and change the split call to split on '\0' so lastCommitHash/lastCommitMessage/lastCommitAuthor/lastCommitTimestamp are extracted safely from logOutput; ensure you still guard against missing parts (use fallback to empty string) after splitting.apps/ui/src/components/views/file-editor-view/components/file-tree.tsx-789-798 (1)
789-798:⚠️ Potential issue | 🟡 Minor
handleRootDragOverclearsdraggedPaths, breaking visual feedback on subsequent drag-over events.Setting
draggedPaths: [](Line 794) while the drag is in progress erases the source information. If the user's cursor moves over the root area and then back over a tree node, the descendant-check inTreeNode.handleDragOver(Line 359) sees an empty array and incorrectly allows drop indicators on self/descendant targets. The drop itself is safe (it re-reads fromdataTransfer), but the visual feedback is wrong.Preserve the existing
draggedPathswhen updating the drop target:Proposed fix
const handleRootDragOver = useCallback( (e: React.DragEvent) => { e.preventDefault(); if (effectivePath) { e.dataTransfer.dropEffect = 'move'; - setDragState({ draggedPaths: [], dropTargetPath: effectivePath }); + setDragState((prev) => ({ ...prev, dropTargetPath: effectivePath })); } }, [effectivePath, setDragState] );Note: this requires
setDragStateto accept a callback, or read the current drag state from the store. Alternative: just spread the currentdragStatefrom the store:+ const { dragState, setDragState } = useFileEditorStore(); // ... - setDragState({ draggedPaths: [], dropTargetPath: effectivePath }); + setDragState({ ...dragState, dropTargetPath: effectivePath });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx` around lines 789 - 798, handleRootDragOver is resetting draggedPaths to an empty array which erases the source info and breaks TreeNode.handleDragOver's descendant checks; update handleRootDragOver so it only updates dropTargetPath (preserving existing draggedPaths) by calling setDragState with an updater callback or by reading the current drag state and spreading it (e.g., keep dragState.draggedPaths while setting dropTargetPath to effectivePath and dropEffect to 'move') so visual feedback remains correct during drag-over.apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts-3-10 (1)
3-10:⚠️ Potential issue | 🟡 MinorRemove duplicate
FileTreeNodedefinition; import from shared types instead.
FileTreeNodeis already defined inapps/ui/src/store/types/project-types.tsand should be imported from there (or fromapp-storewhich re-exports it), rather than redefined inuse-file-editor-store.ts. Remove the duplicate interface definition at lines 3-10 and import from the canonical location:import type { FileTreeNode } from '@/store/types/project-types.ts';This follows the shared types pattern already established in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts` around lines 3 - 10, Remove the duplicate FileTreeNode interface defined in use-file-editor-store.ts and instead import the canonical type; delete the interface block (the FileTreeNode declaration) and add an import for FileTreeNode from the shared project types (the same source used elsewhere, e.g. the project-types export), then update any local references to use the imported FileTreeNode type.apps/ui/src/components/views/file-editor-view/file-editor-view.tsx-1149-1153 (1)
1149-1153:⚠️ Potential issue | 🟡 MinorKeyboard shortcut hints are hardcoded to "Ctrl" and won't be correct on macOS.
Consider using a platform check (e.g.,
navigator.platformornavigator.userAgent) to show⌘on macOS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around lines 1149 - 1153, The keyboard shortcut hint currently hardcodes "Ctrl" which is incorrect on macOS; update the UI to detect macOS (e.g., via navigator.platform, navigator.userAgent, or navigator.userAgentData) and render "⌘" for macOS and "Ctrl" otherwise. Introduce a small helper (e.g., getShortcutKey or isMac) and use that in the JSX where isMobile is checked (the conditional rendering block around Ctrl+S to save · Ctrl+F to search) so the displayed modifier is platform-appropriate. Ensure the helper is used for both occurrences in the string and remains unaffected by the isMobile check.apps/ui/src/components/views/file-editor-view/file-editor-view.tsx-635-660 (1)
635-660:⚠️ Potential issue | 🟡 MinorDeleted directories with open child files won't have their tabs closed.
handleDeleteItemonly closes the tab for the exactpaththat was deleted. If a directory is deleted, files inside it that are open in tabs won't be closed — they'll remain as stale tabs pointing to non-existent files.🐛 Proposed fix: close all tabs under the deleted directory
- // Close tab if the deleted file is open - const tab = tabs.find((t) => t.filePath === path); - if (tab) { - closeTab(tab.id); - } + // Close tabs for the deleted item and any children (if directory) + for (const tab of tabs) { + if (tab.filePath === path || tab.filePath.startsWith(path + '/')) { + closeTab(tab.id); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around lines 635 - 660, handleDeleteItem currently only closes a tab when its filePath exactly equals the deleted path, so deleting a directory leaves open child-file tabs; update the logic in handleDeleteItem to detect directory deletions (use the _isDirectory parameter) and close all tabs whose filePath is inside that directory by normalizing the deleted directory path (ensure a trailing path separator) and iterating over tabs to call closeTab(tab.id) for every tab where tab.filePath.startsWith(normalizedDirPath); keep the existing single-file behavior for non-directory deletes.
🧹 Nitpick comments (17)
apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts (1)
160-164: Thefile-editornav item appears to lack both ashortcutproperty and corresponding keyboard shortcut definition, unlikeboard,graph,agent, andterminalitems. Clarify whether a shortcut is planned for the File Editor and, if so, add both theshortcutproperty to the nav item and the corresponding mapping in the shortcuts interface. If it's intentionally omitted, add an inline comment explaining why.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts` around lines 160 - 164, The nav item with id 'file-editor' in use-navigation.ts is missing a shortcut property and corresponding keyboard mapping; either add a shortcut string (e.g., 'ctrl+e') to the 'file-editor' entry and add the same key to the shortcuts interface/definition used by the sidebar shortcuts mapping (where other ids like 'board', 'graph', 'agent', 'terminal' are defined), or if no shortcut is intended, add an inline comment on the 'file-editor' nav item explaining the omission; locate and update the nav item object with id 'file-editor' and the shortcuts interface/mapping where other shortcut keys are declared to keep them consistent.apps/ui/package.json (1)
45-55: Consider@uiw/codemirror-extensions-langsto replace 11 individual language packages.Since
@uiw/react-codemirror4.25.0, the companion package@uiw/codemirror-extensions-langsprovides language extensions keyed by file extension, eliminating the need to manually install and maintain each@codemirror/lang-*package individually. This would reduce 11 entries to a single well-integrated dependency.♻️ Proposed refactor
- "@codemirror/lang-cpp": "^6.0.3", - "@codemirror/lang-css": "^6.3.1", - "@codemirror/lang-html": "^6.4.11", - "@codemirror/lang-java": "^6.0.2", - "@codemirror/lang-javascript": "^6.2.4", - "@codemirror/lang-json": "^6.0.2", - "@codemirror/lang-markdown": "^6.5.0", - "@codemirror/lang-php": "^6.0.2", - "@codemirror/lang-python": "^6.2.1", - "@codemirror/lang-rust": "^6.0.2", - "@codemirror/lang-sql": "^6.10.0", + "@uiw/codemirror-extensions-langs": "^4.25.4",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/package.json` around lines 45 - 55, Replace the eleven individual `@codemirror/lang-`* dependencies with the single `@uiw/codemirror-extensions-langs` package: remove the entries for "@codemirror/lang-cpp", "-css", "-html", "-java", "-javascript", "-json", "-markdown", "-php", "-python", "-rust", and "-sql" from package.json and add "@uiw/codemirror-extensions-langs" (compatible with `@uiw/react-codemirror` v4.25.0); then update any imports in your CodeMirror setup components (places that import from "@codemirror/lang-*" or reference those language extension symbols) to import the extension map from "@uiw/codemirror-extensions-langs" and use its language-by-extension lookup instead of individual language constructors.libs/types/src/settings.ts (1)
1008-1016: Consider adding default values toDEFAULT_GLOBAL_SETTINGS.The new fields document their defaults in comments (
13,'default',false,1000) butDEFAULT_GLOBAL_SETTINGS(Line 1598+) doesn't include them. While the fields are optional and defaults are reportedly handled in hydration/migration, adding them to the canonical defaults object would improve discoverability and consistency with other settings likeserverLogLevel(Line 1619).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/src/settings.ts` around lines 1008 - 1016, DEFAULT_GLOBAL_SETTINGS is missing the documented defaults for the new file editor fields; update the DEFAULT_GLOBAL_SETTINGS object to include editorFontSize: 13, editorFontFamily: 'default', editorAutoSave: false, and editorAutoSaveDelay: 1000 so the canonical defaults match the comments and other settings like serverLogLevel, ensuring these keys (editorFontSize, editorFontFamily, editorAutoSave, editorAutoSaveDelay) are present with the specified values in DEFAULT_GLOBAL_SETTINGS.apps/server/src/routes/fs/routes/move.ts (1)
60-61:fs.renamefails across filesystem boundaries (EXDEV).
secureFs.renamewraps Node'sfs.rename, which doesn't work across mount points/devices. If source and destination are on different filesystems, this will throw anEXDEVerror. Consider a fallback strategy (copy + delete) for cross-device moves, or at minimum return a clear error message if this is expected to only work within the same filesystem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/fs/routes/move.ts` around lines 60 - 61, secureFs.rename(sourcePath, destinationPath) can throw EXDEV when moving across filesystems; update the move handler to catch errors from secureFs.rename and if err.code === 'EXDEV' perform a fallback copy-then-delete (e.g., use secureFs.copyFile or stream copy from sourcePath to destinationPath, then secureFs.unlink(sourcePath) once copy succeeds) and propagate any errors; if cross-device moves are unsupported, return a clear, specific error message referencing EXDEV instead of letting the raw exception bubble up.apps/ui/src/components/views/file-editor-view/components/code-editor.tsx (1)
78-296:getLanguageExtensionandgetLanguageNameduplicate file-extension mapping.Both functions contain nearly identical
switchblocks mapping file extensions to languages. Consider extracting a shared lookup table to keep them in sync and reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/code-editor.tsx` around lines 78 - 296, getLanguageExtension and getLanguageName duplicate the same extension-to-language logic; extract a single shared lookup (e.g., const languageMap or an object keyed by extension/name) that maps file extensions and special basenames to both the CodeMirror Extension factory (or StreamLanguage.define wrapper) and the human-readable name, then refactor getLanguageExtension to look up and return the Extension (or null) and getLanguageName to return the display name (or 'Plain Text'); preserve special-case handling for filenames like 'dockerfile', 'makefile', '.env', and dotfiles and ensure the fallback behaviors (null for extension, 'Plain Text' for name) remain unchanged, updating references in both getLanguageExtension and getLanguageName to use the new shared mapping.apps/server/src/routes/git/index.ts (1)
23-24: Business logic belongs inservices/, not inline in route handlersBoth
createDetailsHandlerandcreateEnhancedStatusHandlerrun git shell commands, parse porcelain output, and build response objects directly inside the handler factory. Per coding guidelines, server business logic should be organized into services in theservices/directory, with route handlers only delegating to those services.As per coding guidelines: "Server business logic should be organized into services in the services/ directory, with Express route handlers in routes/ that delegate to services."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/git/index.ts` around lines 23 - 24, The route handlers createDetailsHandler and createEnhancedStatusHandler contain business logic (running git shell commands, parsing porcelain output, building responses); move that logic into new service functions under services/ (e.g., gitService.getDetails(projectPath) and gitService.getEnhancedStatus(projectPath)) and refactor the handlers to only validate input and call these service methods, returning their results; ensure the git command execution, parsing, and response-shaping code is extracted intact into the service, add unit tests for the new service functions, and update imports in the routes to call the service APIs instead of performing git work inline.apps/ui/src/routes/file-editor.lazy.tsx (1)
2-2: Prefer barrel import over direct file pathThe
FileEditorViewbarrel already exists atapps/ui/src/components/views/file-editor-view/index.ts. Import from it for stability and refactorability.♻️ Proposed fix
-import { FileEditorView } from '@/components/views/file-editor-view/file-editor-view'; +import { FileEditorView } from '@/components/views/file-editor-view';Based on learnings: "prefer barrel exports from the components index barrel rather than direct path imports" (stefandevo, PR
#656).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/routes/file-editor.lazy.tsx` at line 2, Replace the direct import of FileEditorView with the barrel export: update the import statement that currently references '@/components/views/file-editor-view/file-editor-view' to import FileEditorView from the view's index barrel (the components/views/file-editor-view index export) so the module uses the barrel export for stability and refactorability, keeping the imported symbol name FileEditorView unchanged.apps/ui/src/types/electron.d.ts (1)
665-665:downloadItemreturnsPromise<void>— consider returning a structured resultEvery other file-system method on
ElectronAPI(readFile,writeFile,mkdir,deleteFile,copyItem,moveItem) returnsPromise<{ success: boolean; error?: string }>.downloadItemsilently returnsvoid, giving callers no way to detect or surface failures.🔧 Suggested signature
- downloadItem?: (filePath: string) => Promise<void>; + downloadItem?: (filePath: string) => Promise<{ success: boolean; error?: string }>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/types/electron.d.ts` at line 665, The downloadItem declaration currently returns Promise<void> but should match the other ElectronAPI file-system methods (readFile, writeFile, mkdir, deleteFile, copyItem, moveItem) by returning Promise<{ success: boolean; error?: string }>; update the downloadItem signature in apps/ui/src/types/electron.d.ts to Promise<{ success: boolean; error?: string }>, then update the corresponding implementation and any callers to return and handle a { success, error } object (ensure errors are caught and returned as { success: false, error: error.message } and successes as { success: true }).apps/ui/src/components/views/settings-view/editor/editor-section.tsx (1)
1-10: Prefer barrel exports for UI primitives.Pull the UI primitives from the components index barrel to keep imports stable during refactors.
♻️ Suggested import consolidation
-import { Label } from '@/components/ui/label'; -import { Slider } from '@/components/ui/slider'; -import { Switch } from '@/components/ui/switch'; -import { - Select, - SelectContent, - SelectItem, - SelectTrigger, - SelectValue, -} from '@/components/ui/select'; +import { + Label, + Slider, + Switch, + Select, + SelectContent, + SelectItem, + SelectTrigger, + SelectValue, +} from '@/components';Based on learnings: In the apps/ui codebase, when importing UI components within the same app, prefer barrel exports from ../components (i.e., import from the components index barrel) rather than direct path imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/settings-view/editor/editor-section.tsx` around lines 1 - 10, Replace the multiple direct UI primitive imports with a single barrel import from the components index: import Label, Slider, Switch, Select, SelectContent, SelectItem, SelectTrigger, SelectValue from the app's components barrel so EditorSection (symbols: Label, Slider, Switch, Select, SelectContent, SelectItem, SelectTrigger, SelectValue) pulls these primitives via the centralized components export to keep imports stable during refactors.apps/ui/src/lib/http-api-client.ts (1)
1270-1282: Consider reusingthis.getHeaders()to avoid duplicating header construction logic.
downloadItemmanually assembles auth headers (Lines 1272–1282) which duplicates the logic ingetHeaders()(Lines 963–982). If auth header logic changes, this method would need separate updates.Proposed simplification
async downloadItem(filePath: string): Promise<void> { - const serverUrl = getServerUrl(); - const headers: Record<string, string> = { - 'Content-Type': 'application/json', - }; - const apiKey = getApiKey(); - if (apiKey) { - headers['X-API-Key'] = apiKey; - } - const token = getSessionToken(); - if (token) { - headers['X-Session-Token'] = token; - } - - const response = await fetch(`${serverUrl}/api/fs/download`, { + await waitForApiKeyInit(); + const response = await fetch(`${this.serverUrl}/api/fs/download`, { method: 'POST', - headers, + headers: this.getHeaders(), credentials: 'include', body: JSON.stringify({ filePath }), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/lib/http-api-client.ts` around lines 1270 - 1282, downloadItem duplicates header construction currently encapsulated in getHeaders(); replace the manual assembly in downloadItem with a call to this.getHeaders() (or await this.getHeaders() if it’s async) and then merge or override the 'Content-Type' if needed (e.g., ensure 'Content-Type': 'application/json' remains). Update any type expectations (Record<string,string>) to accept the returned headers and remove the duplicated calls to getApiKey/getSessionToken/getServerUrl as appropriate; keep using getServerUrl() only for the URL but centralize auth/header logic in getHeaders() to avoid future drift.apps/ui/src/components/views/file-editor-view/components/file-tree.tsx (2)
353-368:setDragStatecalled on everydragOverevent may cause excessive re-renders.
handleDragOverfires continuously during a drag. Each invocation creates a new state object viasetDragState({ ...dragState, dropTargetPath: node.path }), triggering Zustand updates and re-renders on every mouse move. Consider guarding with a check:Proposed optimization
const handleDragOver = (e: React.DragEvent) => { e.preventDefault(); e.stopPropagation(); if (!node.isDirectory) return; const dragged = dragState.draggedPaths; const isDescendant = dragged.some((p) => node.path === p || node.path.startsWith(p + '/')); if (isDescendant) { e.dataTransfer.dropEffect = 'none'; return; } e.dataTransfer.dropEffect = 'move'; - setDragState({ ...dragState, dropTargetPath: node.path }); + if (dragState.dropTargetPath !== node.path) { + setDragState({ ...dragState, dropTargetPath: node.path }); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx` around lines 353 - 368, handleDragOver is calling setDragState({ ...dragState, dropTargetPath: node.path }) on every drag event causing frequent Zustand updates; change it to compare the current dragState.dropTargetPath with node.path and only call setDragState when they differ (and still respect node.isDirectory and the self/descendant guard). Locate the handleDragOver function and add a conditional that returns early if dragState.dropTargetPath === node.path (or if dropTargetPath is already the intended value), otherwise update dropTargetPath; this prevents redundant state writes and re-renders.
168-231:DestinationPickermodal lacks focus trapping and click-outside-to-close.The dialog renders a full-screen overlay but a keyboard user can tab out of it, and clicking the backdrop doesn't dismiss it. Consider using a
<Dialog>component from your UI library (which already handles focus trap and overlay dismiss) instead of a bespoke<div>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx` around lines 168 - 231, The DestinationPicker modal currently uses a raw full-screen div and doesn't trap focus or allow clicking the backdrop to dismiss; replace the outer wrapper with your UI library's Dialog component (e.g., <Dialog> with open/onOpenChange or onClose) so focus is trapped and overlay clicks call onCancel; pass inputRef as Dialog's initialFocus (or equivalent) to focus/select the input on open, wire Dialog onOpenChange to call onCancel when closed (so Escape/backdrop close works), and keep the existing input handlers and onSubmit logic inside the Dialog content.apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts (1)
145-163:reset()reuses the sameSet/Mapinstances frominitialState— safe today but fragile.
set(initialState)(Line 286) passes the original object whoseexpandedFolders,gitStatusMap,enhancedGitStatusMap, andselectedPathsare the sameSet/Mapinstances created at module load. If any code path ever mutates these in-place (instead of creating new instances), the mutation would persist across resets. Creating fresh collections inresetis safer:Proposed fix
- reset: () => set(initialState), + reset: () => set({ + ...initialState, + expandedFolders: new Set<string>(), + gitStatusMap: new Map<string, string>(), + enhancedGitStatusMap: new Map<string, EnhancedGitFileStatus>(), + selectedPaths: new Set<string>(), + dragState: { draggedPaths: [], dropTargetPath: null }, + }),Also applies to: 286-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts` around lines 145 - 163, The reset() call currently does set(initialState) which reuses the module-level initialState object and its collection instances; change reset() to set a new state object that clones primitive fields but creates fresh Set/Map instances for expandedFolders, gitStatusMap, enhancedGitStatusMap, and selectedPaths (rather than reusing the ones from initialState) so mutations after reset don't leak into future resets; locate reset(), initialState, and the set(initialState) usage to implement this cloning behavior.apps/server/src/routes/git/routes/enhanced-status.ts (1)
25-59: Duplicated status-label and conflict-detection logic withdetails.ts.The
getStatusLabelfunction (Lines 25–59) and the conflict/staged/unstaged derivation (Lines 136–143) are nearly identical to the logic indetails.ts(Lines 145–191). Extract a shared utility (e.g., in../common.js) to keep them in sync.As per coding guidelines, server business logic should be organized into services. Extracting this shared git-status logic into a service or shared utility would also satisfy that guideline.
Also applies to: 121-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/git/routes/enhanced-status.ts` around lines 25 - 59, Duplicate git-status and conflict-detection logic exists in getStatusLabel (enhanced-status.ts) and in details.ts; extract this into a shared utility/service (e.g., gitStatusUtils) that exports a getStatusLabel(indexStatus, workTreeStatus) and a helper to derive staged/unstaged/conflict booleans, then replace the local implementations in both enhanced-status.ts and details.ts to import and call the shared functions (preserve the same return strings and behavior: Conflicted, Staged, Staged + Modified, Modified, Added, Deleted, Renamed, Copied, Untracked, or default char). Ensure the new module is used by both places so business logic lives in a single service and both files simply import and call gitStatusUtils.getStatusLabel and the conflict/staged detection helpers.apps/ui/src/components/views/file-editor-view/file-editor-view.tsx (3)
356-449:tabsin the dependency array makeshandleFileSelectidentity unstable, cascading to many downstream callbacks.The
tabsdependency is only needed for the.find()check on line 359. UsinguseFileEditorStore.getState().tabsinside the callback (same pattern already used byhandleCursorChangeandhandleEditorChange) would keep the reference stable and avoid re-creatinghandleMobileFileSelect,handleCreateFile,handleRenameItem,handleMoveItem, and the initial-path effect on every tab change.♻️ Proposed fix
const handleFileSelect = useCallback( async (filePath: string) => { // Check if already open - const existing = tabs.find((t) => t.filePath === filePath); + const existing = useFileEditorStore.getState().tabs.find((t) => t.filePath === filePath); if (existing) { setActiveTab(existing.id); return; } // ... rest unchanged ... }, - [tabs, setActiveTab, openTab, maxFileSize] + [setActiveTab, openTab, maxFileSize] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around lines 356 - 449, The handleFileSelect callback should not depend on the changing "tabs" reference; replace the tabs.find(...) call with a direct read from the store (useFileEditorStore.getState().tabs) so the callback identity stays stable. In the handleFileSelect function (the useCallback that currently closes over tabs), change the lookup to useFileEditorStore.getState().tabs.find(...) and remove "tabs" from the dependency array, leaving only stable dependencies like setActiveTab, openTab, and maxFileSize.
525-551: Auto-save timer depends onhandleSave, which changes on every content edit — timer resets continuously.
handleSavecapturesactiveTab(line 480), so it gets a new identity on every keystroke. The effect on line 525 clears and re-sets the debounce timer each time, which is arguably the desired debounce behavior. However, the same result can be achieved more cleanly by depending onsaveTabById(already stable) and calling it with the active tab ID, avoiding the indirect coupling throughhandleSave.♻️ Proposed simplification
useEffect(() => { - if (!editorAutoSave || !activeTab || !activeTab.isDirty) { + if (!editorAutoSave || !activeTabId) { if (autoSaveTimerRef.current) { clearTimeout(autoSaveTimerRef.current); autoSaveTimerRef.current = null; } return; } + const { tabs: currentTabs } = useFileEditorStore.getState(); + const tab = currentTabs.find((t) => t.id === activeTabId); + if (!tab?.isDirty) return; + if (autoSaveTimerRef.current) { clearTimeout(autoSaveTimerRef.current); } autoSaveTimerRef.current = setTimeout(() => { - handleSave(); + saveTabById(activeTabId); autoSaveTimerRef.current = null; }, editorAutoSaveDelay); return () => { if (autoSaveTimerRef.current) { clearTimeout(autoSaveTimerRef.current); autoSaveTimerRef.current = null; } }; - }, [editorAutoSave, editorAutoSaveDelay, activeTab?.isDirty, activeTab?.content, handleSave]); + }, [editorAutoSave, editorAutoSaveDelay, activeTabId, saveTabById]);Note: You'd still need the effect to trigger on content changes. Since
activeTab?.contentchanges are the desired trigger, you may keep that dep. But this removes thehandleSavedependency which recreates on every activeTab change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around lines 525 - 551, The effect currently depends on handleSave (which changes on every edit) causing unnecessary timer resets; replace the dependence on handleSave with the stable saveTabById and invoke saveTabById(activeTab.id) inside the timeout instead of calling handleSave. Keep the other deps (editorAutoSave, editorAutoSaveDelay, activeTab?.isDirty, activeTab?.content), guard that activeTab exists before scheduling, and continue to clear autoSaveTimerRef in the cleanup so the debounce behavior is preserved without coupling to a recreating handleSave function.
1329-1395: Editor settings UI is duplicated between the desktop popover and the mobile actions panel.The font size slider, font family selector, and auto-save toggle are rendered identically in two places. Extracting a small
<EditorSettingsControls />component would eliminate this duplication and ensure settings changes stay in sync across both surfaces.Also applies to: 1515-1579
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around lines 1329 - 1395, Extract the duplicated editor settings UI (the Font Size Slider, Font Family Select, and Auto Save Switch currently rendered inside <PopoverContent> and the mobile actions panel) into a new presentational component (e.g., EditorSettingsControls) and use it in both places; the component should accept props for editorFontSize, setEditorFontSize, editorFontFamily, setEditorFontFamily, editorAutoSave, setEditorAutoSave and reuse constants DEFAULT_FONT_VALUE and UI_MONO_FONT_OPTIONS, plus the same Slider/Select/Switch markup (including the Reset button using setEditorFontSize(13)), so both PopoverContent and the mobile actions panel render the same single source of truth UI and remain in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/fs/routes/copy.ts`:
- Around line 31-85: Add a guard in createCopyHandler to prevent copying a
directory into its own descendant: after resolving and stat-ing the source
(before calling copyDirectoryRecursive) compute resolvedSource =
path.resolve(sourcePath) and resolvedDest = path.resolve(destinationPath) and if
stats.isDirectory() and resolvedDest.startsWith(resolvedSource + path.sep) then
return an error (e.g., 400/409) indicating destination is a subpath of source;
this prevents infinite recursion in copyDirectoryRecursive and mirrors the
move.ts descendant check.
In `@apps/server/src/routes/fs/routes/download.ts`:
- Around line 69-76: The current use of execAsync interpolating filePath and
fileName into a shell command (the execAsync call that runs `cd
"${path.dirname(filePath)}" && zip -r "${tmpZipPath}" "${fileName}"`) is
vulnerable to command injection; replace this with a non-shell API or safe
zipping library: either call a child-process method that bypasses the shell
(e.g., execFile/spawn with arguments) and pass path.dirname(filePath),
tmpZipPath and fileName as separate, validated arguments, or remove the shell
entirely and use a Node zipping library (e.g., archiver) to create tmpZipPath
from filePath; also add strict validation/normalization of filePath/fileName
(from req.body) before use and ensure tmpZipPath is controlled by the server.
In `@apps/server/src/routes/fs/routes/move.ts`:
- Around line 28-37: The current descendant check only prevents moving into a
child but misses the case where resolvedDest === resolvedSrc; update the move
handler to treat a move-to-self as invalid: after computing resolvedSrc and
resolvedDest (variables resolvedSrc and resolvedDest) add an equality check
(resolvedDest === resolvedSrc) and respond with a 400 JSON error (same shape as
the descendant error) and return early; ensure this check runs before any
overwrite logic that may unlink paths so we never remove the source when
overwrite: true.
In `@apps/server/src/routes/git/index.ts`:
- Line 23: The /details route accepts an optional filePath and currently calls
router.post('/details', validatePathParams('projectPath'),
createDetailsHandler()) without validating/confining filePath, so update the
middleware call to validate the optional filePath as well by adding the optional
param to validatePathParams (e.g., pass 'filePath?' alongside 'projectPath') so
createDetailsHandler() receives a path-confined filePath; ensure you only change
the middleware args on the router.post line that registers createDetailsHandler.
In `@apps/server/src/routes/git/routes/details.ts`:
- Around line 62-66: The git command invocations in the details route use
execAsync with a template-interpolated filePath (e.g., the `execAsync` call that
runs `git log -1 ... -- "${filePath}"`), which allows shell injection; replace
these with a shell-bypassing call that passes args as an array (e.g., swap
`execAsync`/`exec` usage for an `execFile`/`spawnFile` style helper or a
promisified `child_process.execFile` equivalent) and pass the git subcommand and
filePath as separate arguments rather than interpolating into a single shell
string; apply this change to every occurrence where `filePath` is interpolated
(the `execAsync` calls around the `git log`, `git show`, and other git commands
in this module) so the filePath is not interpreted by a shell, or alternatively
validate/reject filePath values for shell metacharacters before use if you
cannot switch to execFile.
In `@apps/ui/src/components/views/file-editor-view/components/code-editor.tsx`:
- Around line 522-566: The useMemo for "extensions" rebuilds whenever onSave or
onCursorChange change; replace direct callback usage with refs to avoid unstable
inline functions causing needless reconfiguration: create refs (e.g., onSaveRef
and onCursorChangeRef) and update them in a useEffect after render, then inside
EditorView.updateListener.of(...) and the keymap run() call, invoke the current
callbacks via onCursorChangeRef.current(...) and onSaveRef.current(); remove
onSave and onCursorChange from the useMemo dependency list so the extensions
depend only on structural props (filePath, wordWrap, tabSize, editorTheme) while
still calling the latest handlers via the refs.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx`:
- Around line 662-723: The current handleRenameItem implementation recreates
directories with api.mkdir then deletes the original (api.deleteFile), causing
data loss; replace that flow by adding/using an atomic rename operation on the
IPC/server API and calling it here instead of mkdir+deleteFile. Specifically,
extend the ElectronAPI with a rename(oldPath, newPath) method (and corresponding
server route that uses fs.rename), then in handleRenameItem (the function using
getElectronAPI() and calling api.readFile/api.writeFile/api.deleteFile) call
api.rename(oldPath, newPath) for both files and directories; remove the
mkdir+deleteFile block, keep the tab update logic (tabs.find, closeTab,
handleFileSelect/handleMobileFileSelect), preserve the hidden-file toggle
(useFileEditorStore/getState and store.setShowHiddenFiles), and still call
loadTree(preserveExpanded: true) and loadGitStatus() after the rename.
In `@apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts`:
- Line 1: The editor store must be wrapped with Zustand's persist middleware so
tabs, activeTabId, cursorLine, cursorCol, scrollTop (and related collections)
survive reloads; update the create(...) call in use-file-editor-store.ts to
compose persist(...) around the existing store initializer, supply a unique name
key, add a migration handler consistent with
ui-cache-store.ts/ideation-store.ts, and provide a custom storage adapter
(serialize/deserialize) that converts Set fields like expandedFolders and
selectedPaths to/from arrays to preserve their contents across storage; ensure
the persisted state shape includes tabs, activeTabId, cursorLine, cursorCol,
scrollTop and that the restore logic maps fields back to Set where needed.
In `@apps/ui/src/hooks/use-settings-migration.ts`:
- Around line 761-763: The migration/hydration is missing the editorAutoSave and
editorAutoSaveDelay keys so users' auto-save preferences are not persisted; add
editorAutoSave: settings.editorAutoSave ?? <default> and editorAutoSaveDelay:
settings.editorAutoSaveDelay ?? <defaultDelay> alongside editorFontSize and
editorFontFamily in the same hydration/migration object (i.e., the settings
mapping where editorFontSize and editorFontFamily are set), and mirror the same
additions in the other migration spot that currently sets
editorFontSize/editorFontFamily so both hydration and migration include
editorAutoSave and editorAutoSaveDelay.
In `@apps/ui/src/lib/http-api-client.ts`:
- Around line 1270-1310: downloadItem currently uses raw fetch and only checks
response.ok, so it doesn't trigger the same auth flow as the post() helper;
update downloadItem to detect 401/403 responses and call the existing
handleUnauthorized() handler (the same used by post()) before throwing, ensuring
the user is redirected/cleared on session expiry; locate downloadItem and add
logic after receiving response to if (response.status === 401 || response.status
=== 403) { await handleUnauthorized(); throw new Error('Unauthorized'); } (or
equivalent) so authorization failures are handled consistently.
---
Duplicate comments:
In `@apps/ui/src/components/views/file-editor-view/components/editor-tabs.tsx`:
- Around line 4-9: The imports in editor-tabs.tsx currently pull UI primitives
directly from '@/components/ui/dropdown-menu'; update them to use the app's
barrel export instead by importing DropdownMenu, DropdownMenuContent,
DropdownMenuItem, and DropdownMenuTrigger from the components index (e.g.,
'@/components' or the app's ../components barrel) so the file uses the
centralized barrel export pattern for UI primitives.
In
`@apps/ui/src/components/views/file-editor-view/components/worktree-directory-dropdown.tsx`:
- Around line 14-22: Replace the direct imports of UI primitives with the app's
barrel export: instead of importing Button and the DropdownMenu* symbols from
'@/components/ui/button' and '@/components/ui/dropdown-menu', import Button,
DropdownMenu, DropdownMenuContent, DropdownMenuItem, DropdownMenuLabel,
DropdownMenuSeparator, and DropdownMenuTrigger from the components index barrel
(i.e., '@/components') so the file worktree-directory-dropdown.tsx uses the
centralized barrel exports for these UI primitives.
---
Nitpick comments:
In `@apps/server/src/routes/fs/routes/move.ts`:
- Around line 60-61: secureFs.rename(sourcePath, destinationPath) can throw
EXDEV when moving across filesystems; update the move handler to catch errors
from secureFs.rename and if err.code === 'EXDEV' perform a fallback
copy-then-delete (e.g., use secureFs.copyFile or stream copy from sourcePath to
destinationPath, then secureFs.unlink(sourcePath) once copy succeeds) and
propagate any errors; if cross-device moves are unsupported, return a clear,
specific error message referencing EXDEV instead of letting the raw exception
bubble up.
In `@apps/server/src/routes/git/index.ts`:
- Around line 23-24: The route handlers createDetailsHandler and
createEnhancedStatusHandler contain business logic (running git shell commands,
parsing porcelain output, building responses); move that logic into new service
functions under services/ (e.g., gitService.getDetails(projectPath) and
gitService.getEnhancedStatus(projectPath)) and refactor the handlers to only
validate input and call these service methods, returning their results; ensure
the git command execution, parsing, and response-shaping code is extracted
intact into the service, add unit tests for the new service functions, and
update imports in the routes to call the service APIs instead of performing git
work inline.
In `@apps/server/src/routes/git/routes/enhanced-status.ts`:
- Around line 25-59: Duplicate git-status and conflict-detection logic exists in
getStatusLabel (enhanced-status.ts) and in details.ts; extract this into a
shared utility/service (e.g., gitStatusUtils) that exports a
getStatusLabel(indexStatus, workTreeStatus) and a helper to derive
staged/unstaged/conflict booleans, then replace the local implementations in
both enhanced-status.ts and details.ts to import and call the shared functions
(preserve the same return strings and behavior: Conflicted, Staged, Staged +
Modified, Modified, Added, Deleted, Renamed, Copied, Untracked, or default
char). Ensure the new module is used by both places so business logic lives in a
single service and both files simply import and call
gitStatusUtils.getStatusLabel and the conflict/staged detection helpers.
In `@apps/ui/package.json`:
- Around line 45-55: Replace the eleven individual `@codemirror/lang-`*
dependencies with the single `@uiw/codemirror-extensions-langs` package: remove
the entries for "@codemirror/lang-cpp", "-css", "-html", "-java", "-javascript",
"-json", "-markdown", "-php", "-python", "-rust", and "-sql" from package.json
and add "@uiw/codemirror-extensions-langs" (compatible with
`@uiw/react-codemirror` v4.25.0); then update any imports in your CodeMirror setup
components (places that import from "@codemirror/lang-*" or reference those
language extension symbols) to import the extension map from
"@uiw/codemirror-extensions-langs" and use its language-by-extension lookup
instead of individual language constructors.
In `@apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts`:
- Around line 160-164: The nav item with id 'file-editor' in use-navigation.ts
is missing a shortcut property and corresponding keyboard mapping; either add a
shortcut string (e.g., 'ctrl+e') to the 'file-editor' entry and add the same key
to the shortcuts interface/definition used by the sidebar shortcuts mapping
(where other ids like 'board', 'graph', 'agent', 'terminal' are defined), or if
no shortcut is intended, add an inline comment on the 'file-editor' nav item
explaining the omission; locate and update the nav item object with id
'file-editor' and the shortcuts interface/mapping where other shortcut keys are
declared to keep them consistent.
In `@apps/ui/src/components/views/file-editor-view/components/code-editor.tsx`:
- Around line 78-296: getLanguageExtension and getLanguageName duplicate the
same extension-to-language logic; extract a single shared lookup (e.g., const
languageMap or an object keyed by extension/name) that maps file extensions and
special basenames to both the CodeMirror Extension factory (or
StreamLanguage.define wrapper) and the human-readable name, then refactor
getLanguageExtension to look up and return the Extension (or null) and
getLanguageName to return the display name (or 'Plain Text'); preserve
special-case handling for filenames like 'dockerfile', 'makefile', '.env', and
dotfiles and ensure the fallback behaviors (null for extension, 'Plain Text' for
name) remain unchanged, updating references in both getLanguageExtension and
getLanguageName to use the new shared mapping.
In `@apps/ui/src/components/views/file-editor-view/components/file-tree.tsx`:
- Around line 353-368: handleDragOver is calling setDragState({ ...dragState,
dropTargetPath: node.path }) on every drag event causing frequent Zustand
updates; change it to compare the current dragState.dropTargetPath with
node.path and only call setDragState when they differ (and still respect
node.isDirectory and the self/descendant guard). Locate the handleDragOver
function and add a conditional that returns early if dragState.dropTargetPath
=== node.path (or if dropTargetPath is already the intended value), otherwise
update dropTargetPath; this prevents redundant state writes and re-renders.
- Around line 168-231: The DestinationPicker modal currently uses a raw
full-screen div and doesn't trap focus or allow clicking the backdrop to
dismiss; replace the outer wrapper with your UI library's Dialog component
(e.g., <Dialog> with open/onOpenChange or onClose) so focus is trapped and
overlay clicks call onCancel; pass inputRef as Dialog's initialFocus (or
equivalent) to focus/select the input on open, wire Dialog onOpenChange to call
onCancel when closed (so Escape/backdrop close works), and keep the existing
input handlers and onSubmit logic inside the Dialog content.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx`:
- Around line 356-449: The handleFileSelect callback should not depend on the
changing "tabs" reference; replace the tabs.find(...) call with a direct read
from the store (useFileEditorStore.getState().tabs) so the callback identity
stays stable. In the handleFileSelect function (the useCallback that currently
closes over tabs), change the lookup to
useFileEditorStore.getState().tabs.find(...) and remove "tabs" from the
dependency array, leaving only stable dependencies like setActiveTab, openTab,
and maxFileSize.
- Around line 525-551: The effect currently depends on handleSave (which changes
on every edit) causing unnecessary timer resets; replace the dependence on
handleSave with the stable saveTabById and invoke saveTabById(activeTab.id)
inside the timeout instead of calling handleSave. Keep the other deps
(editorAutoSave, editorAutoSaveDelay, activeTab?.isDirty, activeTab?.content),
guard that activeTab exists before scheduling, and continue to clear
autoSaveTimerRef in the cleanup so the debounce behavior is preserved without
coupling to a recreating handleSave function.
- Around line 1329-1395: Extract the duplicated editor settings UI (the Font
Size Slider, Font Family Select, and Auto Save Switch currently rendered inside
<PopoverContent> and the mobile actions panel) into a new presentational
component (e.g., EditorSettingsControls) and use it in both places; the
component should accept props for editorFontSize, setEditorFontSize,
editorFontFamily, setEditorFontFamily, editorAutoSave, setEditorAutoSave and
reuse constants DEFAULT_FONT_VALUE and UI_MONO_FONT_OPTIONS, plus the same
Slider/Select/Switch markup (including the Reset button using
setEditorFontSize(13)), so both PopoverContent and the mobile actions panel
render the same single source of truth UI and remain in sync.
In `@apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts`:
- Around line 145-163: The reset() call currently does set(initialState) which
reuses the module-level initialState object and its collection instances; change
reset() to set a new state object that clones primitive fields but creates fresh
Set/Map instances for expandedFolders, gitStatusMap, enhancedGitStatusMap, and
selectedPaths (rather than reusing the ones from initialState) so mutations
after reset don't leak into future resets; locate reset(), initialState, and the
set(initialState) usage to implement this cloning behavior.
In `@apps/ui/src/components/views/settings-view/editor/editor-section.tsx`:
- Around line 1-10: Replace the multiple direct UI primitive imports with a
single barrel import from the components index: import Label, Slider, Switch,
Select, SelectContent, SelectItem, SelectTrigger, SelectValue from the app's
components barrel so EditorSection (symbols: Label, Slider, Switch, Select,
SelectContent, SelectItem, SelectTrigger, SelectValue) pulls these primitives
via the centralized components export to keep imports stable during refactors.
In `@apps/ui/src/lib/http-api-client.ts`:
- Around line 1270-1282: downloadItem duplicates header construction currently
encapsulated in getHeaders(); replace the manual assembly in downloadItem with a
call to this.getHeaders() (or await this.getHeaders() if it’s async) and then
merge or override the 'Content-Type' if needed (e.g., ensure 'Content-Type':
'application/json' remains). Update any type expectations
(Record<string,string>) to accept the returned headers and remove the duplicated
calls to getApiKey/getSessionToken/getServerUrl as appropriate; keep using
getServerUrl() only for the URL but centralize auth/header logic in getHeaders()
to avoid future drift.
In `@apps/ui/src/routes/file-editor.lazy.tsx`:
- Line 2: Replace the direct import of FileEditorView with the barrel export:
update the import statement that currently references
'@/components/views/file-editor-view/file-editor-view' to import FileEditorView
from the view's index barrel (the components/views/file-editor-view index
export) so the module uses the barrel export for stability and refactorability,
keeping the imported symbol name FileEditorView unchanged.
In `@apps/ui/src/types/electron.d.ts`:
- Line 665: The downloadItem declaration currently returns Promise<void> but
should match the other ElectronAPI file-system methods (readFile, writeFile,
mkdir, deleteFile, copyItem, moveItem) by returning Promise<{ success: boolean;
error?: string }>; update the downloadItem signature in
apps/ui/src/types/electron.d.ts to Promise<{ success: boolean; error?: string
}>, then update the corresponding implementation and any callers to return and
handle a { success, error } object (ensure errors are caught and returned as {
success: false, error: error.message } and successes as { success: true }).
In `@libs/types/src/settings.ts`:
- Around line 1008-1016: DEFAULT_GLOBAL_SETTINGS is missing the documented
defaults for the new file editor fields; update the DEFAULT_GLOBAL_SETTINGS
object to include editorFontSize: 13, editorFontFamily: 'default',
editorAutoSave: false, and editorAutoSaveDelay: 1000 so the canonical defaults
match the comments and other settings like serverLogLevel, ensuring these keys
(editorFontSize, editorFontFamily, editorAutoSave, editorAutoSaveDelay) are
present with the specified values in DEFAULT_GLOBAL_SETTINGS.
Comment on lines
28
to
37
| // Prevent moving a folder into its own descendant | ||
| const resolvedSrc = path.resolve(sourcePath); | ||
| const resolvedDest = path.resolve(destinationPath); | ||
| if (resolvedDest.startsWith(resolvedSrc + path.sep)) { | ||
| res.status(400).json({ | ||
| success: false, | ||
| error: 'Cannot move a folder into one of its own descendants', | ||
| }); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Self-move with overwrite: true would delete the source.
The descendant check on Line 31 doesn't catch resolvedDest === resolvedSrc. If a client sends a request to move a file to itself with overwrite: true, Line 52 removes the destination (which is also the source), then the rename on Line 61 fails on a now-deleted path.
🐛 Proposed fix
const resolvedSrc = path.resolve(sourcePath);
const resolvedDest = path.resolve(destinationPath);
+ if (resolvedDest === resolvedSrc) {
+ // No-op: source and destination are the same
+ res.json({ success: true });
+ return;
+ }
if (resolvedDest.startsWith(resolvedSrc + path.sep)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/routes/fs/routes/move.ts` around lines 28 - 37, The current
descendant check only prevents moving into a child but misses the case where
resolvedDest === resolvedSrc; update the move handler to treat a move-to-self as
invalid: after computing resolvedSrc and resolvedDest (variables resolvedSrc and
resolvedDest) add an equality check (resolvedDest === resolvedSrc) and respond
with a 400 JSON error (same shape as the descendant error) and return early;
ensure this check runs before any overwrite logic that may unlink paths so we
never remove the source when overwrite: true.
apps/ui/src/components/views/file-editor-view/components/code-editor.tsx
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,287 @@ | |||
| import { create } from 'zustand'; | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the use-file-editor-store.ts file content around lines 1 and 165-287
cat -n apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts | head -50Repository: AutoMaker-Org/automaker
Length of output: 1706
🏁 Script executed:
# Check if other Zustand stores use persist middleware
rg -n 'persist' --type ts -g '*store*' apps/ui/src/Repository: AutoMaker-Org/automaker
Length of output: 2565
🏁 Script executed:
# Get a broader view of the file structure
wc -l apps/ui/src/components/views/file-editor-view/use-file-editor-store.tsRepository: AutoMaker-Org/automaker
Length of output: 141
🏁 Script executed:
sed -n '160,287p' apps/ui/src/components/views/file-editor-view/use-file-editor-store.tsRepository: AutoMaker-Org/automaker
Length of output: 3645
🏁 Script executed:
# Check if there are any other file-editor related stores in the standard store directory
find apps/ui/src/store -name '*editor*' -o -name '*file*' 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 49
Add Zustand persist middleware to the editor store to maintain open files, active tabs, and cursor positions across page reloads.
The store manages state (tabs, activeTabId, cursorLine, cursorCol, scrollTop) that should survive navigation and page reloads per the PR description and coding guidelines. Without the persist middleware, this data is lost. Other similar stores in the project (ui-cache-store.ts, ideation-store.ts) use persist with migration handlers. Add the middleware following the established pattern, using a custom storage adapter if needed to handle the Set type in expandedFolders and selectedPaths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts` at
line 1, The editor store must be wrapped with Zustand's persist middleware so
tabs, activeTabId, cursorLine, cursorCol, scrollTop (and related collections)
survive reloads; update the create(...) call in use-file-editor-store.ts to
compose persist(...) around the existing store initializer, supply a unique name
key, add a migration handler consistent with
ui-cache-store.ts/ideation-store.ts, and provide a custom storage adapter
(serialize/deserialize) that converts Set fields like expandedFolders and
selectedPaths to/from arrays to preserve their contents across storage; ensure
the persisted state shape includes tabs, activeTabId, cursorLine, cursorCol,
scrollTop and that the restore logic maps fields back to Set where needed.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
apps/ui/src/components/views/file-editor-view/components/code-editor.tsx (1)
78-202:getLanguageExtensionandgetLanguageNameduplicate filename parsing and extension matching.Both functions independently parse filenames and switch on extensions. Consider extracting a shared lookup table or a common parser to keep these in sync and reduce the chance of them diverging when new languages are added.
Also applies to: 205-296
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/code-editor.tsx` around lines 78 - 202, getLanguageExtension and getLanguageName duplicate filename parsing/extension matching; extract the shared parsing and mapping into a single source of truth (e.g., a lookup map and a helper parseFileName function) and have both getLanguageExtension and getLanguageName call that helper. Specifically, create a parseFileInfo(filePath: string) that returns { name, ext } and a single mapping object (e.g., LANGUAGE_BY_NAME and LANGUAGE_BY_EXT or one combined map) that maps filenames and extensions to the language descriptor/identifier used by getLanguageExtension (Extension) and getLanguageName (string), then replace the duplicated switch logic in getLanguageExtension and getLanguageName with lookups into that shared map; ensure special-case filenames like 'Dockerfile', '.env', and dotfiles are present in the mapping so behavior is preserved.apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts (1)
146-164:reset()reuses the sameinitialStateobject reference — shared mutableSetandMapinstances.
initialStateis defined once at module scope. Each call toreset()passes the sameSetandMapinstances (e.g.,expandedFolders,gitStatusMap,selectedPaths). While current store actions always create new instances, any future code that mutates these collections in-place would corrupt subsequent resets.♻️ Proposed fix — spread fresh collections on reset
- reset: () => set(initialState), + reset: () => + set({ + ...initialState, + expandedFolders: new Set<string>(), + gitStatusMap: new Map<string, string>(), + enhancedGitStatusMap: new Map<string, EnhancedGitFileStatus>(), + selectedPaths: new Set<string>(), + dragState: { draggedPaths: [], dropTargetPath: null }, + }),Also applies to: 297-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts` around lines 146 - 164, initialState currently holds module-scoped Set/Map instances (expandedFolders, selectedPaths, gitStatusMap, enhancedGitStatusMap) which are reused by reset(), causing shared mutable state; change reset() to recreate fresh collection instances instead of reusing initialState's references (either implement a getInitialState() factory that returns a new object each call or in reset() spread/construct new Set/Map for expandedFolders, selectedPaths, gitStatusMap, enhancedGitStatusMap and assign them to the store); update references to initialState in the file (e.g., the initialState constant and the reset() implementation in use-file-editor-store.ts) so every reset gets brand-new collections rather than shared ones.apps/ui/src/components/views/file-editor-view/file-editor-view.tsx (1)
1319-1385: Editor settings UI is duplicated between the desktop Popover and the mobile HeaderActionsPanel.The font-size slider, font-family selector, and auto-save toggle are copy-pasted between the desktop popover (Lines 1319–1385) and the mobile actions panel (Lines 1504–1569). Extract a shared
EditorSettingsFormcomponent to keep them in sync and reduce maintenance burden.Also applies to: 1504-1569
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around lines 1319 - 1385, Extract the duplicated editor settings markup into a new presentational component named EditorSettingsForm that accepts props { editorFontSize, setEditorFontSize, editorFontFamily, setEditorFontFamily, editorAutoSave, setEditorAutoSave } (and any constants like DEFAULT_FONT_VALUE and UI_MONO_FONT_OPTIONS can be imported by the new component). Move the Slider, Select and Switch blocks (the font-size, font-family and auto-save UI) into EditorSettingsForm and wire them to the passed props (use the same value handlers: setEditorFontSize, setEditorFontFamily, setEditorAutoSave). Replace the duplicated markup in the PopoverContent and in HeaderActionsPanel with a single <EditorSettingsForm .../> instance, passing the local state and setters so both desktop and mobile use the same implementation. Ensure styling/className props remain applied by wrapping EditorSettingsForm where needed so visual layout does not change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/routes/fs/routes/copy.ts`:
- Around line 59-74: The current logic removes the destination
(secureFs.rm(destinationPath)) before copying, which is non-atomic and can cause
data loss; change the flow to copy the source into a uniquely named temporary
sibling (e.g., destinationPath + ".tmp-<random>" or use a tmp directory) and
once the copy to that temp path completes successfully, atomically rename/move
the temp into destinationPath (using secureFs.rename or equivalent) to replace
it; ensure you generate a unique temp name, handle errors by removing the temp
on failure, and only call secureFs.rm on the original destination if you must
and after a successful swap (update the code around secureFs.stat, secureFs.rm,
and the copy/rename steps to implement this swap pattern).
- Around line 15-29: The recursive copy function copyDirectoryRecursive
currently follows symlinks because entry.isDirectory() can be true for symlinked
directories, risking infinite recursion; update the loop to detect symlinks via
entry.isSymbolicLink() and handle them (either skip or shallow-copy the symlink)
instead of descending: call secureFs.lstat on srcPath or use
entry.isSymbolicLink() to detect symlinks, if symlink then either recreate the
link at dest with secureFs.symlink (or skip) and do not recurse, otherwise if
it's a real directory use mkdirSafe(destPath) and await
copyDirectoryRecursive(srcPath, destPath), and for files continue to use
secureFs.copyFile; ensure you reference copyDirectoryRecursive,
entry.isDirectory(), entry.isSymbolicLink(), secureFs.copyFile, mkdirSafe when
making the changes.
In `@apps/server/src/routes/fs/routes/download.ts`:
- Around line 70-75: The code currently shells out to the system zip utility via
execFileAsync('zip', ...) using tmpZipPath, fileName and filePath which is not
portable; replace this by either checking for the presence of the zip binary at
startup and failing fast (or falling back) or, preferably, stop using
execFileAsync here and implement zipping in-process using a Node library such as
archiver: create a zip archive of fileName rooted at path.dirname(filePath) and
write to tmpZipPath (or stream directly to the response) instead of calling
execFileAsync, and update the handler that references tmpZipPath, fileName,
filePath and execFileAsync accordingly.
- Around line 79-81: The Content-Disposition header is built from
user-controlled fileName/zipFileName and is vulnerable to header injection;
update the code that sets the header (the res.setHeader('Content-Disposition',
...) call using zipFileName and the analogous call using fileName at lines
~118-120) to sanitize and RFC5987-encode filenames: strip/replace CR/LF and
quotes, restrict/control non-ASCII or normalize them, then set both a safe ASCII
filename (filename="...") and an RFC5987-encoded filename*
(filename*=UTF-8''...) per RFC5987; implement a small helper (e.g.,
encodeAttachmentFilename or sanitizeFilename) and use it where
zipFileName/fileName are interpolated.
- Around line 21-36: getDirectorySize currently follows symlinks via
entry.isDirectory() which can cause unbounded recursion on symlink cycles;
update getDirectorySize to avoid following symlinks or detect cycles by using
secureFs.lstat to check for symbolic links and either skip them or resolve their
realpath and track visited targets (e.g., a Set of realpaths or device+inode
from stat) before recursing, and use secureFs.stat only after deciding to follow
a link; ensure the logic updates references in getDirectorySize
(entry.isDirectory(), secureFs.stat, secureFs.readdir) so recursive calls never
re-enter an already visited path.
In `@apps/server/src/routes/fs/routes/move.ts`:
- Around line 62-66: secureFs.rename can throw EXDEV across filesystem
boundaries; wrap the call in a try/catch in the move handler (the code around
mkdirSafe and secureFs.rename) and on EXDEV fall back to a copy-then-delete
move: for files use fs.promises.copyFile then fs.promises.unlink, and for
directories perform a recursive copy (preserving mode/mtime) and then remove the
source recursively (or use a vetted utility that does this atomically); ensure
errors from copy or delete propagate as proper HTTP errors and only delete the
source after a successful copy.
In `@apps/ui/src/components/views/file-editor-view/components/code-editor.tsx`:
- Around line 534-599: The extensions array currently pushes an explicit
search() extension while the CodeMirror component's basicSetup also includes
search by default, causing duplicate search UI/keybinding conflicts; fix by
either removing the explicit search() from the extensions array (the entry at
the top of the exts array) or disabling search in the CodeMirror basicSetup (the
basicSetup prop) so only one search extension is active—locate the search() call
in the useMemo that builds extensions and the basicSetup object on the
CodeMirror props (symbols: search(), extensions, basicSetup, CodeMirror) and
modify one of them to eliminate the duplicate.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx`:
- Around line 60-105: The BINARY_EXTENSIONS Set in file-editor-view.tsx
incorrectly treats 'svg' as binary which prevents text editing; remove the 'svg'
entry from the BINARY_EXTENSIONS constant so SVG files are handled as text (and
will open in the code editor with XML syntax highlighting) and verify any
downstream checks that rely on BINARY_EXTENSIONS still behave correctly for SVG
files.
- Around line 662-713: handleRenameItem currently calls api.moveItem?.(...) and
if moveItem is undefined the result is undefined, causing toast.error to show an
empty description; add an explicit guard that checks whether api.moveItem exists
(from getElectronAPI()) before calling it and, if missing, immediately
toast.error with a clear message (e.g., "Rename API unavailable") and return;
otherwise call api.moveItem, handle result.success as now, and include
result.error when present in toast.error to avoid confusing undefined
descriptions.
- Around line 462-480: handleSave and saveTabById only log save failures; update
both to surface user-facing notifications: when a save succeeds call the
existing UI toast/notification function (e.g., showToast or notify) with a brief
success message and file name, and when a save fails catch the
error/result.error and call the toast with an error message including the error
text so the user is informed; place these calls alongside the existing
logger.error branches in handleSave (use getElectronAPI(), markTabSaved,
loadGitStatus references) and in saveTabById (the auto-save path) so both manual
and auto-save failures/successes notify the user.
- Around line 835-849: handleDragDropMove currently awaits each handleMoveItem
and will abort on the first thrown error, leaving previously moved items in
place; change it so the loop catches per-item errors, continues processing
remaining sourcePaths, and aggregates failures to present a single consolidated
error/summary at the end (use sourcePaths, targetFolderPath, handleMoveItem and
fileName/destinationPath variables to locate the logic). Additionally, implement
a compensating rollback strategy: if you want atomic-like behavior, on any
failure attempt to move already-successful items back to their original paths by
calling handleMoveItem with swapped source/destination (record moved pairs as
you go), and collect any rollback failures to include in the final aggregated
report; ensure the function still returns/resolves after presenting the
aggregated result to the caller.
In `@apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts`:
- Around line 305-311: The current partialize persists full tabs (EditorTab)
including content and originalContent which can exceed localStorage quota;
update partialize to persist only tab metadata (e.g., id, path, name,
cursor/selection, unsaved flag) by mapping state.tabs to omit
content/originalContent before casting to FileEditorState, and implement/ensure
a restore path (e.g., in the tab restore/open logic or a loadFileContent
function) that reloads file content on-demand when a tab is reactivated;
optionally add a size cap or skip persisting tabs if total serialized size
exceeds a threshold to avoid localStorage write failures and keep existing error
handling for storage writes.
---
Nitpick comments:
In `@apps/ui/src/components/views/file-editor-view/components/code-editor.tsx`:
- Around line 78-202: getLanguageExtension and getLanguageName duplicate
filename parsing/extension matching; extract the shared parsing and mapping into
a single source of truth (e.g., a lookup map and a helper parseFileName
function) and have both getLanguageExtension and getLanguageName call that
helper. Specifically, create a parseFileInfo(filePath: string) that returns {
name, ext } and a single mapping object (e.g., LANGUAGE_BY_NAME and
LANGUAGE_BY_EXT or one combined map) that maps filenames and extensions to the
language descriptor/identifier used by getLanguageExtension (Extension) and
getLanguageName (string), then replace the duplicated switch logic in
getLanguageExtension and getLanguageName with lookups into that shared map;
ensure special-case filenames like 'Dockerfile', '.env', and dotfiles are
present in the mapping so behavior is preserved.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx`:
- Around line 1319-1385: Extract the duplicated editor settings markup into a
new presentational component named EditorSettingsForm that accepts props {
editorFontSize, setEditorFontSize, editorFontFamily, setEditorFontFamily,
editorAutoSave, setEditorAutoSave } (and any constants like DEFAULT_FONT_VALUE
and UI_MONO_FONT_OPTIONS can be imported by the new component). Move the Slider,
Select and Switch blocks (the font-size, font-family and auto-save UI) into
EditorSettingsForm and wire them to the passed props (use the same value
handlers: setEditorFontSize, setEditorFontFamily, setEditorAutoSave). Replace
the duplicated markup in the PopoverContent and in HeaderActionsPanel with a
single <EditorSettingsForm .../> instance, passing the local state and setters
so both desktop and mobile use the same implementation. Ensure styling/className
props remain applied by wrapping EditorSettingsForm where needed so visual
layout does not change.
In `@apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts`:
- Around line 146-164: initialState currently holds module-scoped Set/Map
instances (expandedFolders, selectedPaths, gitStatusMap, enhancedGitStatusMap)
which are reused by reset(), causing shared mutable state; change reset() to
recreate fresh collection instances instead of reusing initialState's references
(either implement a getInitialState() factory that returns a new object each
call or in reset() spread/construct new Set/Map for expandedFolders,
selectedPaths, gitStatusMap, enhancedGitStatusMap and assign them to the store);
update references to initialState in the file (e.g., the initialState constant
and the reset() implementation in use-file-editor-store.ts) so every reset gets
brand-new collections rather than shared ones.
Comment on lines
+15
to
+29
| async function copyDirectoryRecursive(src: string, dest: string): Promise<void> { | ||
| await mkdirSafe(dest); | ||
| const entries = await secureFs.readdir(src, { withFileTypes: true }); | ||
|
|
||
| for (const entry of entries) { | ||
| const srcPath = path.join(src, entry.name); | ||
| const destPath = path.join(dest, entry.name); | ||
|
|
||
| if (entry.isDirectory()) { | ||
| await copyDirectoryRecursive(srcPath, destPath); | ||
| } else { | ||
| await secureFs.copyFile(srcPath, destPath); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find apps/server -name '*secure*' -type f | head -20Repository: AutoMaker-Org/automaker
Length of output: 99
🏁 Script executed:
rg -l 'secureFs\|secure-fs' --type=ts -g '!node_modules/**' apps/server/src/ | head -10Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
cat -n apps/server/src/lib/secure-fs.tsRepository: AutoMaker-Org/automaker
Length of output: 1013
🏁 Script executed:
cat -n apps/server/src/routes/fs/routes/copy.tsRepository: AutoMaker-Org/automaker
Length of output: 3915
🏁 Script executed:
rg -n 'isSymbolicLink\|symlink' --type=ts -C3 -g '!node_modules/**' apps/server/src/Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
rg -n 'copyDirectoryRecursive\|copyFile' --type=ts -C2 -g '!node_modules/**' apps/server/src/ | head -30Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
rg -n 'readdir.*withFileTypes' --type=ts -C5 -g '!node_modules/**' apps/server/src/ | head -40Repository: AutoMaker-Org/automaker
Length of output: 3423
🏁 Script executed:
# Check if there are any tests related to copyDirectoryRecursive or symlinks
find apps/server -name '*.test.ts' -o -name '*.spec.ts' | xargs grep -l 'copyDirectory\|symlink' 2>/dev/null || echo "No tests found"Repository: AutoMaker-Org/automaker
Length of output: 110
🏁 Script executed:
# Check `@automaker/platform` package to understand Dirent API
rg -n 'Dirent\|readdir' --type=ts -g '@automaker/platform' 2>/dev/null | head -20 || echo "Cannot search external package"Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
cat -n apps/server/tests/unit/lib/fs-utils.test.tsRepository: AutoMaker-Org/automaker
Length of output: 7049
🏁 Script executed:
# Check if Dirent has isSymbolicLink method in Node.js typings or usage
rg -n 'isSymbolicLink' --type=ts -g '!node_modules/**' . | head -20Repository: AutoMaker-Org/automaker
Length of output: 345
🏁 Script executed:
# Check for circular/recursive patterns that might indicate symlink handling
rg -n 'entry\.is' --type=ts -C2 -g '!node_modules/**' apps/server/src/ | grep -A2 -B2 'isDirectory\|isFile\|isSymbol'Repository: AutoMaker-Org/automaker
Length of output: 8314
Handle symlinks in copyDirectoryRecursive to prevent infinite recursion.
The entry.isDirectory() check follows symlinks, so a symlink pointing to an ancestor directory could cause unbounded recursion despite the top-level path validation. Add a check for entry.isSymbolicLink() to skip symlinks or shallow-copy them instead of recursing into them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/routes/fs/routes/copy.ts` around lines 15 - 29, The recursive
copy function copyDirectoryRecursive currently follows symlinks because
entry.isDirectory() can be true for symlinked directories, risking infinite
recursion; update the loop to detect symlinks via entry.isSymbolicLink() and
handle them (either skip or shallow-copy the symlink) instead of descending:
call secureFs.lstat on srcPath or use entry.isSymbolicLink() to detect symlinks,
if symlink then either recreate the link at dest with secureFs.symlink (or skip)
and do not recurse, otherwise if it's a real directory use mkdirSafe(destPath)
and await copyDirectoryRecursive(srcPath, destPath), and for files continue to
use secureFs.copyFile; ensure you reference copyDirectoryRecursive,
entry.isDirectory(), entry.isSymbolicLink(), secureFs.copyFile, mkdirSafe when
making the changes.
Comment on lines
+59
to
+74
| try { | ||
| await secureFs.stat(destinationPath); | ||
| // Destination exists | ||
| if (!overwrite) { | ||
| res.status(409).json({ | ||
| success: false, | ||
| error: 'Destination already exists', | ||
| exists: true, | ||
| }); | ||
| return; | ||
| } | ||
| // If overwrite is true, remove the existing destination first to avoid merging | ||
| await secureFs.rm(destinationPath, { recursive: true }); | ||
| } catch { | ||
| // Destination doesn't exist - good to proceed | ||
| } |
Contributor
There was a problem hiding this comment.
Non-atomic overwrite: rm then copy leaves a window for data loss.
If overwrite is true, the existing destination is removed before the copy begins (Line 71). A process crash or failure between rm and the subsequent copy leaves the user with neither the old destination nor the new copy. This is a known trade-off for simplicity, but consider documenting it or copying to a temp path first, then swapping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/routes/fs/routes/copy.ts` around lines 59 - 74, The current
logic removes the destination (secureFs.rm(destinationPath)) before copying,
which is non-atomic and can cause data loss; change the flow to copy the source
into a uniquely named temporary sibling (e.g., destinationPath + ".tmp-<random>"
or use a tmp directory) and once the copy to that temp path completes
successfully, atomically rename/move the temp into destinationPath (using
secureFs.rename or equivalent) to replace it; ensure you generate a unique temp
name, handle errors by removing the temp on failure, and only call secureFs.rm
on the original destination if you must and after a successful swap (update the
code around secureFs.stat, secureFs.rm, and the copy/rename steps to implement
this swap pattern).
Comment on lines
+21
to
+36
| async function getDirectorySize(dirPath: string): Promise<number> { | ||
| let totalSize = 0; | ||
| const entries = await secureFs.readdir(dirPath, { withFileTypes: true }); | ||
|
|
||
| for (const entry of entries) { | ||
| const entryPath = path.join(dirPath, entry.name); | ||
| if (entry.isDirectory()) { | ||
| totalSize += await getDirectorySize(entryPath); | ||
| } else { | ||
| const stats = await secureFs.stat(entryPath); | ||
| totalSize += Number(stats.size); | ||
| } | ||
| } | ||
|
|
||
| return totalSize; | ||
| } |
Contributor
There was a problem hiding this comment.
Unbounded recursion in getDirectorySize on symlink cycles.
Like copyDirectoryRecursive in copy.ts, this function follows symlinks via entry.isDirectory(). A symlink loop would cause infinite recursion and stack overflow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/routes/fs/routes/download.ts` around lines 21 - 36,
getDirectorySize currently follows symlinks via entry.isDirectory() which can
cause unbounded recursion on symlink cycles; update getDirectorySize to avoid
following symlinks or detect cycles by using secureFs.lstat to check for
symbolic links and either skip them or resolve their realpath and track visited
targets (e.g., a Set of realpaths or device+inode from stat) before recursing,
and use secureFs.stat only after deciding to follow a link; ensure the logic
updates references in getDirectorySize (entry.isDirectory(), secureFs.stat,
secureFs.readdir) so recursive calls never re-enter an already visited path.
Comment on lines
+70
to
+75
| // Use system zip command (available on macOS and Linux) | ||
| // Use execFile to avoid shell injection via user-provided paths | ||
| await execFileAsync('zip', ['-r', tmpZipPath, fileName], { | ||
| cwd: path.dirname(filePath), | ||
| maxBuffer: 50 * 1024 * 1024, | ||
| }); |
Contributor
There was a problem hiding this comment.
zip command may not be available on all platforms.
The handler shells out to the system zip utility, which is not guaranteed on Windows or minimal Linux containers. Consider either checking for zip availability at startup or using a Node.js library like archiver to avoid the platform dependency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/routes/fs/routes/download.ts` around lines 70 - 75, The code
currently shells out to the system zip utility via execFileAsync('zip', ...)
using tmpZipPath, fileName and filePath which is not portable; replace this by
either checking for the presence of the zip binary at startup and failing fast
(or falling back) or, preferably, stop using execFileAsync here and implement
zipping in-process using a Node library such as archiver: create a zip archive
of fileName rooted at path.dirname(filePath) and write to tmpZipPath (or stream
directly to the response) instead of calling execFileAsync, and update the
handler that references tmpZipPath, fileName, filePath and execFileAsync
accordingly.
Comment on lines
+79
to
+81
| res.setHeader('Content-Type', 'application/zip'); | ||
| res.setHeader('Content-Disposition', `attachment; filename="${zipFileName}"`); | ||
| res.setHeader('Content-Length', zipStats.size.toString()); |
Contributor
There was a problem hiding this comment.
Content-Disposition header is vulnerable to header injection.
fileName and zipFileName are derived from user-supplied filePath and interpolated directly into the Content-Disposition header. A filename containing " or non-ASCII characters could break the header or enable response splitting.
Use RFC 5987 encoding or at minimum sanitize the filename:
🔒 Proposed fix
+ const safeFileName = fileName.replace(/[^\w.\-]/g, '_');
+
if (stats.isDirectory()) {
// ...
- res.setHeader('Content-Disposition', `attachment; filename="${zipFileName}"`);
+ const safeZipName = safeFileName + '.zip';
+ res.setHeader('Content-Disposition', `attachment; filename="${safeZipName}"`);
// ...
} else {
- res.setHeader('Content-Disposition', `attachment; filename="${fileName}"`);
+ res.setHeader('Content-Disposition', `attachment; filename="${safeFileName}"`);Also applies to: 118-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/routes/fs/routes/download.ts` around lines 79 - 81, The
Content-Disposition header is built from user-controlled fileName/zipFileName
and is vulnerable to header injection; update the code that sets the header (the
res.setHeader('Content-Disposition', ...) call using zipFileName and the
analogous call using fileName at lines ~118-120) to sanitize and RFC5987-encode
filenames: strip/replace CR/LF and quotes, restrict/control non-ASCII or
normalize them, then set both a safe ASCII filename (filename="...") and an
RFC5987-encoded filename* (filename*=UTF-8''...) per RFC5987; implement a small
helper (e.g., encodeAttachmentFilename or sanitizeFilename) and use it where
zipFileName/fileName are interpolated.
Comment on lines
+60
to
+105
| const BINARY_EXTENSIONS = new Set([ | ||
| 'png', | ||
| 'jpg', | ||
| 'jpeg', | ||
| 'gif', | ||
| 'bmp', | ||
| 'ico', | ||
| 'svg', | ||
| 'webp', | ||
| 'avif', | ||
| 'mp3', | ||
| 'mp4', | ||
| 'wav', | ||
| 'ogg', | ||
| 'webm', | ||
| 'avi', | ||
| 'mov', | ||
| 'flac', | ||
| 'zip', | ||
| 'tar', | ||
| 'gz', | ||
| 'bz2', | ||
| 'xz', | ||
| '7z', | ||
| 'rar', | ||
| 'pdf', | ||
| 'doc', | ||
| 'docx', | ||
| 'xls', | ||
| 'xlsx', | ||
| 'ppt', | ||
| 'pptx', | ||
| 'exe', | ||
| 'dll', | ||
| 'so', | ||
| 'dylib', | ||
| 'bin', | ||
| 'dat', | ||
| 'woff', | ||
| 'woff2', | ||
| 'ttf', | ||
| 'otf', | ||
| 'eot', | ||
| 'sqlite', | ||
| 'db', | ||
| ]); |
Contributor
There was a problem hiding this comment.
SVG files are classified as binary but are typically text-editable.
SVG is XML-based text. Including 'svg' in BINARY_EXTENSIONS prevents users from editing SVG files in the code editor. Consider removing it — the code editor already has XML syntax highlighting support.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around
lines 60 - 105, The BINARY_EXTENSIONS Set in file-editor-view.tsx incorrectly
treats 'svg' as binary which prevents text editing; remove the 'svg' entry from
the BINARY_EXTENSIONS constant so SVG files are handled as text (and will open
in the code editor with XML syntax highlighting) and verify any downstream
checks that rely on BINARY_EXTENSIONS still behave correctly for SVG files.
Comment on lines
+462
to
+480
| // ─── Handle Save ───────────────────────────────────────────── | ||
| const handleSave = useCallback(async () => { | ||
| if (!activeTab || !activeTab.isDirty) return; | ||
|
|
||
| try { | ||
| const api = getElectronAPI(); | ||
| const result = await api.writeFile(activeTab.filePath, activeTab.content); | ||
|
|
||
| if (result.success) { | ||
| markTabSaved(activeTab.id, activeTab.content); | ||
| // Refresh git status after save | ||
| loadGitStatus(); | ||
| } else { | ||
| logger.error('Failed to save file:', result.error); | ||
| } | ||
| } catch (error) { | ||
| logger.error('Failed to save file:', error); | ||
| } | ||
| }, [activeTab, markTabSaved, loadGitStatus]); |
Contributor
There was a problem hiding this comment.
Save failures are silently logged with no user feedback.
handleSave logs errors via logger.error but never shows a toast or other notification to the user. A failed save is a significant event — the user may believe their changes were persisted. The same applies to saveTabById (Lines 483–504) for auto-save failures.
🐛 Proposed fix
if (result.success) {
markTabSaved(activeTab.id, activeTab.content);
// Refresh git status after save
loadGitStatus();
} else {
logger.error('Failed to save file:', result.error);
+ toast.error('Save failed', { description: result.error });
}
} catch (error) {
logger.error('Failed to save file:', error);
+ toast.error('Save failed', {
+ description: error instanceof Error ? error.message : 'Unknown error',
+ });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around
lines 462 - 480, handleSave and saveTabById only log save failures; update both
to surface user-facing notifications: when a save succeeds call the existing UI
toast/notification function (e.g., showToast or notify) with a brief success
message and file name, and when a save fails catch the error/result.error and
call the toast with an error message including the error text so the user is
informed; place these calls alongside the existing logger.error branches in
handleSave (use getElectronAPI(), markTabSaved, loadGitStatus references) and in
saveTabById (the auto-save path) so both manual and auto-save failures/successes
notify the user.
Comment on lines
+662
to
+713
| const handleRenameItem = useCallback( | ||
| async (oldPath: string, newName: string) => { | ||
| const parentPath = oldPath.substring(0, oldPath.lastIndexOf('/')); | ||
| const newPath = `${parentPath}/${newName}`; | ||
|
|
||
| try { | ||
| const api = getElectronAPI(); | ||
|
|
||
| // Use the moveItem API for an atomic rename (works for both files and directories) | ||
| const result = await api.moveItem?.(oldPath, newPath); | ||
|
|
||
| if (result?.success) { | ||
| // Update the open tab if it was renamed | ||
| const tab = tabs.find((t) => t.filePath === oldPath); | ||
| if (tab) { | ||
| closeTab(tab.id); | ||
| if (isMobile) { | ||
| handleMobileFileSelect(newPath); | ||
| } else { | ||
| handleFileSelect(newPath); | ||
| } | ||
| } | ||
|
|
||
| // If the new name starts with a dot, auto-enable hidden files visibility | ||
| // so the renamed file doesn't "disappear" from the tree | ||
| if (newName.startsWith('.')) { | ||
| const { showHiddenFiles } = useFileEditorStore.getState(); | ||
| if (!showHiddenFiles) { | ||
| store.setShowHiddenFiles(true); | ||
| } | ||
| } | ||
|
|
||
| await loadTree(undefined, { preserveExpanded: true }); | ||
| loadGitStatus(); | ||
| } else { | ||
| toast.error('Rename failed', { description: result?.error }); | ||
| } | ||
| } catch (error) { | ||
| logger.error('Failed to rename item:', error); | ||
| } | ||
| }, | ||
| [ | ||
| tabs, | ||
| closeTab, | ||
| handleFileSelect, | ||
| handleMobileFileSelect, | ||
| isMobile, | ||
| loadTree, | ||
| loadGitStatus, | ||
| store, | ||
| ] | ||
| ); |
Contributor
There was a problem hiding this comment.
Previous data-loss issue resolved — rename now uses atomic moveItem.
The rename handler now delegates to api.moveItem instead of the previous mkdir + deleteFile pattern, which is correct for both files and directories.
However, when moveItem is not available (Line 671 returns undefined), the user sees toast.error('Rename failed', { description: undefined }) — a confusing empty error. Add an explicit guard:
🐛 Proposed fix
try {
const api = getElectronAPI();
+ if (!api.moveItem) {
+ toast.error('Rename not supported');
+ return;
+ }
+
// Use the moveItem API for an atomic rename (works for both files and directories)
- const result = await api.moveItem?.(oldPath, newPath);
+ const result = await api.moveItem(oldPath, newPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around
lines 662 - 713, handleRenameItem currently calls api.moveItem?.(...) and if
moveItem is undefined the result is undefined, causing toast.error to show an
empty description; add an explicit guard that checks whether api.moveItem exists
(from getElectronAPI()) before calling it and, if missing, immediately
toast.error with a clear message (e.g., "Rename API unavailable") and return;
otherwise call api.moveItem, handle result.success as now, and include
result.error when present in toast.error to avoid confusing undefined
descriptions.
Comment on lines
+835
to
+849
| const handleDragDropMove = useCallback( | ||
| async (sourcePaths: string[], targetFolderPath: string) => { | ||
| for (const sourcePath of sourcePaths) { | ||
| const fileName = sourcePath.split('/').pop() || ''; | ||
| const destinationPath = `${targetFolderPath}/${fileName}`; | ||
|
|
||
| // Prevent moving to the same location | ||
| const sourceDir = sourcePath.substring(0, sourcePath.lastIndexOf('/')); | ||
| if (sourceDir === targetFolderPath) continue; | ||
|
|
||
| await handleMoveItem(sourcePath, destinationPath); | ||
| } | ||
| }, | ||
| [handleMoveItem] | ||
| ); |
Contributor
There was a problem hiding this comment.
Sequential drag-drop moves stop on first failure with no rollback.
handleDragDropMove iterates sourcePaths with await handleMoveItem(...) in a loop. If one move fails (exception or user cancels the overwrite confirm), remaining items are skipped. Already-moved items are not rolled back. This can leave the filesystem in a partial state that's confusing to the user. At minimum, continue processing remaining items and aggregate errors:
♻️ Proposed fix
const handleDragDropMove = useCallback(
async (sourcePaths: string[], targetFolderPath: string) => {
+ const errors: string[] = [];
for (const sourcePath of sourcePaths) {
const fileName = sourcePath.split('/').pop() || '';
const destinationPath = `${targetFolderPath}/${fileName}`;
const sourceDir = sourcePath.substring(0, sourcePath.lastIndexOf('/'));
if (sourceDir === targetFolderPath) continue;
- await handleMoveItem(sourcePath, destinationPath);
+ try {
+ await handleMoveItem(sourcePath, destinationPath);
+ } catch (error) {
+ errors.push(fileName);
+ }
+ }
+ if (errors.length > 0) {
+ toast.error(`Failed to move: ${errors.join(', ')}`);
}
},
[handleMoveItem]
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around
lines 835 - 849, handleDragDropMove currently awaits each handleMoveItem and
will abort on the first thrown error, leaving previously moved items in place;
change it so the loop catches per-item errors, continues processing remaining
sourcePaths, and aggregates failures to present a single consolidated
error/summary at the end (use sourcePaths, targetFolderPath, handleMoveItem and
fileName/destinationPath variables to locate the logic). Additionally, implement
a compensating rollback strategy: if you want atomic-like behavior, on any
failure attempt to move already-successful items back to their original paths by
calling handleMoveItem with swapped source/destination (record moved pairs as
you go), and collect any rollback failures to include in the final aggregated
report; ensure the function still returns/resolves after presenting the
aggregated result to the caller.
Comment on lines
+305
to
+311
| partialize: (state) => | ||
| ({ | ||
| tabs: state.tabs, | ||
| activeTabId: state.activeTabId, | ||
| expandedFolders: state.expandedFolders, | ||
| markdownViewMode: state.markdownViewMode, | ||
| }) as unknown as FileEditorState, |
Contributor
There was a problem hiding this comment.
Persisting full tabs (including file content) risks exceeding the localStorage quota.
Each EditorTab stores content and originalContent. A few open files with a few hundred KB of content each could hit the ~5MB localStorage limit, causing silent write failures (caught on Line 346). Consider either excluding content from persistence (reload it on restore) or capping the persisted size.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/file-editor-view/use-file-editor-store.ts`
around lines 305 - 311, The current partialize persists full tabs (EditorTab)
including content and originalContent which can exceed localStorage quota;
update partialize to persist only tab metadata (e.g., id, path, name,
cursor/selection, unsaved flag) by mapping state.tabs to omit
content/originalContent before casting to FileEditorState, and implement/ensure
a restore path (e.g., in the tab restore/open logic or a loadFileContent
function) that reloads file content on-demand when a tab is reactivated;
optionally add a size cap or skip persisting tabs if total serialized size
exceeds a threshold to avoid localStorage write failures and keep existing error
handling for storage writes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/ui/src/components/views/file-editor-view/components/editor-settings-form.tsx (1)
53-54: Extract the hardcoded default font-size13into a shared constant.The value
13appears in both theonClickhandler and thedisabledcondition. If the store's default changes, this will silently diverge.♻️ Proposed refactor
+const DEFAULT_FONT_SIZE = 13; ... onClick={() => setEditorFontSize(DEFAULT_FONT_SIZE)} disabled={editorFontSize === DEFAULT_FONT_SIZE}Ideally this constant is co-located with the store's default value so they stay in sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/components/editor-settings-form.tsx` around lines 53 - 54, The component hardcodes the editor default font-size (13) in the onClick and disabled checks which can drift from the store default; extract that literal into a shared constant (e.g., EDITOR_DEFAULT_FONT_SIZE) colocated with the store's default and import it into editor-settings-form.tsx, then replace both uses of 13 with the constant in the setEditorFontSize call and the editorFontSize === comparison (refer to setEditorFontSize and editorFontSize to locate the spots).apps/ui/src/components/views/file-editor-view/file-editor-view.tsx (1)
191-213: Parallelize subdirectory reads inbuildTreewithPromise.all.The loop awaits each
buildTreecall sequentially, making initial tree load time proportional to the number of top-level directories.Promise.allwould dispatch all reads concurrently.♻️ Proposed refactor
- for (const node of nodes) { - if (node.isDirectory) { - node.children = await buildTree(node.path, depth + 1); - } - } + await Promise.all( + nodes.filter((n) => n.isDirectory).map(async (node) => { + node.children = await buildTree(node.path, depth + 1); + }) + );The same pattern applies to the pre-loading loop in
loadSubdirectory(Lines 314–334).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around lines 191 - 213, The sequential awaits in buildTree are slowing initial loads; replace the for-await loop that calls buildTree for each directory node with a concurrent approach using Promise.all so all child builds for nodes (from the nodes array) are dispatched at once and then assigned back to node.children; do the same refactor in loadSubdirectory where the pre-loading loop (that calls buildTree or reads subdirs) currently awaits each iteration sequentially—collect promises for each directory, run Promise.all, and map results back to the corresponding node.children to preserve order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/ui/src/components/views/file-editor-view/components/editor-settings-form.tsx`:
- Line 37: The Labels in editor-settings-form.tsx (e.g., the "Font Size" Label)
are not associated with their form controls; add an id to each control (the
Slider instance for font size, the Select instance for theme, and the Switch
instance for word wrap) and set the corresponding Label's htmlFor to that id so
screen readers can link label->control; update the three Label elements (lines
near the Font Size Label, the theme Select Label, and the word-wrap Switch
Label) to use htmlFor and add matching id props on the Slider, Select, and
Switch components respectively.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx`:
- Around line 909-936: The code captures fileTree (and expandedFolders) once via
useFileEditorStore.getState() before awaiting loadSubdirectory, then uses that
stale snapshot to compute updateChildren, which can overwrite concurrent
mutations; after awaiting loadSubdirectory(path) re-read the latest state (e.g.,
const { fileTree: latestTree } = useFileEditorStore.getState()) and run
updateChildren against latestTree (and optionally re-check expandedFolders if
needed) before calling setFileTree so you don't clobber intervening updates;
keep the existing helpers (findNode, updateChildren) and only change the source
of truth used after the await.
- Around line 564-649: The catch blocks in handleCreateFile, handleCreateFolder,
and handleDeleteItem only log errors with logger.error and do not surface
failures to the user; update each catch to both log and show a user-facing
notification (displaying a concise message plus error details) so users get
immediate feedback. In each of handleCreateFile, handleCreateFolder, and
handleDeleteItem, keep the existing logger.error call but also call the app's
notification API (for example, the store's toast/error setter or a
showToast/showError helper) with a clear message like "Failed to create
file/folder" or "Failed to delete item" and include error.message; ensure the
notification is invoked for all three handlers and preserves the existing
preserveExpanded/loadTree behavior.
---
Nitpick comments:
In
`@apps/ui/src/components/views/file-editor-view/components/editor-settings-form.tsx`:
- Around line 53-54: The component hardcodes the editor default font-size (13)
in the onClick and disabled checks which can drift from the store default;
extract that literal into a shared constant (e.g., EDITOR_DEFAULT_FONT_SIZE)
colocated with the store's default and import it into editor-settings-form.tsx,
then replace both uses of 13 with the constant in the setEditorFontSize call and
the editorFontSize === comparison (refer to setEditorFontSize and editorFontSize
to locate the spots).
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx`:
- Around line 191-213: The sequential awaits in buildTree are slowing initial
loads; replace the for-await loop that calls buildTree for each directory node
with a concurrent approach using Promise.all so all child builds for nodes (from
the nodes array) are dispatched at once and then assigned back to node.children;
do the same refactor in loadSubdirectory where the pre-loading loop (that calls
buildTree or reads subdirs) currently awaits each iteration sequentially—collect
promises for each directory, run Promise.all, and map results back to the
corresponding node.children to preserve order.
| {/* Font Size */} | ||
| <div className="space-y-2"> | ||
| <div className="flex items-center justify-between"> | ||
| <Label className="text-xs font-medium">Font Size</Label> |
Contributor
There was a problem hiding this comment.
Associate <Label> with each form control via htmlFor/id.
All three Labels are visually adjacent but not programmatically linked to their controls (Slider, Select, Switch). Screen readers depend on this association to announce the label when the control receives focus.
♿ Proposed fix
- <Label className="text-xs font-medium">Font Size</Label>
+ <Label htmlFor="editor-font-size" className="text-xs font-medium">Font Size</Label>
...
<Slider
+ id="editor-font-size"
value={[editorFontSize]}- <Label className="text-xs font-medium">Font Family</Label>
+ <Label htmlFor="editor-font-family" className="text-xs font-medium">Font Family</Label>
<Select
+ name="editor-font-family"
value={editorFontFamily || DEFAULT_FONT_VALUE}- <Label className="text-xs font-medium">Auto Save</Label>
- <Switch checked={editorAutoSave} onCheckedChange={setEditorAutoSave} />
+ <Label htmlFor="editor-auto-save" className="text-xs font-medium">Auto Save</Label>
+ <Switch id="editor-auto-save" checked={editorAutoSave} onCheckedChange={setEditorAutoSave} />Also applies to: 64-64, 90-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/ui/src/components/views/file-editor-view/components/editor-settings-form.tsx`
at line 37, The Labels in editor-settings-form.tsx (e.g., the "Font Size" Label)
are not associated with their form controls; add an id to each control (the
Slider instance for font size, the Select instance for theme, and the Switch
instance for word wrap) and set the corresponding Label's htmlFor to that id so
screen readers can link label->control; update the three Label elements (lines
near the Font Size Label, the theme Select Label, and the word-wrap Switch
Label) to use htmlFor and add matching id props on the Slider, Select, and
Switch components respectively.
Comment on lines
+909
to
+936
| const { expandedFolders, fileTree } = useFileEditorStore.getState(); | ||
|
|
||
| if (!expandedFolders.has(path)) { | ||
| // Loading children for newly expanded folder | ||
| const findNode = (nodes: FileTreeNode[]): FileTreeNode | null => { | ||
| for (const n of nodes) { | ||
| if (n.path === path) return n; | ||
| if (n.children) { | ||
| const found = findNode(n.children); | ||
| if (found) return found; | ||
| } | ||
| } | ||
| return null; | ||
| }; | ||
|
|
||
| const node = findNode(fileTree); | ||
| if (node && !node.children) { | ||
| const children = await loadSubdirectory(path); | ||
| // Update the tree with loaded children | ||
| const updateChildren = (nodes: FileTreeNode[]): FileTreeNode[] => { | ||
| return nodes.map((n) => { | ||
| if (n.path === path) return { ...n, children }; | ||
| if (n.children) return { ...n, children: updateChildren(n.children) }; | ||
| return n; | ||
| }); | ||
| }; | ||
| setFileTree(updateChildren(fileTree)); | ||
| } |
Contributor
There was a problem hiding this comment.
Re-read fileTree from store state after await loadSubdirectory to avoid a stale-snapshot race.
fileTree is snapshotted from getState() at line 909, but then used again at line 934 to build the updated tree — after an await. If any other operation (e.g., file create, tree refresh) mutates the tree during that await, the stale snapshot overwrites those changes. The codebase already uses the correct pattern in handleCursorChange and handleEditorChange (lines 881, 891): re-read from getState() instead of relying on captured variables.
🐛 Proposed fix
const children = await loadSubdirectory(path);
+ // Re-read current tree after async gap to avoid overwriting concurrent updates
+ const { fileTree: currentFileTree } = useFileEditorStore.getState();
const updateChildren = (nodes: FileTreeNode[]): FileTreeNode[] => {
return nodes.map((n) => {
if (n.path === path) return { ...n, children };
if (n.children) return { ...n, children: updateChildren(n.children) };
return n;
});
};
- setFileTree(updateChildren(fileTree));
+ setFileTree(updateChildren(currentFileTree));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/file-editor-view/file-editor-view.tsx` around
lines 909 - 936, The code captures fileTree (and expandedFolders) once via
useFileEditorStore.getState() before awaiting loadSubdirectory, then uses that
stale snapshot to compute updateChildren, which can overwrite concurrent
mutations; after awaiting loadSubdirectory(path) re-read the latest state (e.g.,
const { fileTree: latestTree } = useFileEditorStore.getState()) and run
updateChildren against latestTree (and optionally re-check expandedFolders if
needed) before calling setFileTree so you don't clobber intervening updates;
keep the existing helpers (findNode, updateChildren) and only change the source
of truth used after the await.
gsxdsm
added a commit
to gsxdsm/automaker
that referenced
this pull request
Feb 21, 2026
* feat: Add file management feature * feat: Add auto-save functionality to file editor * fix: Replace HardDriveDownload icon with Save icon for consistency * fix: Prevent recursive copy/move and improve shell injection prevention * refactor: Extract editor settings form into separate component
10 tasks
gsxdsm
added a commit
that referenced
this pull request
Feb 21, 2026
…dling (#790) * feat: Add PR review comments and resolution endpoints, improve prompt handling * Feature: File Editor (#789) * feat: Add file management feature * feat: Add auto-save functionality to file editor * fix: Replace HardDriveDownload icon with Save icon for consistency * fix: Prevent recursive copy/move and improve shell injection prevention * refactor: Extract editor settings form into separate component * ``` fix: Improve error handling and stabilize async operations - Add error event handlers to GraphQL process spawns to prevent unhandled rejections - Replace execAsync with execFile for safer command execution and better control - Fix timeout cleanup in withTimeout generator to prevent memory leaks - Improve outdated comment detection logic by removing redundant condition - Use resolveModelString for consistent model string handling - Replace || with ?? for proper falsy value handling in dialog initialization - Add comments clarifying branch name resolution logic for local branches with slashes - Add catch handler for project selection to handle async errors gracefully ``` * refactor: Extract PR review comments logic to dedicated service * fix: Improve robustness and UX for PR review and file operations * fix: Consolidate exec utilities and improve type safety * refactor: Replace ScrollArea with div and improve file tree layout
This was referenced Feb 21, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Introduces a full-featured in-app File Editor view, providing developers with a complete code editing experience directly within Automaker — no need to switch to an external editor while managing features and worktrees.
✨ New Features
Core File Editor (
FilesView)CodeMirror-Based Code Editor (
code-editor.tsx)@uiw/react-codemirror) with a custom dark theme using CSS variables for seamless design system integration@replit/codemirror-vimand@replit/codemirror-emacsFile Tree (
file-tree.tsx)node_modules,.git,dist,.next, and other noise directoriesFile Search Dialog (
file-search-dialog.tsx)Markdown Preview
.md/.mdxfiles: Editor, Preview, SplitMarkdowncomponentEditor Settings Section
Server-Side
/fsRoutesPOST /search-filesPOST /search-contentPOST /git-diffPOST /git-stageGET /git-statusPOST /renameAll routes enforce path security (confined to project/worktree root).
🧪 Tests
apps/ui/tests/files/git-integration.spec.ts— Playwright E2E tests covering file open, edit, save, git diff annotations, and file searchTest Plan
.mdfile and cycle through Editor → Preview → Split modesSummary by CodeRabbit
New Features
Chores