feat: reload Kaoto editor on external file changes#1330
Conversation
📝 WalkthroughWalkthroughAdds a debounced file watcher that detects external edits to a single YAML file (ignoring VS Code self-saves), integrates it into the Kaoto editor channel to auto-reload editor content on external changes, and includes unit and end-to-end tests validating the behavior. Changes
Sequence DiagramsequenceDiagram
actor External as External Process (e.g., git pull)
participant FSW as FileSystemWatcher
participant Watcher as ExternalFileChangeWatcher
participant API as VSCodeKaotoEditorChannelApi
participant Editor as Kaoto Editor
External->>FSW: Modify YAML file on disk
FSW->>Watcher: onDidChange event
Watcher->>Watcher: Start debounce timer (300ms)
Watcher->>Watcher: Read file contents from disk (UTF-8)
Watcher->>Watcher: Compare with lastSelfSavedContent
alt External change detected
Watcher->>API: onExternalChange(newContent)
API->>API: Normalize path (workspace-relative or basename)
API->>Editor: setContent(normalizedPath, content)
Editor->>Editor: Refresh diagram
else Self-save detected
Watcher->>Watcher: Suppress callback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Looks good. Thanks for also providing the video. Have you tested the case when you have conflicting changes? Means when I open the Kaoto editor and have unsaved changes and then the process outside changes the file too? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
it-tests/ExternalFileChange.test.ts (1)
54-64: Consider wrapping closeEditor in try-catch for robustness.If
closeEditorthrows (e.g., editor already closed or unexpected modal state), the afterEach hook would fail. Wrapping it would make cleanup more resilient across various failure modes.♻️ Proposed defensive cleanup
afterEach(async function () { if (globalKaotoWebView !== undefined) { try { await globalKaotoWebView.switchBack(); } catch { // editor may already be closed, continue } globalKaotoWebView = undefined; } - await closeEditor(testFileName, false); + try { + await closeEditor(testFileName, false); + } catch { + // editor may already be closed or unavailable, continue + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@it-tests/ExternalFileChange.test.ts` around lines 54 - 64, The afterEach cleanup should not fail if closeEditor throws; wrap the existing await closeEditor(testFileName, false) call inside a try-catch so any errors (e.g., editor already closed or modal issues) are caught and ignored or logged for debugging; update the afterEach block that references globalKaotoWebView, switchBack(), and closeEditor(testFileName, false) to perform the closeEditor call inside a try { await closeEditor(...) } catch (e) { /* swallow or log e */ } to make cleanup robust.src/webview/VSCodeKaotoEditorChannelApi.ts (1)
41-46: Consider wrapping editor.setContent in try-catch.If
editor.setContentthrows, the error may surface as an unhandled promise rejection. Wrapping the call would allow for graceful error handling and logging.♻️ Proposed defensive error handling
this.externalFileChangeWatcher = new ExternalFileChangeWatcher(docUri, async (content) => { const normalizedPath = workspaceFolder ? path.relative(workspaceFolder.uri.fsPath, docUri.fsPath).split(path.sep).join('/') : path.basename(docUri.fsPath); - await editor.setContent(normalizedPath, content); - KaotoOutputChannel.logInfo(`Kaoto editor reloaded after external change to: ${docUri.fsPath}`); + try { + await editor.setContent(normalizedPath, content); + KaotoOutputChannel.logInfo(`Kaoto editor reloaded after external change to: ${docUri.fsPath}`); + } catch (ex) { + KaotoOutputChannel.logError(`Failed to reload Kaoto editor after external change to: ${docUri.fsPath}`, ex); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webview/VSCodeKaotoEditorChannelApi.ts` around lines 41 - 46, The async callback passed to ExternalFileChangeWatcher can cause an unhandled rejection if editor.setContent throws; wrap the await editor.setContent(normalizedPath, content) in a try-catch inside that callback, on success keep the KaotoOutputChannel.logInfo(...) call, and on catch call KaotoOutputChannel.logError(...) (including the error and docUri.fsPath or normalizedPath) so failures are logged and don't surface as unhandled promise rejections.src/helpers/ExternalFileChangeWatcher.ts (1)
56-60: Potential for callback invocation after dispose.If
dispose()is called whilehandleFileChange()is mid-execution (after the timer fires but beforeonExternalChangecompletes), the callback may still run. Consider adding a disposed flag to guard the callback invocation.♻️ Proposed fix with disposed guard
export class ExternalFileChangeWatcher implements vscode.Disposable { private readonly didSaveDisposable: vscode.Disposable; private readonly fileWatcher: vscode.FileSystemWatcher; private lastSelfSavedContent: string | undefined; private debounceTimer: NodeJS.Timeout | undefined; + private disposed = false; // ... constructor ... private async handleFileChange(): Promise<void> { + if (this.disposed) { + return; + } const content = await fs.readFile(this.docUri.fsPath, 'utf8'); // ... } dispose(): void { + this.disposed = true; this.fileWatcher.dispose(); // ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helpers/ExternalFileChangeWatcher.ts` around lines 56 - 60, Add a disposed guard to prevent callbacks running after disposal: introduce a boolean (e.g., this.disposed) set to true in dispose(), ensure dispose clears any pending this.debounceTimer, and modify the timeout callback and the async handleFileChange() to check this.disposed before proceeding (and again before calling onExternalChange) so that if dispose() was called while the timer fired or while handleFileChange is mid-execution the callback short-circuits and does not call onExternalChange.
🤖 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/helpers/ExternalFileChangeWatcher.ts`:
- Around line 63-73: In handleFileChange, wrap the
fs.readFile(this.docUri.fsPath, 'utf8') call in a try/catch so file read errors
(e.g., file deleted/renamed/permission denied) do not cause unhandled
rejections; on error set this.lastSelfSavedContent = undefined and bail out (or
optionally log the error) instead of letting the exception propagate, then only
call this.onExternalChange(content) when the read succeeded and content differs
from this.lastSelfSavedContent.
---
Nitpick comments:
In `@it-tests/ExternalFileChange.test.ts`:
- Around line 54-64: The afterEach cleanup should not fail if closeEditor
throws; wrap the existing await closeEditor(testFileName, false) call inside a
try-catch so any errors (e.g., editor already closed or modal issues) are caught
and ignored or logged for debugging; update the afterEach block that references
globalKaotoWebView, switchBack(), and closeEditor(testFileName, false) to
perform the closeEditor call inside a try { await closeEditor(...) } catch (e) {
/* swallow or log e */ } to make cleanup robust.
In `@src/helpers/ExternalFileChangeWatcher.ts`:
- Around line 56-60: Add a disposed guard to prevent callbacks running after
disposal: introduce a boolean (e.g., this.disposed) set to true in dispose(),
ensure dispose clears any pending this.debounceTimer, and modify the timeout
callback and the async handleFileChange() to check this.disposed before
proceeding (and again before calling onExternalChange) so that if dispose() was
called while the timer fired or while handleFileChange is mid-execution the
callback short-circuits and does not call onExternalChange.
In `@src/webview/VSCodeKaotoEditorChannelApi.ts`:
- Around line 41-46: The async callback passed to ExternalFileChangeWatcher can
cause an unhandled rejection if editor.setContent throws; wrap the await
editor.setContent(normalizedPath, content) in a try-catch inside that callback,
on success keep the KaotoOutputChannel.logInfo(...) call, and on catch call
KaotoOutputChannel.logError(...) (including the error and docUri.fsPath or
normalizedPath) so failures are logged and don't surface as unhandled promise
rejections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1812f034-21b1-4213-a59f-93839b11247d
📒 Files selected for processing (4)
it-tests/ExternalFileChange.test.tssrc/helpers/ExternalFileChangeWatcher.tssrc/test/helpers/ExternalFileChangeWatcher.test.tssrc/webview/VSCodeKaotoEditorChannelApi.ts
Ah, good thought.. Unfortunately if another process overwrites the file, the unsaved local changes will be lost. |
Just throwing an idea...
|
Great idea, I'll get started on this. Thanks! |
|
But I would only show the dialog if the current Kaoto Editor is marked dirty and then an external change is detected. Otherwise it will end up with a lot of annoying dialogs for the user :D |
|
This change is a bit harder than I expected because the editor is being marked as dirty even when no local modifications are made but background updates happen. This complicates the logic because subsequent updates will trigger the prompt each time even if no local changes were made. I'm thinking through how to solve this, will look more into it once I get some time. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
|
rebased the PR and also comitted the suggested try/catch change from coderabbit |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/webview/VSCodeKaotoEditorChannelApi.ts (1)
24-24: Mark the field asreadonlysince it's never reassigned.SonarCloud correctly identified that
externalFileChangeWatcheris only assigned in the constructor and never reassigned.♻️ Proposed fix
- private externalFileChangeWatcher: ExternalFileChangeWatcher | undefined; + private readonly externalFileChangeWatcher: ExternalFileChangeWatcher | undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/webview/VSCodeKaotoEditorChannelApi.ts` at line 24, The field externalFileChangeWatcher on class VSCodeKaotoEditorChannelApi is only assigned in the constructor and should be declared readonly; update the declaration "private externalFileChangeWatcher: ExternalFileChangeWatcher | undefined" to "private readonly externalFileChangeWatcher: ExternalFileChangeWatcher | undefined" and keep the existing assignment in the constructor (no other code changes needed).src/helpers/ExternalFileChangeWatcher.ts (1)
17-18: Consider using Node.js protocol prefixes for built-in modules.SonarCloud flagged the use of
fsandpathwithout thenode:prefix. Using thenode:prefix is a modern best practice that makes it explicit these are Node.js built-in modules and avoids potential naming conflicts with npm packages.♻️ Proposed fix
-import { promises as fs } from 'fs'; -import * as path from 'path'; +import { promises as fs } from 'node:fs'; +import * as path from 'node:path';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helpers/ExternalFileChangeWatcher.ts` around lines 17 - 18, Update the built-in module imports to use Node.js protocol prefixes: replace the current imports of fs and path (the lines importing "{ promises as fs } from 'fs'" and "import * as path from 'path'") with their node: counterparts so the module resolution is explicit (use node:fs and node:path); keep the same imported symbols (fs and path) so usages throughout ExternalFileChangeWatcher (e.g., any functions/methods referencing fs.promises or path.*) continue to work unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/helpers/ExternalFileChangeWatcher.ts`:
- Around line 17-18: Update the built-in module imports to use Node.js protocol
prefixes: replace the current imports of fs and path (the lines importing "{
promises as fs } from 'fs'" and "import * as path from 'path'") with their node:
counterparts so the module resolution is explicit (use node:fs and node:path);
keep the same imported symbols (fs and path) so usages throughout
ExternalFileChangeWatcher (e.g., any functions/methods referencing fs.promises
or path.*) continue to work unchanged.
In `@src/webview/VSCodeKaotoEditorChannelApi.ts`:
- Line 24: The field externalFileChangeWatcher on class
VSCodeKaotoEditorChannelApi is only assigned in the constructor and should be
declared readonly; update the declaration "private externalFileChangeWatcher:
ExternalFileChangeWatcher | undefined" to "private readonly
externalFileChangeWatcher: ExternalFileChangeWatcher | undefined" and keep the
existing assignment in the constructor (no other code changes needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81174ee2-b15c-472e-a349-59a349612033
📒 Files selected for processing (4)
it-tests/ExternalFileChange.test.tssrc/helpers/ExternalFileChangeWatcher.tssrc/test/helpers/ExternalFileChangeWatcher.test.tssrc/webview/VSCodeKaotoEditorChannelApi.ts
✅ Files skipped from review due to trivial changes (1)
- src/test/helpers/ExternalFileChangeWatcher.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- it-tests/ExternalFileChange.test.ts
2 participants
Fixes #1192 by implementing an
ExternalFileChangeWatcher.This is especially useful when using AI agents to update route files, as the file must currently be closed and reopened to see the changes.
Demo video:
https://github.com/user-attachments/assets/229004c0-7c93-4d41-8eac-a5d7b7ec0a4b
Summary by CodeRabbit
New Features
Tests