fix: address document editor review feedback from PR #4502#4578
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 message arrives while the panel is already open - Call documentManager.closeDocument() when closing the document editor panel to reset stale editorCoordinator state Co-Authored-By: Claude <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd7a1af05e
ℹ️ 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.
Avoid clearing DocumentManager in this close handler, because the current reopen path only posts MainWindow.openDocumentEditor from InlineSurfaceRouter (clients/shared/Features/Chat/InlineWidgets/InlineSurfaceRouter.swift:82,114) and MainWindowView handles that notification by only setting selection = .panel(.documentEditor) (MainWindowView.swift:465-467) without recreating document state. After this change, closing via the panel X sets hasActiveDocument to false, so reopening from an inline document preview no longer shows the editor (showPanel: documentManager.hasActiveDocument at MainWindowView.swift:1149).
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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, leaving stale coordinator reference (clients/macos/vellum-assistant/Features/MainWindow/DocumentManager.swift:88-98)
The PR description explicitly states that closeDocument() should reset the editorCoordinator to fix stale state, but the implementation doesn't actually set editorCoordinator = nil. When the user closes the document editor panel and a new document_editor_show message arrives, createDocument at DocumentManager.swift:52 finds a non-nil editorCoordinator (pointing to the old, dead coordinator) and calls setInitialContent on it instead of setting pendingInitialContent.
Root cause and impact
In closeDocument() (DocumentManager.swift:88-98), every piece of state is reset except editorCoordinator. The editorCoordinator property is a strong reference (var editorCoordinator: DocumentEditorCoordinator?), so the old Coordinator object is retained even after its WKWebView is destroyed.
On the next createDocument call (DocumentManager.swift:52-54):
if let coordinator = editorCoordinator {
coordinator.setInitialContent(title: title, markdown: initialContent)The stale coordinator is found, and setInitialContent is called. Inside the coordinator (DocumentEditorView.swift:93-94):
guard let webView = webView else { return }The weak var webView is likely nil (WKWebView was deallocated), so the call silently returns. Crucially, pendingInitialContent is never set because the else branch at line 56 was skipped.
The content still appears to load because makeNSView calls contentForEditorView() as a fallback, but this is fragile — if the old WKWebView hasn't been deallocated yet (SwiftUI view lifecycle is non-deterministic), setInitialContent would send content to the wrong, about-to-be-destroyed webview.
Impact: Stale coordinator reference causes a memory leak and makes the content loading path fragile/non-deterministic on panel reopen.
View 4 additional findings in Devin Review.
|
📦 macOS DMG artifact for this PR is ready for download: Download vellum-assistant-pr-4578.dmg |
1 participant
Summary
Addresses review feedback on #4502
Fix 1: Replace
togglePanel(.documentEditor)withwindowState.selection = .panel(.documentEditor)inhandleDocumentEditorShow. The previoustogglePanelcall would accidentally close the panel if a seconddocument_editor_showmessage arrived while the panel was already open (because togglePanel checks if the panel is already selected and closes it if so).Fix 2: Call
documentManager.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 content🤖 Generated with Claude Code