fix: prevent document editor panel closing on second show; close document on panel dismiss#4821
Conversation
…ment on panel dismiss Address review feedback on PR #4502: - Replace togglePanel(.documentEditor) with direct selection assignment to avoid accidentally closing the panel when a second document_editor_show or document_load_response message arrives while the panel is already open - Call documentManager.closeDocument() when closing the document editor panel to reset stale editorCoordinator state, ensuring reopened documents load correctly Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Devin Review found 1 potential issue.
🐛 1 issue in files not directly in the diff
🐛 closeDocument() does not reset editorCoordinator, causing stale coordinator to swallow initial content on reopen (clients/macos/vellum-assistant/Features/MainWindow/DocumentManager.swift:112-124)
The PR adds a documentManager.closeDocument() call on panel dismiss to fix stale coordinator state, but closeDocument() at DocumentManager.swift:112-124 never sets editorCoordinator = nil. This means the fix is incomplete.
Root Cause and Impact
After the user closes the document editor panel, SwiftUI tears down the DocumentEditorView and its WKWebView. However, documentManager.editorCoordinator still holds a strong reference to the old Coordinator object (whose webView weak reference is now nil).
When a new document is opened via createDocument() (DocumentManager.swift:47-62), the code checks:
if let coordinator = editorCoordinator {
coordinator.setInitialContent(title: title, markdown: initialContent)
} else {
pendingInitialContent = initialContent
}Since editorCoordinator is non-nil (stale), it takes the first branch and calls setInitialContent on the old coordinator. Inside setInitialContent (DocumentEditorView.swift:93-94):
guard let webView = webView else { return }The webView weak reference is nil, so the method silently returns. The content is never stored as pendingInitialContent either, so when the new DocumentEditorView creates a fresh coordinator and sets documentManager.editorCoordinator, the didSet at DocumentManager.swift:38-44 finds pendingInitialContent is nil and does nothing.
Impact: After closing and reopening the document editor, documents will appear without their initial content — the exact bug the PR description claims to fix.
View 3 additional findings in Devin Review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9aa64f491a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| documentManager: documentManager, | ||
| daemonClient: daemonClient, | ||
| onClose: { windowState.selection = nil } | ||
| onClose: { windowState.selection = nil; documentManager.closeDocument() } |
There was a problem hiding this comment.
Preserve unsaved edits when dismissing document panel
Calling documentManager.closeDocument() inside the panel close handler turns a UI dismiss action into a hard document reset, which clears currentContent/initialContent and cancels pending saves in DocumentManager.closeDocument(). In the common flow where a user types in the editor but has not clicked Save yet, closing and reopening the panel now drops those local edits and reloads only daemon-backed content, causing silent data loss that did not happen when close only hid the panel.
Useful? React with 👍 / 👎.
Summary
Addresses review feedback on #4502
Fix 1 (
MainWindow.swift): ReplacetogglePanel(.documentEditor)withwindowState.selection = .panel(.documentEditor)in bothhandleDocumentEditorShowandhandleDocumentLoadResponse. The previoustogglePanelcall would accidentally close the panel if a seconddocument_editor_showordocument_load_responsemessage arrived while the panel was already open (becausetogglePanelchecks if the panel is already selected and closes it if so).Fix 2 (
MainWindowView.swift): CalldocumentManager.closeDocument()in theonClosehandler forDocumentEditorPanelView. When the user closes the document editor panel, the document manager state (includingeditorCoordinator) was not being reset. This stale state meant that on the nextdocument_editor_show,createDocumentmight try to use the old coordinator (which no longer has a live web view), causing documents to appear without their initial content.Test plan
document_editor_showwhile panel is open: verify panel stays open with new contentdocument_load_responsewhile panel is open: verify panel stays open with new content🤖 Generated with Claude Code