Repository navigation
feat(files): edit a text file from the file viewer, opt-in (#771) - #3064
Merged
Merged
Conversation
Settings > Chat > File viewer > Edit text files (off by default) adds Edit to the viewer for a text file in the pane's folder. Save asks once more and is refused when the file changed since it was opened. GET/PUT /api/fs/text (server/file-edit.ts): only an existing UTF-8 file under the pane folder's real path (never .git, never a filesystem root), owned by this user, with one link and one kind of line ending, up to 1 MiB; written to a temp file beside it with its mode and group and renamed over it. On Linux every step after the proof goes through an O_DIRECTORY|O_NOFOLLOW descriptor of its folder (/proc/self/fd/N), on macOS the owned-folders proof runs again right before the rename; other platforms answer 501. The SHA-256 revision is checked before the temp file is written and again right before the rename. Only a device that drives the PC (not watch, Portal or an open LAN window); the PC proxy forwards fs/text and the other PC applies the same rules.
# Conflicts: # src/lib/settings.legacy-fixture.ts
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 8 minutes. View limit details
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. |
…hread (#771) Review round on the text file save: - A draft typed and not saved is kept above the viewer (by PC and file), so Close, Back, moving the viewer beside the pane or opening another file no longer loses it; reopening the file opens its editor with it. Only Save and Cancel drop it, and a reload or closed tab asks first (beforeunload). - The new file gets the original's extended attributes and access list (file-attributes.ts, through libc: Linux posix ACLs are an attribute, macOS ACLs via acl_get_fd_np); a save whose access list cannot be kept is refused (403 access_list) instead of widening who may read the file. - The mode is set after the write, which cleared set-id bits given before it. - Reads and saves run on a file thread (file-pool.ts text-read/text-save, the long deadline), one save at a time, so a slow disk no longer holds the server's thread. - The temp file has a short name of its own, so a long file name saves. - A file or folder removed during a save is a 409, not a 500.
Owner
Author
Review round 1 (omo BLOCK, Codex Terra xhigh) at c18f2f5, and what changedFixed in fa71d86:
Not changed, with the reason:
|
Re-review of the text file save: - The new file loses any attribute it took from its folder that the file did not have, so a folder's default access list no longer opens a saved file to more users (Linux fremovexattr; macOS an empty ACL when the file had none). - A failed attribute or access list read is no longer taken for "none": only ENOTSUP/EOPNOTSUPP (and ENOENT for a macOS ACL) mean none; otherwise the save is refused (errno read through libc). - One save at a time across every file thread, for as long as it really runs: a lock in shared memory (withSaveLock) replaces the request queue, which let the next save in when an earlier one only passed its deadline. - A save to a file removed since it was opened is a 409 conflict, not a 404. - Editor: while any draft is kept the page asks before a reload, not only while its editor is open; a save answering after its editor was closed removes only the draft it wrote.
Owner
Author
Review round 2 (omo BLOCK and Codex Terra xhigh at 1af02b2), fixed in e1bf1a1
Left, with the reason:
|
…ft (#771) Third review of the text file save: - A save runs on the server's own thread again, synchronously; reads stay on a file thread. The cross-thread lock of the last round let a save that waited past its deadline write after its 503, held the lock for good when a worker died, and filled the file threads with waiting saves. Without it two saves never interleave and no save outlives its answer. A save is at most 1 MiB to the disk the file was just read from. - A save answering late removes only the draft entry it submitted, so an editor opened again meanwhile keeps its draft (and the reload prompt). - The Linux-specific save tests run on Linux only.
Owner
Author
Review round 3 (omo CLEAR and Codex Terra xhigh at 0cbf95b), fixed in 593662aCodex reproduced problems in the cross-thread save lock that round 2 added, so this round removes it
The trade-off: a save is at most 1 MiB written to the disk the file was just read from, and a disk Left, with the reason:
|
Owner
Author
Final review at 95ef324: omo CLEAR (with Codex Terra)No HIGH. What remains, none reproduced and none blocking:
CodeRabbit did not review this PR (its review limit was reached when the PR opened). The reviews |
Owner
Author
|
Merge note: all GitHub checks green at this head; omo CLEAR at this exact head; Codex Terra xhigh adversarial design review before code, then three omo + Codex rounds (findings and fixes in the PR comments; remaining notes unreproduced MEDIUM/LOW only); threads resolved; CodeRabbit was rate-limited, so the omo + Codex reviews stand in. First file write route: opt-in and off by default; drive role only (not watch, Portal or the open LAN listener); the pane folder's real path only (no .git, no filesystem root); UTF-8 text up to 1 MiB, single-link files the user owns; temp + rename keeping mode, group, xattrs and ACL (refused if an ACL cannot be kept); the Linux rename goes through an O_DIRECTORY|O_NOFOLLOW folder descriptor proven inside the pane folder; Windows refused in this version. Fixes #771. |
This was referenced Nov 5, 2026
devswha
added a commit
that referenced
this pull request
Nov 5, 2026
) (#3073) ## What changes Part of #1196. The first slice of the Markdown documents panel: **list, search and open**. The safe write route of #771 (#3064) merged while this was in progress, and a document opened from the new tab opens in that same file viewer: with **Settings > Edit text files** on, it can be edited and saved there, with #3064's revision check. Creating and deleting a document are not in this PR (follow-ups on #1196). - **Settings > Chat > Documents in Files** (a device setting, **off by default**). Off, Files is exactly as it is today: three tabs, no new request. - On, Files gets a fourth tab, **Documents**: the Markdown files (`.md`, `.markdown`) under the pane's folder, **newest first**, each with the folder it is in and when it changed. A **Filter documents** field narrows them by path (Escape clears it before it closes Files), a Refresh button reads the list again (nothing polls), and a row opens the file in the existing file viewer, which renders it with its outline. It works in the dialog, in Files beside the pane, and on a phone's sheet. - **Server:** new read-only route `GET /api/pane/docs?pane_id=` -> `PaneDocuments` (`shared/files.ts`). In a repository it asks git afresh (`git ls-files -co --exclude-standard`, the same safe invocation as @-mentions), so ignored build output and `node_modules` stay out and Refresh sees a file made a moment ago. Outside a repository, it walks with the same caps but counts only Markdown files, so a folder of other files cannot crowd a note out. Every file found is statted and the newest 500 kept. The reads run on a file thread (a new `documents` task), stop after **2 s** (a fixed bound), and count as a **scan** in the file pool, so at most two threads run them and file routes keep the rest. The request's abort reaches the task. `truncated` says when a cap or the time limit left files out; the tab then says "Some Markdown files here are not listed." and never claims the newest are shown. - **Access:** the route is gated like `/api/pane/files`. It is refused (403 `read_only`) to a watching device, on this PC and through another PC's route. It is forwarded to other PCs (`MACHINE_PROXY_PATH`). On a herdr-core machine's own server, whose files are on another machine, it answers 501 `unsupported`. - **Phone layout:** four tabs do not fit beside the title and Close at 390px (66px over), or in the narrowest docked panel (320px). There, and only while the fourth tab is shown, the tabs take the row under the title. With three tabs (the setting off), the header is unchanged. - An older PC that does not know the route gets its own message ("This PC's bridge cannot list documents yet."). No new control is visible by default: the tab exists only behind its setting, and the composer is not touched. Design reference: Paseo's `paseo-markdown-viewer` plugin (a read-only Markdown list beside the agent that opens each file in a reader) and the Codex app's side panel, which opens a `.md` file next to Review. This slice follows Paseo's read-only list, inside Files, and leaves editing to #771. @vibewriter-hand offered to build the editing slice on #771's write API: this PR adds no editor and no write route, so create, save-a-new-note and delete remain open for that slice. ## Screenshots Demo fixtures, setting on (`scripts/files-documents-demo-regression.ts`). | | Light | Dark | |---|---|---| | 1280, dialog | <img src="https://raw.githubusercontent.com/devswha/ovis/595c88a192163359a025cfb50f1e31d3a3b8e1da/files-documents-1280-light.png" width="320"> | <img src="https://raw.githubusercontent.com/devswha/ovis/595c88a192163359a025cfb50f1e31d3a3b8e1da/files-documents-1280-dark.png" width="320"> | | 1280, beside the pane at its narrowest (320px) | <img src="https://raw.githubusercontent.com/devswha/ovis/595c88a192163359a025cfb50f1e31d3a3b8e1da/files-documents-panel-1280-light.png" width="320"> | <img src="https://raw.githubusercontent.com/devswha/ovis/595c88a192163359a025cfb50f1e31d3a3b8e1da/files-documents-panel-1280-dark.png" width="320"> | | 390, sheet (tabs on their own row) | <img src="https://raw.githubusercontent.com/devswha/ovis/595c88a192163359a025cfb50f1e31d3a3b8e1da/files-documents-390-light.png" width="320"> | <img src="https://raw.githubusercontent.com/devswha/ovis/595c88a192163359a025cfb50f1e31d3a3b8e1da/files-documents-390-dark.png" width="320"> | | 1280, a row opened beside the pane | <img src="https://raw.githubusercontent.com/devswha/ovis/595c88a192163359a025cfb50f1e31d3a3b8e1da/files-documents-file-panel-1280-light.png" width="320"> | <img src="https://raw.githubusercontent.com/devswha/ovis/595c88a192163359a025cfb50f1e31d3a3b8e1da/files-documents-file-panel-1280-dark.png" width="320"> | | 390, the setting (off by default) | <img src="https://raw.githubusercontent.com/devswha/ovis/595c88a192163359a025cfb50f1e31d3a3b8e1da/files-documents-setting-390-light.png" width="320"> | <img src="https://raw.githubusercontent.com/devswha/ovis/595c88a192163359a025cfb50f1e31d3a3b8e1da/files-documents-setting-390-dark.png" width="320"> | ## Validation - Failing first: the new rows in `server/device-access.contract.test.ts` (a watching device refused on this PC, its alias and another PC's route; a driving one answered), `server/machine-proxy.test.ts` (never forwarded for a watching device), `server/hc-machines.test.ts` (501 on a herdr-core machine's own server) and the `paneDocuments` cases in `server/files.test.ts` (newest first with sizes and times, skipped folders and git-ignored files left out, the cap and `truncated`) failed on the base (2d8581e) and pass now. New `src/components/MarkdownDocuments.test.tsx` (server order, open by absolute path, filter, an older PC's 404 told apart from a missing pane) and the `filesDocuments` rows in `src/lib/settings.test.ts` (off by default, kept in its own key). - New `scripts/files-documents-demo-regression.ts` (added to `scripts/ci-browser.sh`): off by default no Documents tab; on, the list in order, the folder named, no sideways scroll, the filter and Escape, a row opening the viewer; in the dialog, beside the pane at 320px and on a 390px sheet, where it checks that the four tabs and Close fit; light and dark. - Review round (7ff0796): `server/files.test.ts` gained a repository of 2,000 old notes plus a newer `zz-newest.md` (listed first), a non-git folder of 1,000 `.txt` files plus `z-note.md` (listed, not cut) and the 2 s budget (a zero budget answers cut); `server/file-pool.test.ts` gained "a Markdown listing counts as a scan". The three behaviour tests failed on 9456ed6 and pass now. - Second round (c94745f): `server/file-pool.test.ts` "runs one Markdown listing at a time, and a stuck one leaves commands scans both their threads" failed on b8d4fa7 and passes now. - `bun run check fast`: passed at c94745f (origin/main merged): workflow syntax, generated files, typecheck, build, unit tests 12420 pass, 0 fail. - Reviews: omo CLEAR at c94745f; Codex Terra (xhigh) CLEAR at c94745f after two fix rounds (findings and resolutions in the PR comments). - Browser regressions for the touched shared components (Files dialog and panel, file viewer, Settings) in the self-hosted CI runner image (`herdr-ci-runner:local`, `scripts/ci-browser-parallel.ts`, after `bun run build`, inside `bun run check run`), 12/12 passed at 9456ed6 (files-documents again at 7ff0796, 4/4 locally): files-documents, files-panel, files-prompt, file-viewer, file-viewer-mobile, deep-link, diff-find, diff-line-notes, palette-local-page, settings-pages, file-text-size, text-file-edit. - Skipped locally: `bun run check integration` and the full browser lane (CI runs them).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change
Fixes #771. Settings > Chat > File viewer > Edit text files (a device setting, off by default)
adds Edit file to the file viewer for a text file in the pane's folder. Off, the viewer is
exactly as it is today. This is the first route that writes a file, so the server side is narrow
and was design-reviewed (Codex, adversarial) before any code.
TextFileEditor.tsx): the file's whole text in a plain mono text box under one bar.Save asks once more in the same bar ("Save changes to NAME?", Keep editing / Save); Cancel
leaves without writing. While the text differs from the file, Escape, a press on the scrim and the
sheet swipe do not close the viewer. HTML and SVG are only ever text in the box, never rendered.
A file that changed meanwhile follows Zed's "changed on disk" handling: the typed text stays in
the box to copy and Reload file shows the file as it is now, but there is no Overwrite, so a
save never replaces a change the editor has not seen. A refusal says why (outside the pane's
folder, too large, not UTF-8, mixed line endings, read-only, another PC that can't save yet).
shared/files.ts,server/file-edit.ts):GET /api/fs/text?pane_id=&path=answers{ path, text, revision, line_endings, size, modified }, withrevisionthe SHA-256 of thebytes;
PUT /api/fs/text(application/jsononly) takes{ pane_id, path, revision, text }andanswers the new revision, or 409
stale_revisionwith the file's current revision and nothingwritten.
textEditAllowed, checked indispatch.tsbefore the route and before the PCproxy): a device that drives this PC (paired device, token, this PC's own browser, a relay phone
with drive). Never a watching device, a Portal visitor (even one given drive) or an open LAN
window that signed in as nobody. On another PC, that PC's own bridge token is accepted for its
own route only, never for a PC behind it. Mutations keep the existing same-origin gate, and a
text/plainor form body is refused before it is read.folder's real path, never in
.gitand never when the pane's folder is a filesystem root; ownedby this server's user, with one link, at most 1 MiB, UTF-8 without NUL (the whole file is checked,
not a sniffed prefix) and one kind of line ending (CRLF is given back as LF and written back as
CRLF; a BOM round-trips). The request body is read as a stream capped at 8 MiB, and the text must
have no CR, NUL or unpaired surrogate.
.NAME.ovis-<hex>.tmpbeside the file (O_EXCL|O_NOFOLLOW), giventhe file's mode and group (
fchmod,fchown; a failure refuses the save), fsynced and renamedover it, so a reader sees the old text or the new. The original must open for writing first, so a
read-only file is refused rather than replaced. The revision is checked before the temp file is
written and again right before the rename.
through an
O_DIRECTORY|O_NOFOLLOWdescriptor of its folder (/proc/self/fd/N/<name>), whosekernel path must lie in the pane's folder right before the rename. A folder on the way swapped for
a link cannot send the rename anywhere else, and a folder moved out of the pane's folder gets
nothing. On macOS the existing
ownedFoldersproof (every folder from the pane's folder down isthis user's own and writable by nobody else) runs again right before the rename, as for rendered
pages' media. Windows and other platforms answer 501
unsupported_platformfor now.fs/textjoins the PC proxy's routes. This server checks the caller's role,and the other PC applies the same file rules itself.
What is not guaranteed: another program writing the file in the moment between the last check and
the rename is not detected (no lock), the same as editors that do not lock a file. Node has no
renameat, which is why the Linux path goes through/proc/self/fd.Design reference: Zed's handling of a buffer whose file changed on disk (keep the buffer, offer a
reload), without its Overwrite. The Codex and Claude apps, Paseo and T3 Code do not edit files in
their viewers.
Design review (before code)
Codex (Terra, xhigh) attacked the first design. Each finding and what changed:
shared/files.ts); a second hash check runs right before the rename.git.gitanywhere refused (case-folded on macOS/Windows); containment byrelative()Screenshots
Owned herdr pane, real server. Editing with changes, the save confirm, a conflict, a read-only
file, and the setting (off by default).
Validation
server/file-edit.test.ts(21 tests): open and save keeping mode with nothing left besideit; CRLF and BOM round trip; stale revision refused with nothing written; a write by another
program between the checks refused;
../, absolute, linked file, linked parent folder,.gitand a filesystem-root pane all refused for GET and PUT; a folder swapped for a link right before
the rename (
textSaveHooks.beforeRename) cannot send the save outside, and a folder moved outgets nothing; binary past the sniffed prefix, non-UTF-8, mixed and lone-CR endings, oversized,
read-only and hard-linked files refused; content type, other origin, CR, lone surrogate, bad
revision, 8 MiB body, missing pane, method; 501 off Linux/macOS; relay watch refused, relay drive
served, open LAN window refused; the role table. Each guard was removed once to see its test go
red (re-check before rename, folder anchor, Portal rule, containment check), then restored.
server/machine-proxy.test.ts: a save is forwarded with its body, content type and the PC'scredential, the PC's 409 comes back, and a watching device is refused before the PC is asked.
src/components/TextFileEditor.test.tsx: nothing is sent until the second Save, which namesthe revision opened; Cancel sends nothing; a conflict keeps the typed text and Reload file shows
the PC's; a refusal shows its reason and no text box.
scripts/text-file-edit-regression.ts(added toscripts/ci-browser.sh): at 1280 and 390,light and dark, on an owned herdr pane: off by default (no Edit; the Settings switch off); on,
Escape keeps unsaved changes, the first Save writes nothing and the second writes the file; Cancel
writes nothing; a file changed on the PC is not overwritten and Reload file shows it; a read-only
file says it can't be written. 16/16 PASS locally.
bun run check fast: passed at c18f2f5 (origin/main 86b7951 merged in; workflow syntax, generated files, typecheck, build, unit tests: 12348 pass, 0 fail).herdr-ci-runner:local,scripts/ci-browser-parallel.ts, 4 workers): 8/8 passed at baa07f1 (before that merge, whichtouched only
settings.legacy-fixture.ts): text-file-edit, file-viewer, file-viewer-mobile,files-panel-demo, file-text-size-demo, files-prompt, deep-link-demo, settings-pages-demo.
bun run check integrationand the full browser lane (CI runs them); noWindows or macOS run (Windows answers 501 by design; the macOS branch reuses
ownedFolders).