Repository navigation
terminal suggest - adopt vscode.fs watcher - #276477
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the file system watching mechanism in the terminal-suggest extension to use VS Code's createFileSystemWatcher API instead of Node.js's native fs.watch. This improves consistency with VS Code's extension patterns and ensures proper lifecycle management through the extension context.
Key Changes
- Migrated from Node.js
fs.watchto VS Code'screateFileSystemWatcherAPI - Improved watcher registration by adding to
context.subscriptionsfor automatic cleanup - Optimized directory checking by moving the
activeWatchers.has(dir)check earlier in the loop
| if (activeWatchers.has(dir)) { | ||
| // Skip if already watching or directory doesn't exist | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| const stat = await fs.stat(dir); | ||
| if (!stat.isDirectory()) { | ||
| continue; | ||
| } | ||
| } catch { | ||
| // File not found | ||
| continue; | ||
| } | ||
|
|
||
| const watcher = filesystem.watch(dir, { persistent: false }, () => { | ||
| if (pathExecutableCache) { | ||
| // Refresh cache when directory contents change | ||
| pathExecutableCache.refresh(dir); | ||
| } | ||
| }); | ||
| const handleChange = () => { | ||
| // Refresh cache when directory contents change | ||
| pathExecutableCache?.refresh(dir); | ||
| }; | ||
|
|
||
| activeWatchers.add(dir); | ||
| const watcher = vscode.workspace.createFileSystemWatcher(new vscode.RelativePattern(vscode.Uri.file(dir), '*')); | ||
| context.subscriptions.push( | ||
| watcher, | ||
| watcher.onDidCreate(handleChange), | ||
| watcher.onDidChange(handleChange), | ||
| watcher.onDidDelete(handleChange) | ||
| ); | ||
|
|
||
| context.subscriptions.push(new vscode.Disposable(() => { | ||
| try { | ||
| watcher.close(); | ||
| activeWatchers.delete(dir); | ||
| } catch { } { } | ||
| })); | ||
| } catch { } | ||
| activeWatchers.add(dir); |
There was a problem hiding this comment.
The activeWatchers set is scoped locally to this function invocation, which means it won't persist across multiple calls to watchPathDirectories. If this function could be called multiple times (e.g., when the PATH environment variable changes), it would create duplicate watchers for the same directories without the ability to track or clean them up. Consider moving activeWatchers to module scope or implementing a more robust tracking mechanism that persists across function calls.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Megan Rogge (meganrogge)
left a comment
Megan Rogge (meganrogge)
left a comment
There was a problem hiding this comment.
I think I was not aware of this API. Thank you!
Megan Rogge (@meganrogge) do you recall why you were using
fsfor file watching and not ourvscodeAPI?//cc Henning Dieterichs (@hediet)