-
Notifications
You must be signed in to change notification settings - Fork 860
fix: relative path fixes for home mode #7722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
marimo/_server/recents.py
Outdated
| files: list[MarimoFile] = [] | ||
|
|
||
| cwd = pathlib.Path.cwd() | ||
| base_dir = (directory or pathlib.Path.cwd()).resolve() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure you want resolve here? should this just absolute path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran git bisect and found a regression in #7570 that broken "recent files" when verifying this. Will followup with a fix
9329322 to
ef6b6bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request fixes UX issues when using marimo edit subdir/ (home mode) by ensuring consistent use of relative paths throughout the application. The changes normalize how paths are handled when working with a specific directory context.
Key Changes:
- File router now stores directory as absolute path and returns relative paths for files
- Recent files are now filtered and displayed relative to the router's directory
- API endpoints resolve relative paths against the file router's directory before processing
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
marimo/_server/session_manager.py |
Moved RecentsTrackerListener subscription from per-session to session manager's event bus for proper recent files tracking |
marimo/_server/recents.py |
Added directory parameter to get_recents() to filter and return paths relative to a base directory |
marimo/_server/file_router.py |
Normalized directory to absolute path and changed filepath resolution to handle relative paths consistently |
marimo/_server/files/directory_scanner.py |
Modified file listing to return relative paths instead of absolute paths |
marimo/_server/api/endpoints/home.py |
Updated to pass file router's directory to recents manager for proper filtering |
marimo/_server/api/endpoints/files.py |
Added logic to resolve relative filenames against file router's directory in save, rename, and copy operations |
marimo/_server/api/endpoints/file_explorer.py |
Changed default root to use file router's directory instead of always using cwd |
marimo/_server/api/endpoints/assets.py |
Added logic to relativize filenames for display when possible |
tests/_server/test_session_manager.py |
Added tests for RecentsTrackerListener event bus subscription and integration |
tests/_server/test_file_router.py |
Updated assertions to match new relative path behavior |
tests/_server/test_file_manager_absolute_path.py |
Added comprehensive tests for relative path handling, path resolution, and security (path traversal protection) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| base_dir = directory or pathlib.Path.cwd() | ||
| limited_files = state.files[: self.MAX_FILES] | ||
| for file in limited_files: | ||
| file_path = pathlib.Path(file) |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code assumes all stored file paths in recent files are absolute, but this assumption is not enforced. If a relative path is somehow stored (e.g., through manual config file editing or a future code change), the is_relative_to check on line 93 will incorrectly return False when comparing a relative file_path to an absolute base_dir, causing valid recent files to be filtered out. Consider converting file_path to absolute before the is_relative_to check to make this code more robust.
| base_dir = directory or pathlib.Path.cwd() | |
| limited_files = state.files[: self.MAX_FILES] | |
| for file in limited_files: | |
| file_path = pathlib.Path(file) | |
| base_dir = (directory or pathlib.Path.cwd()).resolve() | |
| limited_files = state.files[: self.MAX_FILES] | |
| for file in limited_files: | |
| file_path = pathlib.Path(file) | |
| # Normalize to an absolute path relative to base_dir, if needed | |
| if not file_path.is_absolute(): | |
| file_path = (base_dir / file_path).resolve() | |
| else: | |
| file_path = file_path.resolve() |
| import asyncio | ||
|
|
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The asyncio import should be at the top of the file with other imports, not inside the test function. This follows Python best practices for import organization and makes the dependency explicit.
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.18.5-dev176 |
📝 Summary
Solves some ux issues with
marimo edit subdir/