Allow editing files from Browse Files - #127
Conversation
Co-authored-by: Codex <codex@openai.com>
📝 WalkthroughWalkthrough
ChangesEditable File Preview
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant XdFilePane
participant RemoteServer
participant FileSystem
User->>XdFilePane: edit preview and activate Save
XdFilePane->>RemoteServer: send file-browse write with content
RemoteServer->>FileSystem: validate regular file and write content
FileSystem-->>RemoteServer: return write result
RemoteServer-->>XdFilePane: return success or error
XdFilePane-->>User: update editor state and show toast
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f24b1a06ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (gtk_text_buffer_get_modified (self->preview)) | ||
| { | ||
| show_toast (self, "Save or undo changes before reloading."); | ||
| return; |
There was a problem hiding this comment.
When the user switches chats with unsaved edits, xd_file_pane_set_workdir() or xd_file_pane_set_remote() updates the workspace/chat before calling this refresh; this early return then leaves the old buffer and file_path visible. Pressing Save afterward writes that old content to the same relative path in the newly selected local workspace or remote chat, potentially overwriting an unrelated file. Context changes must either preserve the original save target or explicitly resolve/discard the dirty buffer before changing it.
Useful? React with 👍 / 👎.
| send_error (connection, "Only regular files can be edited."); | ||
| return; | ||
| } | ||
| if (!g_file_set_contents (path, content, (gssize) length, &error)) |
There was a problem hiding this comment.
When a remotely browsed executable text file is saved, g_file_set_contents() replaces it using ordinary creation permissions, dropping executable bits that the local g_file_replace_contents_async() path preserves. It also replaces a browsed symlink itself rather than updating its target, so a routine remote edit can silently break scripts or alter repository structure; use the equivalent GFile replacement API or explicitly preserve the original file type and mode.
Useful? React with 👍 / 👎.
| call_remote (self, "write", request->path, request->content, | ||
| on_remote_file_saved, request); |
There was a problem hiding this comment.
For a remote edit near the allowed 1 MB limit, this passes the entire buffer—potentially larger after JSON escaping—to xd_remote_client_call_async(), but send_call() actually performs blocking g_output_stream_write_all() calls on the GTK thread in src/remote/client.c:201-202. On a slow or congested remote connection, clicking Save can therefore freeze the whole window until the request is accepted; the large request needs to be written asynchronously.
Useful? React with 👍 / 👎.
| g_autoptr (GFile) file = g_file_new_for_path (full); | ||
|
|
||
| g_file_replace_contents_async ( | ||
| file, request->content, strlen (request->content), NULL, FALSE, |
There was a problem hiding this comment.
If an agent, terminal, or another editor changes or deletes this local file after the pane reads it, passing a null entity tag makes g_file_replace_contents_async() blindly replace the latest version—or recreate a deleted file—with the stale editor snapshot. That silently discards concurrent workspace changes; retain the entity tag from the read and reject the save with a conflict when it no longer matches, with equivalent version checking in the remote path.
Useful? React with 👍 / 👎.
| if (gtk_text_buffer_get_modified (self->preview)) | ||
| { | ||
| show_toast (self, "Save or undo changes before going back."); | ||
| return; |
There was a problem hiding this comment.
After any accidental edit, Back and Reload only show this toast and there is no discard action. In particular, the project permits GTK 4.10–4.14, where GtkTextBuffer has no built-in undo support, so the suggested recovery is unavailable and the user must save unwanted content or abandon the pane/application; offer an explicit discard confirmation instead of permanently refusing navigation.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test-remote.c (1)
1878-1905: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding negative-path coverage for the new write validations.
The happy-path write is correctly tested, but
handle_file_browse's new "write" branch (server.c) also added rejection paths — missingcontent, content overFILE_PREVIEW_LIMIT, and non-regular targets — none of which are exercised here. Worth a follow-up test asserting eachsend_errorpath is reached.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test-remote.c` around lines 1878 - 1905, Extend the remote file-browse tests around the existing write request to cover handle_file_browse rejection cases: omit content, provide content exceeding FILE_PREVIEW_LIMIT, and target a non-regular file. Assert each request produces the expected send_error response, while preserving the existing successful write coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chat/file-pane.c`:
- Around line 717-724: The navigation and reload guards incorrectly promise that
undo clears the modified state, but GTK undo does not reset it. Update both the
back-navigation guard around gtk_text_buffer_get_modified and
xd_file_pane_refresh’s reload guard to remove the “undo” wording or implement a
discard action that restores the original text and calls
gtk_text_buffer_set_modified(..., FALSE); apply the same behavior at both
affected sites.
---
Nitpick comments:
In `@tests/test-remote.c`:
- Around line 1878-1905: Extend the remote file-browse tests around the existing
write request to cover handle_file_browse rejection cases: omit content, provide
content exceeding FILE_PREVIEW_LIMIT, and target a non-regular file. Assert each
request produces the expected send_error response, while preserving the existing
successful write coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1290d9a8-a93f-4a17-b28d-20278b4d4a09
📒 Files selected for processing (3)
src/chat/file-pane.csrc/remote/server.ctests/test-remote.c
…le-section-allow-to-also-edit-t-95cc8e52 # Conflicts: # src/chat/file-pane.c
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 954ac46510
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| send_error (connection, "Only regular files can be edited."); | ||
| return; | ||
| } | ||
| if (!g_file_set_contents (path, content, (gssize) length, &error)) |
There was a problem hiding this comment.
When the remote workspace contains a symlinked directory, saving a path such as link/file can overwrite a file outside the workspace: browse_path() only lexically canonicalizes the path and verifies its string prefix, while this write follows symlinks in parent components. The new write operation therefore violates the checkout confinement described by browse_path(); resolve the final parent path and verify it remains beneath the resolved workdir before writing.
Useful? React with 👍 / 👎.
| self->showing_preview = TRUE; | ||
| set_header_path (self, path); | ||
| gtk_stack_set_visible_child_name (self->stack, "preview"); | ||
| gtk_widget_grab_focus (GTK_WIDGET (self->editor)); |
There was a problem hiding this comment.
When a local or remote file read is slow and the user moves focus to the composer before it completes, this unconditional focus grab moves subsequent keystrokes into the file editor as soon as the callback arrives. The same can happen after an automatic remote reconnect refresh, so text intended for a message may instead dirty the opened file; only grab focus as part of the initiating navigation event, or preserve the user's current focus when the asynchronous read completes.
Useful? React with 👍 / 👎.
…le-section-allow-to-also-edit-t-95cc8e52
…le-section-allow-to-also-edit-t-95cc8e52
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/remote/server.c (1)
1790-1792: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a path-component boundary when abbreviating
$HOME.A string prefix misrenders sibling paths such as
/home/alice-backupwhen$HOMEis/home/alice, producing~-backup. Only abbreviate when the path equals$HOMEor the next character is a directory separator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/remote/server.c` around lines 1790 - 1792, Update the home-directory abbreviation logic around shown so it only matches when workdir equals g_get_home_dir() or the character immediately after the home prefix is a directory separator; otherwise retain g_strdup(workdir), preventing sibling paths from becoming misleading "~" paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/remote/server.c`:
- Around line 1790-1792: Update the home-directory abbreviation logic around
shown so it only matches when workdir equals g_get_home_dir() or the character
immediately after the home prefix is a directory separator; otherwise retain
g_strdup(workdir), preventing sibling paths from becoming misleading "~" paths.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1995fd7e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| gtk_text_view_set_editable (GTK_TEXT_VIEW (preview_view), FALSE); | ||
| gtk_text_view_set_cursor_visible (GTK_TEXT_VIEW (preview_view), FALSE); | ||
| self->editor = GTK_TEXT_VIEW (preview_view); | ||
| gtk_text_view_set_editable (self->editor, TRUE); |
There was a problem hiding this comment.
When the user closes the window or invokes Ctrl+Q while this newly editable buffer is modified, the changes are discarded without warning: src/xd-window.c:on_close_request() always propagates the close, src/xd-app.c:on_quit_action() quits unconditionally, and the file pane exposes no dirty-state check to either path. Integrate the modified state into application shutdown so accidental closure does not silently lose edits.
Useful? React with 👍 / 👎.
| g_file_replace_contents_async ( | ||
| file, request->content, strlen (request->content), NULL, FALSE, | ||
| G_FILE_CREATE_NONE, self->cancellable, on_local_file_saved, request); |
There was a problem hiding this comment.
On a filesystem where g_input_stream_read_bytes_async() returns a short successful read, on_local_file_read() treats that single chunk as the entire file and exposes only the prefix in the editor. Saving any edit through this new replacement call then overwrites the original file with that prefix, silently truncating the unread remainder; use an all-bytes read or continue reading until EOF/the size limit before making the buffer saveable.
Useful? React with 👍 / 👎.
1 participant
Summary
Testing
docker build --target test --progress plain .Summary by CodeRabbit
New Features
Bug Fixes
Tests