feat(runtimed): create new notebook when opening non-existent path#685
feat(runtimed): create new notebook when opening non-existent path#685
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the OpenNotebook handshake in the runtimed daemon to treat a non-existent .ipynb path as “create a new notebook here” rather than failing, aligning CLI behavior with typical editors.
Changes:
- Checks file existence before attempting to canonicalize the provided path.
- For missing files, derives
notebook_idfrom an absolute path and initializes the room doc with a new empty notebook. - For permission/IO errors during access checks, returns a structured error response to the client.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
crates/runtimed/src/daemon.rs
Outdated
| crate::notebook_sync_server::create_empty_notebook( | ||
| &mut doc, | ||
| &default_runtime.to_string(), | ||
| default_python_env.clone(), | ||
| Some(¬ebook_id), | ||
| ) | ||
| .map_err(|e| anyhow::anyhow!(e))?; | ||
| info!("[runtimed] Created new notebook at {}", path); |
There was a problem hiding this comment.
If create_empty_notebook fails, this function returns early with an error before sending NotebookConnectionInfo, which may leave the client waiting on a response frame. Consider mirroring the structured error-response pattern used above (and in handle_create_notebook) so OpenNotebook failures on the new-notebook path still return a NotebookConnectionInfo { error: ... } to the client.
| crate::notebook_sync_server::create_empty_notebook( | |
| &mut doc, | |
| &default_runtime.to_string(), | |
| default_python_env.clone(), | |
| Some(¬ebook_id), | |
| ) | |
| .map_err(|e| anyhow::anyhow!(e))?; | |
| info!("[runtimed] Created new notebook at {}", path); | |
| let result = crate::notebook_sync_server::create_empty_notebook( | |
| &mut doc, | |
| &default_runtime.to_string(), | |
| default_python_env.clone(), | |
| Some(¬ebook_id), | |
| ); | |
| match result { | |
| Ok(()) => { | |
| info!("[runtimed] Created new notebook at {}", path); | |
| } | |
| Err(e) => { | |
| error!( | |
| "[runtimed] Failed to create new notebook at {}: {}", | |
| path, e | |
| ); | |
| } | |
| } |
| // Check whether this connection needs to stream-load the notebook | ||
| // from disk. The actual load is deferred to the sync loop so cells | ||
| // arrive progressively via Automerge sync messages. | ||
| let (cell_count, needs_load) = { | ||
| // from disk, or create a new empty notebook. | ||
| let (cell_count, needs_load) = if !file_exists { | ||
| // File doesn't exist - create empty notebook in the doc | ||
| let mut doc = room.doc.write().await; | ||
| if doc.cell_count() == 0 { | ||
| crate::notebook_sync_server::create_empty_notebook( | ||
| &mut doc, | ||
| &default_runtime.to_string(), | ||
| default_python_env.clone(), | ||
| Some(¬ebook_id), | ||
| ) | ||
| .map_err(|e| anyhow::anyhow!(e))?; | ||
| info!("[runtimed] Created new notebook at {}", path); | ||
| } | ||
| (doc.cell_count(), None) // No streaming load needed | ||
| } else { |
There was a problem hiding this comment.
Creating an empty notebook for a non-existent file path still uses a path-based notebook_id (not a UUID). In handle_notebook_sync_connection, auto-launch is gated by room.notebook_path.exists() || is_new_notebook where is_new_notebook only becomes true for UUID notebook_ids; for a newly-created (non-existent) file path this will evaluate false and prevent auto-launch even though the doc now has content. Consider adjusting the auto-launch gating for this case (e.g., treat “created empty notebook” as new, or relax the is_new_notebook check when needs_load is None and the doc was just initialized).
crates/runtimed/src/daemon.rs
Outdated
| send_json_frame(&mut writer, &response).await?; | ||
| // Drain any remaining data from client before closing | ||
| let _ = tokio::io::copy(&mut reader, &mut tokio::io::sink()).await; | ||
| return Ok(()); |
There was a problem hiding this comment.
tokio::io::copy(&mut reader, &mut tokio::io::sink()).await will block until the client closes its write-half; if the client waits for the server to close first, this can hang the task indefinitely. Consider removing the drain or wrapping it in a short timeout / bounded read, and/or calling shutdown() on the writer after sending the error frame.
crates/runtimed/src/daemon.rs
Outdated
| notebook_id: String::new(), | ||
| cell_count: 0, | ||
| needs_trust_approval: false, | ||
| error: Some(format!("Cannot access notebook: {}", e)), |
There was a problem hiding this comment.
The error response sent back to the client omits the notebook path, which makes it harder for users to understand which open failed (especially if multiple windows/requests are involved). Consider including the path in the error string (or a dedicated field, if available) so the client can surface a more actionable message.
| error: Some(format!("Cannot access notebook: {}", e)), | |
| error: Some(format!("Cannot access notebook '{}': {}", path, e)), |
crates/runtimed/src/daemon.rs
Outdated
| path_buf | ||
| .canonicalize() | ||
| .unwrap_or_else(|_| path_buf.clone()) | ||
| .to_string_lossy() | ||
| .to_string() |
There was a problem hiding this comment.
For existing files, canonicalize() failures are still silently swallowed via unwrap_or_else(|_| path_buf.clone()). This can produce a non-canonical notebook_id even when the file exists (e.g., due to permission/symlink issues), undermining the stated goal of returning permission/IO errors to the client and potentially creating duplicate rooms for the same file. Consider handling canonicalize errors explicitly (e.g., treat as an error to the client, or fall back to absolute() only for specific cases).
| path_buf | |
| .canonicalize() | |
| .unwrap_or_else(|_| path_buf.clone()) | |
| .to_string_lossy() | |
| .to_string() | |
| match path_buf.canonicalize() { | |
| Ok(canonical) => canonical.to_string_lossy().to_string(), | |
| Err(e) => { | |
| // Permission denied, I/O error, or other issue while canonicalizing | |
| // Treat this as an error rather than silently falling back | |
| let (mut reader, mut writer) = tokio::io::split(stream); | |
| let response = NotebookConnectionInfo { | |
| protocol: PROTOCOL_V2.to_string(), | |
| protocol_version: Some(PROTOCOL_VERSION), | |
| daemon_version: Some(crate::daemon_version().to_string()), | |
| notebook_id: String::new(), | |
| cell_count: 0, | |
| needs_trust_approval: false, | |
| error: Some(format!("Cannot canonicalize notebook path: {}", e)), | |
| }; | |
| send_json_frame(&mut writer, &response).await?; | |
| // Drain any remaining data from client before closing | |
| let _ = tokio::io::copy(&mut reader, &mut tokio::io::sink()).await; | |
| return Ok(()); | |
| } | |
| } |
a6d3aa2 to
51a6424
Compare
Addressed Copilot Review Comments
|
51a6424 to
8a8fc8d
Compare
Pull request was converted to draft
When `runt notebook newfile.ipynb` is called with a path that doesn't exist, the daemon now creates a new empty notebook at that path instead of failing. This matches the expected behavior of CLI tools like vim. - Check file existence before canonicalizing path - For non-existent files: use absolute path for notebook_id, create empty notebook in doc - For permission/IO errors: return error to client instead of silently failing
8a8fc8d to
6585919
Compare
Summary
When
runt notebook newfile.ipynbis called with a path that doesn't exist, the daemon now creates a new empty notebook at that path instead of failing. This matches the expected behavior of CLI tools like vim.Changes
Verification
cargo xtask dev-daemon./target/debug/runt notebook /tmp/test-new.ipynb/tmp/test-new.ipynbis created on disk./target/debug/runt notebook /root/nopermission.ipynbreturns errorPR submitted by @rgbkrk's agent, Quill