fix: track contents to avoid reloading on our own writes #7099
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements a mechanism to prevent unnecessary file reloads when the file system change was triggered by the application's own save operation. The implementation tracks the last saved content and compares it with the current file content before reloading.
- Adds
_last_saved_contenttracking toAppFileManagerto store the content of the last persisted save - Implements
file_content_matches_last_save()method to check if current file content matches the last save - Updates the file change handler in sessions.py to skip reload when content matches the last save
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| marimo/_server/file_manager.py | Adds content tracking via _last_saved_content field, implements comparison method, and updates _save_file signature to use keyword arguments |
| marimo/_server/sessions.py | Adds early return in file change handler when content matches last save to avoid unnecessary reloads |
| tests/_server/test_file_manager.py | Adds comprehensive test coverage for the new file_content_matches_last_save() functionality |
Comments suppressed due to low confidence (2)
marimo/_server/file_manager.py:66
- The
reload()method should clear_last_saved_contentafter successfully loading external changes. Without this, if an external change is reloaded, subsequent saves will still compare against the old content, potentially causing incorrect skip behavior. Addself._last_saved_content = Noneafter line 66 to reset tracking when external changes are loaded.
def reload(self) -> set[CellId_t]:
"""
Reload the app from the file.
Return any new cell IDs that were added or code that was changed.
"""
prev_cell_manager = self.app.cell_manager
self.app = self._load_app(self.path)
marimo/_server/file_manager.py:345
- The
copy()method directly writes to the filesystem usingshutil.copy()without updating_last_saved_content. If the destination is the same asself.filename, this could lead to incorrect behavior where the file watcher detects a change butfile_content_matches_last_save()returns False even though the application made the change. Consider updating_last_saved_contentifdestination == self.filename.
def copy(self, request: CopyNotebookRequest) -> str:
source, destination = request.source, request.destination
shutil.copy(source, destination)
return os.path.basename(destination)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def file_content_matches_last_save(self) -> bool: | ||
| """Check if the current file content matches the last saved content. | ||
| This is used to avoid reloading the file when we detect our own writes. | ||
| Returns: | ||
| bool: True if the file content matches the last save, False otherwise. | ||
| """ | ||
| if self.filename is None or self._last_saved_content is None: | ||
| return False | ||
|
|
||
| try: | ||
| current_content = Path(self.filename).read_text(encoding="utf-8") | ||
| return current_content.strip() == self._last_saved_content |
There was a problem hiding this comment.
There's a potential race condition where the file could be modified between the file watcher detecting a change and this comparison being executed. While the file change locks in sessions.py provide some protection, consider the scenario where: 1) save writes file A, 2) file watcher queues change event, 3) external tool writes file B, 4) this method runs and compares file B with saved content A. The method would return False and trigger a reload, but then the save for file B might complete before the reload, causing the reload to skip. Consider adding a timestamp-based check or file system modification time comparison as an additional validation.
| if persist: | ||
| self._create_file(filename, contents) | ||
| # Record the last saved content to avoid reloading our own writes | ||
| self._last_saved_content = contents.strip() |
There was a problem hiding this comment.
Could save just the hash
There was a problem hiding this comment.
it saves a bit on space (not much), but requires hashing each time
| ) | ||
|
|
||
| # Now the file should match the last save | ||
| assert manager.file_content_matches_last_save() is True |
There was a problem hiding this comment.
Can you add another call mocking debug to make sure that we are skipping? Or maybe mock reload to make sure never called?
There was a problem hiding this comment.
i have this already in a followup PR, will push it up in a bit
This PR track contents that we wrong to avoid reloading on our own writes when file watching.