Watch directory#166
Conversation
|
@copilot Check this PR for errors |
|
@willmcgugan I've opened a new pull request, #167, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
This PR adds filesystem monitoring using the watchdog library to optimize refresh behavior. Previously, Toad refreshed the filesystem after every agent turn unconditionally. Now, directory changes are detected in real-time and refreshes only occur when actual filesystem modifications are detected.
Key changes:
- Introduces a new
DirectoryWatcherclass that monitors file/directory creation, deletion, and moves - Modifies the conversation flow to conditionally refresh only when directory changes are detected
- Adds the watchdog library as a dependency
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/toad/directory_watcher.py | New file implementing DirectoryWatcher class using watchdog's Observer pattern to monitor filesystem changes |
| src/toad/widgets/conversation.py | Integrates DirectoryWatcher lifecycle management and conditional refresh logic based on directory change events |
| pyproject.toml | Adds watchdog>=6.0.0 as a project dependency |
| CHANGELOG.md | Documents the filesystem monitoring improvement in the Unreleased section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| super().__init__() | ||
|
|
||
| def on_any_event(self, event: FileSystemEvent) -> None: | ||
| """We want to respond to files or directories being added, removed, or removed. |
There was a problem hiding this comment.
The docstring contains a repetition error. It says "added, removed, or removed" but should say "added, removed, or moved".
| """We want to respond to files or directories being added, removed, or removed. | |
| """We want to respond to files or directories being added, removed, or moved. |
|
|
||
| ### Changed | ||
|
|
||
| - Smarter filesystem monitoring to avoid refreshes were nothing has changed |
There was a problem hiding this comment.
Grammar error: should be "where nothing has changed" instead of "were nothing has changed".
| - Smarter filesystem monitoring to avoid refreshes were nothing has changed | |
| - Smarter filesystem monitoring to avoid refreshes where nothing has changed |
| DirModifiedEvent, | ||
| FileModifiedEvent, |
There was a problem hiding this comment.
The imports DirModifiedEvent and FileModifiedEvent are unused. They are imported on lines 9-10 but are not referenced anywhere in the event filtering logic. These imports should be removed.
| DirModifiedEvent, | |
| FileModifiedEvent, |
| if ready: | ||
| self._directory_watcher = DirectoryWatcher(self.project_path, self) | ||
| self._directory_watcher.start() |
There was a problem hiding this comment.
Potential resource leak: the directory watcher is created when agent_ready becomes True, but if agent_ready transitions from True back to False (e.g., when the agent changes as seen in watch_agent at line 1200), the existing watcher is not stopped before creating a new one. This could lead to multiple watchers running simultaneously or orphaned observer threads. Consider stopping any existing watcher before creating a new one, or only create the watcher if one doesn't already exist.
| if ready: | |
| self._directory_watcher = DirectoryWatcher(self.project_path, self) | |
| self._directory_watcher.start() | |
| existing_watcher = getattr(self, "_directory_watcher", None) | |
| if not ready: | |
| # Agent is no longer ready; stop any existing directory watcher. | |
| if existing_watcher is not None: | |
| existing_watcher.stop() | |
| self._directory_watcher = None | |
| return | |
| # Agent is ready; restart the directory watcher to reflect current state. | |
| if existing_watcher is not None: | |
| existing_watcher.stop() | |
| self._directory_watcher = DirectoryWatcher(self.project_path, self) | |
| self._directory_watcher.start() |
| def on_any_event(self, event: FileSystemEvent) -> None: | ||
| """We want to respond to files or directories being added, removed, or removed. | ||
|
|
||
| We want to ignore any changes changes that are purely file data or metadata. |
There was a problem hiding this comment.
There is a repeated word "changes" in the comment. It should say "any changes that are" instead of "any changes changes that are".
| We want to ignore any changes changes that are purely file data or metadata. | |
| We want to ignore any changes that are purely file data or metadata. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def start(self) -> None: | ||
| """Start the watcher.""" | ||
|
|
||
| self._observer.schedule( | ||
| self, | ||
| str(self._path), | ||
| recursive=True, | ||
| event_filter=[ | ||
| FileCreatedEvent, | ||
| FileDeletedEvent, | ||
| FileMovedEvent, | ||
| DirCreatedEvent, | ||
| DirDeletedEvent, | ||
| DirMovedEvent, | ||
| ], | ||
| ) | ||
| self._observer.start() | ||
|
|
There was a problem hiding this comment.
The start() method is called synchronously (line 1208) but could potentially block if the Observer takes time to initialize. Consider making this method async and using asyncio.to_thread() similar to the stop() method to avoid blocking the event loop during startup. This would ensure consistent async behavior for both lifecycle methods.
| def start(self) -> None: | |
| """Start the watcher.""" | |
| self._observer.schedule( | |
| self, | |
| str(self._path), | |
| recursive=True, | |
| event_filter=[ | |
| FileCreatedEvent, | |
| FileDeletedEvent, | |
| FileMovedEvent, | |
| DirCreatedEvent, | |
| DirDeletedEvent, | |
| DirMovedEvent, | |
| ], | |
| ) | |
| self._observer.start() | |
| async def start(self) -> None: | |
| """Start the watcher.""" | |
| def run() -> None: | |
| """Start the observer in a thread.""" | |
| self._observer.schedule( | |
| self, | |
| str(self._path), | |
| recursive=True, | |
| event_filter=[ | |
| FileCreatedEvent, | |
| FileDeletedEvent, | |
| FileMovedEvent, | |
| DirCreatedEvent, | |
| DirDeletedEvent, | |
| DirMovedEvent, | |
| ], | |
| ) | |
| self._observer.start() | |
| await asyncio.to_thread(run) |
| widget: Widget which will receive the `DirectoryChanged` event. | ||
| """ | ||
| self._path = path |
There was a problem hiding this comment.
The on_any_event method posts a message but doesn't handle potential exceptions. If the widget is being torn down or the message queue is full, post_message() could raise an exception, causing the watchdog observer to potentially stop working or log errors. Consider wrapping this in a try-except block to gracefully handle edge cases where the widget might no longer be mounted.
| DirCreatedEvent, | ||
| DirDeletedEvent, | ||
| DirMovedEvent, | ||
| ], | ||
| ) | ||
| self._observer.start() | ||
|
|
There was a problem hiding this comment.
The observer is stopped with a 1-second timeout in the join() call. If the observer thread doesn't stop within this timeout, it will be left running. Consider checking the return value or adding error handling to ensure the thread is properly cleaned up. Alternatively, document why a 1-second timeout is sufficient and what happens if it's exceeded.
| def __rich_repr__(self) -> rich.repr.Result: | ||
| yield self._path | ||
| yield self._widget | ||
|
|
||
| def start(self) -> None: | ||
| """Start the watcher.""" | ||
|
|
||
| self._observer.schedule( | ||
| self, | ||
| str(self._path), | ||
| recursive=True, | ||
| event_filter=[ | ||
| FileCreatedEvent, |
There was a problem hiding this comment.
The event_filter parameter may not be available in current versions of the watchdog library (as of early 2025, the standard API uses event_handler, path, and recursive parameters). If this is a feature from a newer version, verify that watchdog 6.0.0 exists and supports this parameter. Otherwise, consider implementing event filtering by overriding specific event handler methods (like on_created, on_deleted, on_moved) instead of using on_any_event, or filter events within the on_any_event method using isinstance() checks.
|
|
||
| ### Changed | ||
|
|
||
| - Smarter filesystem monitoring to avoid refreshes where nothing has changed |
There was a problem hiding this comment.
There is a spelling error in this line. The word "were" should be "where" to make the sentence grammatically correct: "Smarter filesystem monitoring to avoid refreshes where nothing has changed".
| - Smarter filesystem monitoring to avoid refreshes where nothing has changed | |
| - Smarter filesystem monitoring to avoid refreshes where nothing has changed |
| self._turn_count = 0 | ||
| self._shell_count = 0 | ||
|
|
||
| self._directory_changed = False |
There was a problem hiding this comment.
The _directory_changed flag is accessed from multiple contexts without synchronization: it's set in on_directory_changed (which is called from the watchdog observer thread via post_message), and checked/reset in on_terminal_finalized and agent_turn_over.
While Textual's message queue should serialize message handling, there's a potential race condition if filesystem events occur rapidly. Consider using atomic operations or adding a comment explaining the thread-safety guarantees provided by Textual's event system.
Use the watchdog library to monitor the project for changes
Previously Toad would refresh the filesystem after every change, which could result in unnecessary refreshes.