feat(sessions): give a file Lody will not render a way out - #375
Conversation
github-actions
Bot
added
scope: cli
scope: desktop
scope: shared
scope: components
labels
![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: 5d90037348
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| isLocalMachine, | ||
| // Download does not need a resolved host path — it reads through the | ||
| // provider — so it must not be gated on one. | ||
| hasHostPath: localHost !== null, |
There was a problem hiding this comment.
When a local Electron session has no resolvable workspace root—while its Flock metadata is loading or when legacy path metadata is absent—localHost is still non-null, so this reports hasHostPath: true. The menus consequently expose editor/reveal actions that can only call reportOpenFailure, while suppressing the usable download fallback; determine availability from an actually resolved host path instead.
AGENTS.md reference: packages/components/src/components/sessions/AGENTS.md:L839-L850
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3390af7f6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const capacity = Math.max(1, Math.min(maxBytes + 1, stat.size + 1)); | ||
| const buffer = Buffer.alloc(capacity); | ||
| const bytesRead = fs.readSync(fd, buffer, 0, capacity, 0); | ||
| const truncated = bytesRead > maxBytes || stat.size > maxBytes; |
There was a problem hiding this comment.
When a local-project or worktree file is appended after fstatSync, this allocation is only one byte larger than the old size. If the file grows by more than one byte before readSync, the read fills the buffer, but truncated remains false because it compares only against maxBytes and the stale stat.size; the viewer therefore presents an incomplete prefix as the complete file. Detect a full stat-sized buffer and retry/re-stat, or otherwise report the read as truncated.
Useful? React with 👍 / 👎.
The "File is too large to preview" card told the user to open the file on the host machine and then gave them no way to do it. It now carries the actions, and the same set is reachable by right-clicking a file in the Files tree and from a new ... button left of the side panel's +. What each surface may offer is decided once, in useSessionFileActions, because the split is the whole point: copying the path works on every platform and is the whole answer for a file on someone else's machine, reaching a shell (open with the OS, reveal in Finder / File Explorer, open in the configured editor) needs the desktop bridge AND the file's machine to be this one, and downloading is the exact complement of that - offered only where the shell actions are absent, since with the real file one keystroke away a copy in ~/Downloads is a decoy. The absolute path is built only from the owning machine's workspace root plus a genuinely workspace-relative viewer path, so a remote session can never hand this machine's shell a path of its choosing. Also: a local file is no longer refused as "too large". Those budgets (10 MiB text, 1 MiB gzip ceiling) describe the Machine RPC wire, and file/preview-local has no wire to protect, so it now reads to FILE_PREVIEW_V3_LOCAL_LIMITS and ships text uncompressed. The remaining 64 MiB ceiling is about what a viewer can render, not what fits in a reply. The local-project read path also stopped truncating the viewer at its 64 KiB preview default, and no longer allocates the whole ceiling for every read. Model: claude-opus-5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The local preview limit was raised to 64 MiB on the reasoning that a same-machine read crosses no wire. It crosses one: the local IPC client DESTROYS a response body past LOCAL_IPC_MAX_RESPONSE_BODY_BYTES (16 MiB), and the facade reports that as retryable I/O — so a 20 MiB local file would have shown "try again" about a file that will never load, instead of the honest card with its escape hatches. The local limits are now derived from that cap. Binary is base64, a fixed 4/3 expansion, so its raw cap is the budget times 3/4. Text is a JSON string, and escaping is data-dependent — a file of newlines doubles, one of control bytes sextuples — so no raw-size cap can predict it and the service measures the ENCODED payload before answering. Also names the ceiling on the remote Download file action rather than letting it fail generically. It reads through the preview API's one bounded response, so it cannot serve a file past those limits — which is exactly the file whose error card sent the user looking. "Could not download" reads as a glitch worth retrying; it now says the file is too large to download from here and to open it on the machine that owns it. Lifting that needs a ranged or streamed Machine RPC method behind a negotiated protocolCapabilities key, not a client-side change. Model: claude-opus-5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sizing the read buffer from fstat made stat the end condition as well as the hint: a file appended to between the stat and the read fills that buffer exactly, and bytesRead was only ever compared against maxBytes and the stale stat.size. A 5 KiB file whose stat said 100 bytes came back as a 101-byte prefix with truncated: false, so the viewer presented an incomplete file as the whole thing - worse than any size limit, because nothing marked it. The read now loops: a full buffer grows and keeps reading, and only EOF or the budget stops it. truncated is derived from what was actually read rather than from stat.size, so a file that shrank to fit is not reported truncated either, and a short read - which the original single readSync never handled - is safe now too. Model: claude-opus-5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`hasHostPath` was derived from `localHost`, which never required one — so being in the desktop app on the owning machine was enough to offer "Open in <editor>" and "Show in Finder". While that machine's path metadata is still loading, or when it does not resolve at all, both could only fail, and the download that would have worked was hidden behind them (it is their complement). There is now one availability decision, taken from the resolved workspace path, and both halves read it. Model: claude-opus-5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7d5e80e to
1bfec21
Compare
Fourteen of the twenty-seven tests this branch added asserted their own input back. The card test passed `openTarget: 'browser'` and checked that "Open in browser" rendered; the label test checked that a three-branch map returns its three branches; the availability test re-derived a two-line boolean that the hook test already exercises where it actually broke. What is left is what a change to this code should have to survive: the path join refusing anything that could escape the workspace root, the same-machine preview budget and its escaped-payload measurement, the grown-file read, the local-host actions requiring a resolved path, and the download naming its ceiling. Each of those fails against the code as it was before its fix; the deleted ones could not fail at all. `offersFileActions` keeps one pure assertion in the error-state test that already owns that module, rather than a jsdom render of the card. Model: claude-opus-5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19d21355df
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| resolveSessionFileActionAvailability({ | ||
| isElectronRenderer, | ||
| isLocalMachine, | ||
| hasHostPath: workspacePath !== null, | ||
| hasFileProvider: Boolean(fileProvider), |
There was a problem hiding this comment.
For a same-machine Electron session that opens an allowed external file such as agent-produced /tmp output, workspacePath is non-null, so this selects localHost and suppresses download; however, resolveLocalWorkspaceFilePath rejects every absolute file path. The tree and ⋯ menus consequently expose editor/reveal actions whose handlers can only report failure, while an oversized external file's error card offers only Copy Path. Fresh evidence beyond the earlier unresolved-root comment is that this occurs with fully resolved metadata for the explicitly supported external-preview path. Compute the split per file, or retain download whenever that file has no resolvable host target.
AGENTS.md reference: packages/components/AGENTS.md:L172-L177
Useful? React with 👍 / 👎.
| if (snapshot.kind === 'text') { | ||
| downloadBytesAsFile(filePath, new TextEncoder().encode(snapshot.text)); |
There was a problem hiding this comment.
When a remote UTF-8 text file larger than the 64 KiB plain-text threshold starts with a BOM, File Preview uses the gzip path and decodeFilePreviewText decodes it with the default TextDecoder behavior, which strips that BOM. Re-encoding snapshot.text here therefore downloads bytes different from the source file, even though result.entry.hasBom records that the prefix existed. Restore the BOM from that metadata or retain the original bytes for downloads.
Useful? React with 👍 / 👎.
Two conflicts, both additive collisions rather than disagreements: - `combined-mention-textarea.tsx`: #378 replaced Fuse with the vendored VS Code fuzzy scorer and dropped `fuse` from the file source, while this branch added `onActivate: onFilesActivate` to the same object literal. Kept the removal and the new field. - `local-project-control-service.test.ts`: #375 appended a `readProjectFile` describe where this branch had appended the walk and ripgrep describes. Kept all four. The Fuse removal also dated this branch's prose: the entry-identity rule is unchanged, but only `buildMentionFileIndex` is memoised on the entry now, so the comment in `use-local-project-file-paths.ts`, the mentions AGENTS.md paragraph, and the entry-reuse test comment say that instead of naming a Fuse index that no longer exists. Model: claude-opus-5[1m] Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Why
The "File is too large to preview" card told the user to open the file on the host
machine, and then gave them no way to do it. Feedback from 陈加贝: "这里可以直接让我用
finder 打开或者点开浏览器就好了", plus: let me copy the path, and keep straight which
actions only exist on a local Electron machine.
What
One model, three surfaces.
hooks/use-session-file-actions.tsdecides what thisclient may do with a session file; the file-error card, the Files tree's right-click
menu, and a new
⋯button left of the side panel's+all render it. The split is theinvariant:
shell.openPath)One
resolveSessionFileActionAvailabilitycall decides both halves, andhasHostPathis the real resolved workspace path: an Electron renderer on the owning machine still
cannot reach a shell while that machine's path metadata is loading, and in that window
the download fallback must stay offered rather than be hidden behind actions that can
only fail.
The absolute path is resolved on the owning machine (its Flock
dotlodyPath/local-project root) and joined only from that root plus a genuinely workspace-relative
viewer path —
lib/session-local-file-path.tsrejects absolute and..paths, so aremote session can never hand this machine's shell a path of its choosing. New IPC:
app.openLocalPath(absolute + existing →shell.openPath), beside the existingapp.revealLocalPath.A local file is no longer "too large" for a limit that does not apply to it.
FILE_PREVIEW_V3_LIMITS(10 MiB text, 5 MiB binary, 1 MiB gzip ceiling) describes theremote Machine RPC wire.
file/preview-localdoes not cross it, so it passessameMachine: trueand reads toFILE_PREVIEW_V3_LOCAL_LIMITSwith text alwaysutf8-plain. It is still a transport, though, and the local one is the tighterconstraint: its client destroys a response body past
LOCAL_IPC_MAX_RESPONSE_BODY_BYTES(16 MiB), which the facade reports as retryable I/O — "try again" about a file that will
never load. So the local limits are derived from that cap (≈15.94 MiB payload budget;
binary ×3/4 for base64), and the encoded payload is measured before answering,
because JSON escaping is data-dependent and no raw-size cap can predict it (a file of
newlines doubles, one of control bytes sextuples).
Separately, the local-project read path stopped truncating the viewer at its 64 KiB
preview default; its buffer is now sized from
fstatas a hint only, and the readloops to EOF so a file appended to mid-read is not served as a complete prefix.
Download filehas a known ceiling, and says so: it reads through the preview API'sone bounded response, so it cannot serve a file past those limits — exactly the file
whose error card sent the user looking. A generic "could not download" reads as a glitch
worth retrying, so it now names the ceiling. Lifting it needs a ranged/streamed Machine
RPC method behind a negotiated
protocolCapabilitieskey, which is a protocol changerather than a client fix.
Card layout. No status glyph (a 40px icon column indented one short paragraph for
decoration) and the actions stack full-width in one column — buttons sized by their own
labels gave three ragged widths on three lines in a side panel, where the width
difference reads as meaning.
Tests
pnpm typecheck, fullpnpm test:ci,lint,lint:i18nand the three boundary guardspass on the rebased branch. New coverage: the same-machine preview budget and the
escaped-payload measurement (
file-preview-service.test.ts), the grown-file read(
local-project-control-service.test.ts), the availability rule and the download'sceiling reporting (
use-session-file-actions.test.tsx), OS-specific labels(
session-file-actions.test.ts), the workspace-relative path join(
session-local-file-path.test.ts), and which error kinds get the action row(
session-file-error-state-actions.test.tsx). Storybook: three states onCodeCollabFileStatesplusSessionFileActionsMenu.🤖 Generated with Claude Code