Editor stat managment#25
Merged
Merged
Conversation
…ed project state management, including tab handling and folder expansion features. Refactor FileTree and ProjectLayout components for better integration with the new editor state structure.
…ng and state management. Ensure editor content updates correctly on project changes and optimize performance by reducing unnecessary state updates. Enhance dialog handling for unsaved changes and synchronize editor model with loaded file content.
…er saving, preventing cursor disruption. Update file change handling to include event detection and improve state management for unsaved changes.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds per-project tabbed editor state with unsaved-change confirmation and keyboard navigation; converts Monaco editor to uncontrolled with external resync; makes FileTree folder expansion controlled; exposes new log/tunnel IPCs and preload APIs with export-to-.log/.zip flows; adds atoms and editorState helpers; bumps package version. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant EditorView
participant Store as "Project Editor State (Atoms)"
participant MonacoEditor
participant ProjectLayout
participant Preload as "Electron Preload"
participant IPC as "Electron IPC"
participant FS as "File System / Services"
User->>EditorView: open file / switch project
EditorView->>Store: read project editor state (openTabs, activeTabId, lastActiveFile)
EditorView->>EditorView: ensure tab exists / set activeTabId
EditorView->>MonacoEditor: load file contents (defaultValue)
MonacoEditor->>EditorView: onChange -> mark unsaved in Store
User->>EditorView: Close tab (Ctrl/Cmd+W)
EditorView->>Store: check unsaved status
alt Unsaved
EditorView->>EditorView: show confirmation dialog
User->>EditorView: choose Save
EditorView->>ProjectLayout: trigger save -> persist file
ProjectLayout->>FS: write file
ProjectLayout->>Store: clear unsaved, update project state
else Discard or already saved
EditorView->>Store: remove tab, update activeTabId
end
User->>EditorView: Export logs
EditorView->>Preload: invoke exportConsoleLogsProject/exportConsoleLogsAll
Preload->>IPC: invoke channel
IPC->>FS: assemble logs, write .log or .zip
IPC-->>Preload: return file path / success
Preload-->>EditorView: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
… TunnelLogViewer components to ensure consistent locale usage. Refactor TunnelView component to handle port input as a string and normalize digit input for better user experience.
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/EditorView.jsx`:
- Around line 128-140: The component initializes file/tab/folder state with
useState(initialProjectState...) which only runs once, so when projectId changes
this stale state leaks into the new project; add a useEffect that runs when
projectId or initialProjectState changes and resets the local state (call
setCurrentFile, setOpenTabs, setActiveTabId, setExplorerExpanded,
setUnsavedChanges, setGitStatusByPath, etc.) to initialProjectState values (or
sensible defaults) before any persistence logic runs, and ensure the persistence
code that writes project state (the effect around saving/restoring) reads the
updated state for the new project rather than reusing the old values.
- Around line 501-535: The save-and-close path in handleConfirmCloseSave races
with the file watcher because it doesn't set suppressReloadForPathRef like
handleSaveFile does; before calling API.writeFile(tab.path, ...) set
suppressReloadForPathRef.current = tab.path (or increment/mark) and ensure you
clear/unset it after the write completes or in the finally block (both on
success and error) so the watcher suppression mirrors handleSaveFile; keep
performCloseTab and toast behavior unchanged but ensure suppression is set
before API.writeFile and removed after to prevent the reopened tab race.
In `@src/editors/MonacoEditor.jsx`:
- Around line 20-28: The effect in MonacoEditor.jsx uses
editor.setValue(nextValue) which triggers onDidChangeModelContent and causes
EditorView.jsx's handleEditorChange to call setUnsavedChanges, marking clean
files dirty; fix by introducing a syncingExternalValueRef (or similar boolean
ref) in MonacoEditor.jsx that is set true immediately before calling
editor.setValue(nextValue) and set false right after, and then modify
handleEditorChange in EditorView.jsx to ignore change events when that ref is
true (i.e., only call setUnsavedChanges for user-originated edits); reference
editorRef, useEffect, setValue in MonacoEditor.jsx and handleEditorChange and
setUnsavedChanges in EditorView.jsx when implementing the suppression flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5972931e-a479-4e42-9ad1-f036e145bacd
📒 Files selected for processing (7)
package.jsonsrc/components/EditorView.jsxsrc/components/FileTree.jsxsrc/components/ProjectLayout.jsxsrc/editors/MonacoEditor.jsxsrc/store/atoms.jssrc/store/editorState.js
Comment on lines
+128
to
+140
| const [currentFile, setCurrentFile] = useState(initialProjectState?.lastActiveFile || null); | ||
| const [isFileLoading, setIsFileLoading] = useState(false); | ||
| const [fileLoadError, setFileLoadError] = useState(null); | ||
| const [unsavedChanges, setUnsavedChanges] = useAtom(atoms.unsavedChangesAtom); | ||
| const [searchOpen, setSearchOpen] = useState(false); | ||
| const [gitOpen, setGitOpen] = useState(false); | ||
| const [scrollToLine, setScrollToLine] = useState(null); | ||
| const [gitStatusByPath, setGitStatusByPath] = useState({}); | ||
| const [openTabs, setOpenTabs] = useState(initialProjectState?.openTabs || []); | ||
| const [activeTabId, setActiveTabId] = useState(initialProjectState?.activeTabId || null); | ||
| const [explorerExpanded, setExplorerExpanded] = useState( | ||
| initialProjectState?.explorerExpanded || {} | ||
| ); |
Contributor
There was a problem hiding this comment.
Project switches will leak tabs and file state into the next project.
The useState(initialProjectState...) initializers at Lines 128-140 only run on the first mount. If projectId changes while this component stays mounted, those locals still contain the previous project's tabs / expanded folders / currentFile, and Lines 163-175 immediately persist that stale state into the new project's slot. The restore effect later in the file can then reopen a file from the wrong project.
Also applies to: 163-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/EditorView.jsx` around lines 128 - 140, The component
initializes file/tab/folder state with useState(initialProjectState...) which
only runs once, so when projectId changes this stale state leaks into the new
project; add a useEffect that runs when projectId or initialProjectState changes
and resets the local state (call setCurrentFile, setOpenTabs, setActiveTabId,
setExplorerExpanded, setUnsavedChanges, setGitStatusByPath, etc.) to
initialProjectState values (or sensible defaults) before any persistence logic
runs, and ensure the persistence code that writes project state (the effect
around saving/restoring) reads the updated state for the new project rather than
reusing the old values.
Comment on lines
+501
to
+535
| const handleConfirmCloseSave = useCallback(async () => { | ||
| if (pendingCloseTabId == null) return; | ||
| const tab = openTabs.find((t) => t.id === pendingCloseTabId); | ||
| if (!tab) { | ||
| setPendingCloseTabId(null); | ||
| return; | ||
| } | ||
| setIsSavingCloseTab(true); | ||
| const content = | ||
| currentFileRef.current === tab.path | ||
| ? editorContentRef.current | ||
| : unsavedChangesRef.current[tab.path]; | ||
| const contentToWrite = content ?? ""; | ||
| try { | ||
| const success = await API.writeFile(tab.path, contentToWrite); | ||
| if (success) { | ||
| toast.success("File saved"); | ||
| performCloseTab(pendingCloseTabId); | ||
| } else { | ||
| toast.error("Failed to save file"); | ||
| setPendingCloseTabId(null); | ||
| } | ||
| } catch (err) { | ||
| const isDeleted = | ||
| (err?.message && err.message.includes("no longer exists")) || err?.code === "ENOENT"; | ||
| if (isDeleted) { | ||
| toast.info("File was deleted or moved; cannot save."); | ||
| } else { | ||
| toast.error(`Error saving file: ${err?.message || "Unknown error"}`); | ||
| } | ||
| setPendingCloseTabId(null); | ||
| } finally { | ||
| setIsSavingCloseTab(false); | ||
| } | ||
| }, [pendingCloseTabId, openTabs, performCloseTab]); |
Contributor
There was a problem hiding this comment.
Reapply watcher suppression in the save-and-close path.
handleSaveFile stamps suppressReloadForPathRef before API.writeFile, but this flow does not. Because electron/preload.js delivers file-change events asynchronously, saving the active tab here can race with the watcher and reopen the tab you just closed.
Possible fix
setIsSavingCloseTab(true);
+ suppressReloadForPathRef.current[tab.path] = Date.now();
const content =
currentFileRef.current === tab.path
? editorContentRef.current
: unsavedChangesRef.current[tab.path];
const contentToWrite = content ?? "";
try {
const success = await API.writeFile(tab.path, contentToWrite);
if (success) {
toast.success("File saved");
+ delete unsavedChangesRef.current?.[tab.path];
performCloseTab(pendingCloseTabId);
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/EditorView.jsx` around lines 501 - 535, The save-and-close
path in handleConfirmCloseSave races with the file watcher because it doesn't
set suppressReloadForPathRef like handleSaveFile does; before calling
API.writeFile(tab.path, ...) set suppressReloadForPathRef.current = tab.path (or
increment/mark) and ensure you clear/unset it after the write completes or in
the finally block (both on success and error) so the watcher suppression mirrors
handleSaveFile; keep performCloseTab and toast behavior unchanged but ensure
suppression is set before API.writeFile and removed after to prevent the
reopened tab race.
Comment on lines
+20
to
+28
| useEffect(() => { | ||
| const editor = editorRef.current; | ||
| if (!editor) return; | ||
| const nextValue = value ?? ""; | ||
| const currentValue = editor.getValue(); | ||
| if (currentValue !== nextValue) { | ||
| editor.setValue(nextValue); | ||
| } | ||
| }, [value]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Monaco Editor's editor.setValue()triggeronDidChangeModelContent, and does @monaco-editor/reactforward that through itsonChange prop?
💡 Result:
Yes, Monaco Editor's editor.setValue triggers onDidChangeModelContent. Yes, @monaco-editor/react forwards that through its onChange prop.
Citations:
- 1: `setValue` without triggering change events microsoft/monaco-editor#3628
- 2: https://microsoft.github.io/monaco-editor/typedoc/interfaces/editor.ICodeEditor.html
- 3: https://microsoft.github.io/monaco-editor/typedoc/interfaces/editor.ITextModel.html
- 4: https://github.com/suren-atoyan/monaco-react
- 5: Detecting source of change on Controlled Editor | Preventing onChange Event on programmatic value change suren-atoyan/monaco-react#91
🏁 Script executed:
# First, check EditorView.jsx lines 594-607 to see how onChange is handled
fd EditorView.jsx --type f --exec wc -l {} \;Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 111
🏁 Script executed:
# Get the content around lines 594-607 in EditorView.jsx
fd EditorView.jsx --type f -x cat -n {} \; | sed -n '580,620p'Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 1790
🏁 Script executed:
# Also check MonacoEditor.jsx for the handleEditorChange implementation
fd MonacoEditor.jsx --type f -x cat -n {} \;Repository: DevRoots-Studio/SelfHost-Helper
Length of output: 4663
Programmatic resyncs will mark clean tabs as dirty.
When the value prop changes (e.g., during file load/reload), the useEffect calls editor.setValue(nextValue). Monaco Editor's setValue() triggers onDidChangeModelContent, which @monaco-editor/react forwards through its onChange prop. This invokes EditorView.jsx handleEditorChange (lines 594–609), which calls setUnsavedChanges without distinguishing between user edits and programmatic updates. Consequently, switching or reloading a tab marks it dirty even though the file has not been user-modified, causing false dirty indicators and unwanted close prompts.
The suggested fix using a syncingExternalValueRef flag to suppress the onChange callback during programmatic updates is the correct approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/editors/MonacoEditor.jsx` around lines 20 - 28, The effect in
MonacoEditor.jsx uses editor.setValue(nextValue) which triggers
onDidChangeModelContent and causes EditorView.jsx's handleEditorChange to call
setUnsavedChanges, marking clean files dirty; fix by introducing a
syncingExternalValueRef (or similar boolean ref) in MonacoEditor.jsx that is set
true immediately before calling editor.setValue(nextValue) and set false right
after, and then modify handleEditorChange in EditorView.jsx to ignore change
events when that ref is true (i.e., only call setUnsavedChanges for
user-originated edits); reference editorRef, useEffect, setValue in
MonacoEditor.jsx and handleEditorChange and setUnsavedChanges in EditorView.jsx
when implementing the suppression flag.
…ct and tunnel logs, and enhance log management with new API methods for retrieving all logs. Introduce context menus in LogViewer and TunnelView components for improved user interaction.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/LogViewer.jsx (1)
84-85:⚠️ Potential issue | 🟡 MinorDuplicate
term.open()call will cause terminal rendering issues.The terminal is opened twice on the same container element, which can cause unexpected behavior with xterm.js initialization.
🐛 Proposed fix
term.open(terminalContainerRef.current); - term.open(terminalContainerRef.current); requestAnimationFrame(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/LogViewer.jsx` around lines 84 - 85, The duplicate term.open(terminalContainerRef.current) call in LogViewer.jsx is causing xterm.js to initialize the same container twice; remove the redundant call so term.open is invoked only once (or add a guard around term.open in the component initialization logic) and ensure the terminal is opened against terminalContainerRef.current only a single time to avoid rendering issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/ipc/handlers.js`:
- Around line 451-455: The current formatLine function reduces entry.timestamp
to a local time-of-day string, losing date and timezone; update formatLine (and
the other handler's equivalent) to preserve the full ISO timestamp from
entry.timestamp (e.g., use entry.timestamp or new
Date(entry.timestamp).toISOString()) in the exported line while still optionally
including the local time if desired, ensuring you reference entry.timestamp and
the formatLine function so the output contains the full unambiguous timestamp
rather than just the time-of-day.
- Around line 370-387: The ZIP creation currently builds all project log strings
and calls AdmZip/zip.toBuffer() on the main thread (see AdmZip, zip.addFile,
zip.toBuffer, getProjectLogs, sanitizeFileName, fs.writeFile) which can
OOM/freeze; instead either move the archive work into a Node worker thread that
runs the same loop and writes the file, or switch to a streaming archive API
(e.g., archiver or yazl) that pipes entries directly to an fs.createWriteStream
(iterate projects, call getProjectLogs(numericId) per entry, sanitizeFileName
for entry names, and append each entry as a stream) so you never materialize the
full archive buffer in memory before writing to disk. Ensure the handler returns
progress/success once the worker/stream finishes and remove the
zip.toBuffer()/await fs.writeFile pattern from the main thread.
---
Outside diff comments:
In `@src/components/LogViewer.jsx`:
- Around line 84-85: The duplicate term.open(terminalContainerRef.current) call
in LogViewer.jsx is causing xterm.js to initialize the same container twice;
remove the redundant call so term.open is invoked only once (or add a guard
around term.open in the component initialization logic) and ensure the terminal
is opened against terminalContainerRef.current only a single time to avoid
rendering issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c085c944-f011-44f8-8a11-35054498345d
📒 Files selected for processing (10)
electron/ipc/handlers.jselectron/preload.jselectron/services/projectsManager.jselectron/services/tunnelManager.jspackage.jsonsrc/components/LogViewer.jsxsrc/components/Sidebar.jsxsrc/components/TunnelLogViewer.jsxsrc/components/TunnelView.jsxsrc/lib/numberUtils.js
Comment on lines
+370
to
+387
| const zip = new AdmZip(); | ||
|
|
||
| for (const project of projects) { | ||
| const numericId = Number(project?.id); | ||
| if (!Number.isFinite(numericId)) continue; | ||
|
|
||
| const projectName = project?.name || `Project ${numericId}`; | ||
| const logs = getProjectLogs(numericId); | ||
| const header = `===== Console Logs: ${projectName} (ID: ${numericId}) =====\n`; | ||
| const body = Array.isArray(logs) ? logs.map((l) => l?.data ?? "").join("") : ""; | ||
| const text = `${header}${body}`; | ||
|
|
||
| const fileName = `${sanitizeFileName(projectName)}-${numericId}.log`; | ||
| zip.addFile(fileName, Buffer.from(text, "utf8")); | ||
| } | ||
|
|
||
| const out = zip.toBuffer(); | ||
| await fs.writeFile(filePath, out); |
Contributor
There was a problem hiding this comment.
These ZIP exports can freeze the Electron main process on large histories.
Each handler builds every project's log text in memory, keeps those buffers in the archive, then materializes the full ZIP buffer before fs.writeFile. On noisy projects that can stall the whole app or OOM during export. Please move archive creation off the main thread or switch to a streaming/temp-file approach.
Also applies to: 492-509
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.js` around lines 370 - 387, The ZIP creation currently
builds all project log strings and calls AdmZip/zip.toBuffer() on the main
thread (see AdmZip, zip.addFile, zip.toBuffer, getProjectLogs, sanitizeFileName,
fs.writeFile) which can OOM/freeze; instead either move the archive work into a
Node worker thread that runs the same loop and writes the file, or switch to a
streaming archive API (e.g., archiver or yazl) that pipes entries directly to an
fs.createWriteStream (iterate projects, call getProjectLogs(numericId) per
entry, sanitizeFileName for entry names, and append each entry as a stream) so
you never materialize the full archive buffer in memory before writing to disk.
Ensure the handler returns progress/success once the worker/stream finishes and
remove the zip.toBuffer()/await fs.writeFile pattern from the main thread.
Comment on lines
+451
to
+455
| const formatLine = (entry) => { | ||
| const timestamp = entry?.timestamp ? new Date(entry.timestamp) : null; | ||
| const time = timestamp ? timestamp.toLocaleTimeString("en-US", { hour12: false }) : "unknown"; | ||
| const message = entry?.message ?? ""; | ||
| return `[${time}] ${message}\n`; |
Contributor
There was a problem hiding this comment.
Preserve the full tunnel timestamp in exported lines.
electron/services/tunnelManager.js, Lines 41-47 already stores a full ISO timestamp, but these formatters collapse it to local time-of-day only. That makes multi-day exports ambiguous and drops the original timezone when the file is shared.
🛠️ Suggested fix
const formatLine = (entry) => {
const timestamp = entry?.timestamp ? new Date(entry.timestamp) : null;
- const time = timestamp ? timestamp.toLocaleTimeString("en-US", { hour12: false }) : "unknown";
+ const time =
+ timestamp && !Number.isNaN(timestamp.getTime())
+ ? timestamp.toISOString()
+ : "unknown";
const message = entry?.message ?? "";
return `[${time}] ${message}\n`;
};Apply the same formatter change in both handlers.
Also applies to: 485-489
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.js` around lines 451 - 455, The current formatLine
function reduces entry.timestamp to a local time-of-day string, losing date and
timezone; update formatLine (and the other handler's equivalent) to preserve the
full ISO timestamp from entry.timestamp (e.g., use entry.timestamp or new
Date(entry.timestamp).toISOString()) in the exported line while still optionally
including the local time if desired, ensuring you reference entry.timestamp and
the formatLine function so the output contains the full unambiguous timestamp
rather than just the time-of-day.
… for production publishing.
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 by CodeRabbit
New Features
Improvements