diff --git a/marimo/_server/file_manager.py b/marimo/_server/file_manager.py index 08805fbcc64..16e55685b3a 100644 --- a/marimo/_server/file_manager.py +++ b/marimo/_server/file_manager.py @@ -66,8 +66,10 @@ def reload(self) -> set[CellId_t]: # Return the changes cell IDs prev_cell_ids = set(prev_cell_manager.cell_ids()) - changed_cell_ids: set[CellId_t] = set() - for cell_id in self.app.cell_manager.cell_ids(): + current_cell_ids = set(self.app.cell_manager.cell_ids()) + # Capture deleted cells + changed_cell_ids: set[CellId_t] = prev_cell_ids - current_cell_ids + for cell_id in current_cell_ids: if cell_id not in prev_cell_ids: changed_cell_ids.add(cell_id) new_code = self.app.cell_manager.get_cell_code(cell_id) diff --git a/marimo/_server/sessions.py b/marimo/_server/sessions.py index 497e6eba53a..fa1c2214d2a 100644 --- a/marimo/_server/sessions.py +++ b/marimo/_server/sessions.py @@ -50,6 +50,7 @@ from marimo._runtime.requests import ( AppMetadata, CreationRequest, + DeleteCellRequest, ExecuteMultipleRequest, ExecutionRequest, HTTPRequest, @@ -1205,24 +1206,36 @@ def _handle_file_change( # Auto-run cells if configured if should_autorun: - changed_cell_ids_list = list(changed_cell_ids) cell_ids_to_idx = { cell_id: idx for idx, cell_id in enumerate(cell_ids) } + deleted = { + cell_id + for cell_id in changed_cell_ids + if cell_id not in cell_ids_to_idx + } + changed_cell_ids_list = list(changed_cell_ids - deleted) changed_codes = [ codes[cell_ids_to_idx[cell_id]] for cell_id in changed_cell_ids_list + if cell_id not in deleted ] - # This runs the request and also runs UpdateCellCodes - session.put_control_request( - ExecuteMultipleRequest( - cell_ids=changed_cell_ids_list, - codes=changed_codes, - request=None, - ), - from_consumer_id=None, - ) + if changed_cell_ids_list: + # This runs the request and also runs UpdateCellCodes + session.put_control_request( + ExecuteMultipleRequest( + cell_ids=changed_cell_ids_list, + codes=changed_codes, + request=None, + ), + from_consumer_id=None, + ) + for to_delete in deleted: + session.put_control_request( + DeleteCellRequest(cell_id=to_delete), + from_consumer_id=None, + ) else: session.write_operation( UpdateCellCodes( diff --git a/tests/_server/test_file_manager.py b/tests/_server/test_file_manager.py index 8cae82003da..e29257e0ee9 100644 --- a/tests/_server/test_file_manager.py +++ b/tests/_server/test_file_manager.py @@ -1,9 +1,7 @@ from __future__ import annotations import os -import shutil import sys -import tempfile from typing import TYPE_CHECKING import pytest @@ -30,15 +28,15 @@ @pytest.fixture -def app_file_manager() -> Generator[AppFileManager, None, None]: +def app_file_manager(tmp_path: Path) -> Generator[AppFileManager, None, None]: """ Creates an AppFileManager instance with a temporary file. """ # Create a temporary file - temp_file = tempfile.NamedTemporaryFile(suffix=".py", delete=False) + temp_file = tmp_path / "test_app.py" - temp_file.write( - b""" + temp_file.write_text( + """ import marimo __generated_with = "0.0.1" app = marimo.App() @@ -52,15 +50,11 @@ def __(): """ ) - temp_file.close() - # Instantiate AppFileManager with the temporary file and a mock app - manager = AppFileManager(filename=temp_file.name) - yield manager + manager = AppFileManager(filename=str(temp_file)) + return manager - # Clean up the temporary file - if os.path.exists(temp_file.name): - os.remove(temp_file.name) + # No manual cleanup needed - pytest handles it automatically def test_rename_same_filename(app_file_manager: AppFileManager) -> None: @@ -311,10 +305,10 @@ def test_to_code(app_file_manager: AppFileManager) -> None: ) -def test_reload_reorders_cells() -> None: +def test_reload_reorders_cells(tmp_path: Path) -> None: """Test that reload() reorders cell IDs based on similarity to previous cells.""" # Create a temporary file with initial content - temp_file = tempfile.NamedTemporaryFile(suffix=".py", delete=False) + temp_file = tmp_path / "test_reload.py" initial_content = """ import marimo __generated_with = "0.0.1" @@ -333,11 +327,10 @@ def cell2(): if __name__ == "__main__": app.run() """ - temp_file.write(initial_content.encode()) - temp_file.close() + temp_file.write_text(initial_content) # Initialize AppFileManager with the temp file - manager = AppFileManager(filename=temp_file.name) + manager = AppFileManager(filename=str(temp_file)) original_cell_ids = list(manager.app.cell_manager.cell_ids()) assert original_cell_ids == ["Hbol", "MJUe"] @@ -360,8 +353,7 @@ def cell1(): if __name__ == "__main__": app.run() """ - with open(temp_file.name, "w") as f: - f.write(modified_content) + temp_file.write_text(modified_content) # Reload the file changed_cell_ids = manager.reload() @@ -371,14 +363,12 @@ def cell1(): assert len(reloaded_cell_ids) == len(original_cell_ids) assert reloaded_cell_ids == ["MJUe", "Hbol"] assert changed_cell_ids == set() - # Clean up - os.remove(temp_file.name) -def test_reload_updates_content() -> None: +def test_reload_updates_content(tmp_path: Path) -> None: """Test that reload() updates the file contents correctly.""" # Create a temporary file with initial content - temp_file = tempfile.NamedTemporaryFile(suffix=".py", delete=False) + temp_file = tmp_path / "test_reload_content.py" initial_content = """ import marimo __generated_with = "0.0.1" @@ -392,11 +382,10 @@ def cell1(): if __name__ == "__main__": app.run() """ - temp_file.write(initial_content.encode()) - temp_file.close() + temp_file.write_text(initial_content) # Initialize AppFileManager with the temp file - manager = AppFileManager(filename=temp_file.name) + manager = AppFileManager(filename=str(temp_file)) original_code = list(manager.app.cell_manager.codes())[0] assert "x = 1" in original_code @@ -414,8 +403,7 @@ def cell1(): if __name__ == "__main__": app.run() """ - with open(temp_file.name, "w") as f: - f.write(modified_content) + temp_file.write_text(modified_content) # Reload the file changed_cell_ids = manager.reload() @@ -425,15 +413,13 @@ def cell1(): assert "x = 42" in reloaded_code assert "x = 1" not in reloaded_code assert changed_cell_ids == {"Hbol"} - # Clean up - os.remove(temp_file.name) -def test_reload_updates_new_cell() -> None: +def test_reload_updates_new_cell(tmp_path: Path) -> None: """Test that reload() updates the file contents correctly.""" # Create a temp file with initial content - temp_file = tempfile.NamedTemporaryFile(suffix=".py", delete=False) + temp_file = tmp_path / "test_reload_new_cell.py" initial_content = """ import marimo app = marimo.App() @@ -446,11 +432,10 @@ def cell1(): if __name__ == "__main__": app.run() """ - temp_file.write(initial_content.encode()) - temp_file.close() + temp_file.write_text(initial_content) # Initialize AppFileManager with the temp file - manager = AppFileManager(filename=temp_file.name) + manager = AppFileManager(filename=str(temp_file)) assert len(list(manager.app.cell_manager.codes())) == 1 original_cell_ids = list(manager.app.cell_manager.cell_ids()) assert original_cell_ids == ["Hbol"] @@ -473,8 +458,7 @@ def cell1(): if __name__ == "__main__": app.run() """ - with open(temp_file.name, "w") as f: - f.write(modified_content) + temp_file.write_text(modified_content) # Reload the file changed_cell_ids = manager.reload() @@ -487,27 +471,22 @@ def cell1(): next_cell_ids = list(manager.app.cell_manager.cell_ids()) assert next_cell_ids == ["MJUe", "Hbol"] assert changed_cell_ids == {"MJUe"} - # Clean up - os.remove(temp_file.name) -def test_rename_with_special_chars(app_file_manager: AppFileManager) -> None: +def test_rename_with_special_chars( + app_file_manager: AppFileManager, tmp_path: Path +) -> None: """Test that renaming files with special characters works.""" # Create a temporary file - temp_dir = tempfile.mkdtemp() - try: - initial_path = os.path.join(temp_dir, "test.py") - with open(initial_path, "w") as f: - f.write("import marimo") - app_file_manager.filename = initial_path - - # Try to rename to path with special characters - new_path = os.path.join(temp_dir, "test & space.py") - app_file_manager.rename(new_path) - assert app_file_manager.filename == new_path - assert os.path.exists(new_path) - finally: - shutil.rmtree(temp_dir) + initial_path = tmp_path / "test.py" + initial_path.write_text("import marimo") + app_file_manager.filename = str(initial_path) + + # Try to rename to path with special characters + new_path = tmp_path / "test & space.py" + app_file_manager.rename(str(new_path)) + assert app_file_manager.filename == str(new_path) + assert new_path.exists() def test_reload_reinitializes_graph(tmp_path: Path) -> None: @@ -544,9 +523,9 @@ def cell1(): assert manager.app._app._initialized is True # Check initial graph state - cell_ids = list(manager.app.cell_manager.cell_ids()) + cell_ids = set(manager.app.cell_manager.cell_ids()) assert len(cell_ids) == 1 - assert cell_ids[0] == cell_one + assert cell_ids == {cell_one} # Check the graph has the correct cells assert manager.app.graph.cells.keys() == {cell_one} @@ -625,7 +604,8 @@ def cell2(): # Reload the app changed_cell_ids = manager.reload() - assert changed_cell_ids == {cell_two} + # Technically cell 1 did change since it was deleted. + assert changed_cell_ids == {cell_one, cell_two} # Verify cell_manager has only one cell cell_ids = list(manager.app.cell_manager.cell_ids()) @@ -641,9 +621,6 @@ def cell2(): # Check the contents of the cell assert manager.app.cell_manager.get_cell_code(cell_two) == new_code - # Clean up - os.remove(tmp_file) - def test_default_app_settings(tmp_path: Path) -> None: """Test that default_sql_output and default_width are properly applied.""" @@ -699,3 +676,120 @@ def test_overload_app_settings() -> None: finally: os.environ.pop("_MARIMO_APP_OVERLOAD_SQL_OUTPUT", None) os.environ.pop("_MARIMO_APP_OVERLOAD_AUTO_DOWNLOAD", None) + + +def test_reload_detects_deleted_cells(tmp_path: Path) -> None: + """Test that reload() correctly detects deleted cells.""" + # Create a temporary file with two cells + temp_file = tmp_path / "test_reload_deleted.py" + initial_content = """ +import marimo +__generated_with = "0.0.1" +app = marimo.App() + +@app.cell +def cell1(): + x = 1 + return x + +@app.cell +def cell2(): + y = 2 + return y + +if __name__ == "__main__": + app.run() +""" + temp_file.write_text(initial_content) + + # Initialize AppFileManager with the temp file + manager = AppFileManager(filename=str(temp_file)) + original_cell_ids = list(manager.app.cell_manager.cell_ids()) + assert len(original_cell_ids) == 2 + + # Modify the file content - remove one cell + modified_content = """ +import marimo +__generated_with = "0.0.1" +app = marimo.App() + +@app.cell +def cell1(): + x = 1 + return x + +if __name__ == "__main__": + app.run() +""" + temp_file.write_text(modified_content) + + # Reload the file + changed_cell_ids = manager.reload() + + # The deleted cell should be included in changed_cell_ids + reloaded_cell_ids = list(manager.app.cell_manager.cell_ids()) + assert len(reloaded_cell_ids) == 1 + deleted_cell_ids = set(original_cell_ids) - set(reloaded_cell_ids) + assert len(deleted_cell_ids) == 1 + assert deleted_cell_ids.issubset(changed_cell_ids) + + +def test_reload_detects_multiple_deleted_cells(tmp_path: Path) -> None: + """Test that reload() correctly detects multiple deleted cells.""" + # Create a temporary file with three cells + temp_file = tmp_path / "test_reload_multiple_deleted.py" + initial_content = """ +import marimo +__generated_with = "0.0.1" +app = marimo.App() + +@app.cell +def cell1(): + x = 1 + return x + +@app.cell +def cell2(): + y = 2 + return y + +@app.cell +def cell3(): + z = 3 + return z + +if __name__ == "__main__": + app.run() +""" + temp_file.write_text(initial_content) + + # Initialize AppFileManager with the temp file + manager = AppFileManager(filename=str(temp_file)) + original_cell_ids = list(manager.app.cell_manager.cell_ids()) + assert len(original_cell_ids) == 3 + + # Modify the file content - keep only one cell + modified_content = """ +import marimo +__generated_with = "0.0.1" +app = marimo.App() + +@app.cell +def cell2(): + y = 2 + return y + +if __name__ == "__main__": + app.run() +""" + temp_file.write_text(modified_content) + + # Reload the file + changed_cell_ids = manager.reload() + + # Two cells should be deleted and included in changed_cell_ids + reloaded_cell_ids = list(manager.app.cell_manager.cell_ids()) + assert len(reloaded_cell_ids) == 1 + deleted_cell_ids = set(original_cell_ids) - set(reloaded_cell_ids) + assert len(deleted_cell_ids) == 2 + assert deleted_cell_ids.issubset(changed_cell_ids)