[codex] Download files over WebSocket when direct host is unavailable#1863
[codex] Download files over WebSocket when direct host is unavailable#1863kfihihc wants to merge 1 commit into
Conversation
Fall back to transferring file bytes over the WebSocket connection when the daemon has no reachable direct URL (e.g. relay-only connections), with a 50MB cap, progress reporting, and share-sheet integration. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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: b7c27ef7d6
ℹ️ 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".
| ); | ||
| } | ||
|
|
||
| const file = await input.requestFileBytes(input.path); |
There was a problem hiding this comment.
When the active connection has no direct HTTP host, this awaits requestFileBytes, which is wired to client.readFile; that file-explorer request uses the normal correlated session RPC timeout with no override, so it times out after the default 60s. On relay/mobile connections, an otherwise allowed 30–50 MB download can easily take longer than that, causing the new fallback path to fail even though the socket is still healthy and the file is below the cap. Please use a long-running file-transfer request/timeout for this download path instead of the default explorer RPC timeout.
Useful? React with 👍 / 👎.
|
| Filename | Overview |
|---|---|
| packages/app/src/stores/download-store.ts | Core logic for WebSocket fallback download path; adds resolveDownloadConnection, downloadViaFileBytes, triggerBrowserBytesDownload, and shareDownloadedFile. Post-read size cap checks server-reported size rather than actual byte length, and the token request always precedes the WS path even when the token won't be used. |
| packages/app/src/stores/download-store.test.ts | New test file for WS download logic; uses vi.mock, vi.hoisted, vi.stubGlobal, and a polling loop — all banned by the project's test discipline rules. Coverage of the happy paths and size-limit cases is otherwise good. |
| packages/app/src/components/file-explorer-pane.tsx | Threads activeConnectionId (live snapshot vs. preferred fallback) and the new requestFileBytes action into downloadExplorerEntry; integration point changes are minimal and correctly wired. |
| packages/app/src/hooks/use-file-explorer-actions.ts | Adds requestFileBytes callback that delegates to client.readFile; guards on missing workspace root and disconnected client mirror the existing requestFileDownloadToken pattern. |
| docs/architecture.md | Adds a short paragraph explaining why relay connections require the binary WebSocket file-transfer path; accurate and useful. |
| packages/app/src/i18n/resources/en.ts | Adds websocketTooLarge key with {{size}} interpolation across all eight locale files; consistent and complete. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI as FileExplorerPane
participant DS as DownloadStore
participant Token as requestFileDownloadToken
participant WS as requestFileBytes (WebSocket)
participant HTTP as HTTP download (expo-fs)
UI->>DS: "startDownload({ daemonProfile, activeConnectionId, ... })"
DS->>DS: resolveDaemonDownloadTarget(daemonProfile, activeConnectionId)
DS->>Token: requestFileDownloadToken(path)
Token-->>DS: "{ token, fileName, size, mimeType }"
alt baseUrl present (direct TCP active connection)
DS->>HTTP: createDownloadResumable(url, token)
HTTP-->>DS: "{ uri }"
DS->>DS: completeDownload()
else no baseUrl (relay / socket / pipe connection)
DS->>DS: downloadViaFileBytes()
note over DS: check expectedSize <= 50 MB
DS->>WS: requestFileBytes(path)
WS-->>DS: "{ bytes, size, mime }"
note over DS: check bytes.byteLength === file.size
alt isWeb
DS->>DS: triggerBrowserBytesDownload(blob URL)
else native
DS->>DS: write bytes to cache file
DS->>DS: Sharing.shareAsync()
end
DS->>DS: completeDownload()
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant UI as FileExplorerPane
participant DS as DownloadStore
participant Token as requestFileDownloadToken
participant WS as requestFileBytes (WebSocket)
participant HTTP as HTTP download (expo-fs)
UI->>DS: "startDownload({ daemonProfile, activeConnectionId, ... })"
DS->>DS: resolveDaemonDownloadTarget(daemonProfile, activeConnectionId)
DS->>Token: requestFileDownloadToken(path)
Token-->>DS: "{ token, fileName, size, mimeType }"
alt baseUrl present (direct TCP active connection)
DS->>HTTP: createDownloadResumable(url, token)
HTTP-->>DS: "{ uri }"
DS->>DS: completeDownload()
else no baseUrl (relay / socket / pipe connection)
DS->>DS: downloadViaFileBytes()
note over DS: check expectedSize <= 50 MB
DS->>WS: requestFileBytes(path)
WS-->>DS: "{ bytes, size, mime }"
note over DS: check bytes.byteLength === file.size
alt isWeb
DS->>DS: triggerBrowserBytesDownload(blob URL)
else native
DS->>DS: write bytes to cache file
DS->>DS: Sharing.shareAsync()
end
DS->>DS: completeDownload()
end
Reviews (1): Last reviewed commit: "feat(app): download files over WebSocket..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const fsMock = vi.hoisted(() => ({ | ||
| writes: new Map<string, Uint8Array>(), | ||
| pendingWrite: null as Promise<void> | null, | ||
| })); | ||
| const platformMock = vi.hoisted(() => ({ | ||
| isWeb: false, | ||
| })); | ||
| const legacyFileSystemMock = vi.hoisted(() => ({ | ||
| createDownloadResumable: vi.fn(() => ({ | ||
| downloadAsync: vi.fn().mockResolvedValue({ uri: "file:///cache/report.txt" }), | ||
| })), | ||
| })); | ||
|
|
||
| vi.mock("@/constants/platform", () => ({ | ||
| get isWeb() { | ||
| return platformMock.isWeb; | ||
| }, | ||
| })); | ||
|
|
||
| vi.mock("expo-file-system", () => { | ||
| class File { | ||
| readonly uri: string; | ||
|
|
||
| constructor(directory: string, name?: string) { | ||
| this.uri = name ? `${directory.replace(/\/$/, "")}/${name}` : directory; | ||
| } | ||
|
|
||
| get exists(): boolean { | ||
| return fsMock.writes.has(this.uri); | ||
| } | ||
|
|
||
| write(bytes: Uint8Array): void | Promise<void> { | ||
| fsMock.writes.set(this.uri, bytes); | ||
| return fsMock.pendingWrite ?? undefined; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| File, | ||
| Paths: { | ||
| cache: "file:///cache", | ||
| document: "file:///document", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| vi.mock("expo-file-system/legacy", () => ({ | ||
| createDownloadResumable: legacyFileSystemMock.createDownloadResumable, | ||
| })); | ||
|
|
||
| vi.mock("expo-sharing", () => ({ | ||
| isAvailableAsync: vi.fn().mockResolvedValue(false), | ||
| shareAsync: vi.fn(), | ||
| })); |
There was a problem hiding this comment.
vi.hoisted, vi.mock, vi.stubGlobal, and polling loop
The test file relies on four patterns that the project's review standards explicitly ban:
vi.hoisted(lines 5–16) — banned by policy.vi.mock(lines 18, 24, 51, 55) — module-level mocking is banned; a port/adapter approach or a real in-memory substitute is preferred.vi.stubGlobal("URL", ...)andvi.stubGlobal("document", ...)(lines 343–344) — monkey-patching globals is banned.- The polling loop on line 305 (
for (let index = 0; index < 5 && !fsMock.writes.has(...)...)) — polling inside a test body is explicitly prohibited.
The expo-file-system and expo-sharing dependencies are "local-substitutable" (the test already constructs an in-memory File class and write map); the right approach is to wire that substitution through an injected interface rather than replacing the module at import time. The document/URL browser globals are harder — either extract triggerBrowserBytesDownload behind a port the test can inject, or make the test an integration test against the real DOM.
Rule Used: # Code Review Pattern Reference: Slop, Tests, Feat... (source)
| const file = await input.requestFileBytes(input.path); | ||
| if (file.size > MAX_WEBSOCKET_DOWNLOAD_BYTES) { | ||
| throw new Error( | ||
| i18n.t("downloads.websocketTooLarge", { | ||
| size: formatDownloadSize(MAX_WEBSOCKET_DOWNLOAD_BYTES), | ||
| }), | ||
| ); | ||
| } | ||
| if (file.bytes.byteLength !== file.size) { |
There was a problem hiding this comment.
file.size, not actual bytes
file.size comes from the daemon's WS response and can disagree with file.bytes.byteLength. If the daemon sends 60 MB of bytes but reports size: 3, this guard passes and the bytes are already resident in memory. The effective memory cap is only the pre-read expectedSize check (which is skipped when the token response omits a size). Guarding against file.bytes.byteLength catches the actual allocation regardless of what the server claims.
| const file = await input.requestFileBytes(input.path); | |
| if (file.size > MAX_WEBSOCKET_DOWNLOAD_BYTES) { | |
| throw new Error( | |
| i18n.t("downloads.websocketTooLarge", { | |
| size: formatDownloadSize(MAX_WEBSOCKET_DOWNLOAD_BYTES), | |
| }), | |
| ); | |
| } | |
| if (file.bytes.byteLength !== file.size) { | |
| const file = await input.requestFileBytes(input.path); | |
| if (file.bytes.byteLength > MAX_WEBSOCKET_DOWNLOAD_BYTES) { | |
| throw new Error( | |
| i18n.t("downloads.websocketTooLarge", { | |
| size: formatDownloadSize(MAX_WEBSOCKET_DOWNLOAD_BYTES), | |
| }), | |
| ); | |
| } | |
| if (file.bytes.byteLength !== file.size) { |
| try { | ||
| const downloadTarget = resolveDaemonDownloadTarget(daemonProfile, activeConnectionId); | ||
| const tokenResponse = await requestFileDownloadToken(path); | ||
| if (tokenResponse.error || !tokenResponse.token) { | ||
| throw new Error(tokenResponse.error ?? i18n.t("downloads.requestTokenFailed")); | ||
| } | ||
|
|
||
| const downloadTarget = resolveDaemonDownloadTarget(daemonProfile); | ||
| const resolvedFileName = tokenResponse.fileName ?? fileName; | ||
| const expectedSize = tokenResponse.size; | ||
|
|
||
| if (!downloadTarget.baseUrl) { | ||
| throw new Error(i18n.t("downloads.hostUnavailable")); | ||
| await downloadViaFileBytes({ | ||
| id, | ||
| path, | ||
| fileName: resolvedFileName, | ||
| expectedSize, | ||
| requestFileBytes, | ||
| completeDownload: get().completeDownload, | ||
| updateProgress: get().updateProgress, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
requestFileDownloadToken is called unconditionally, even when downloadTarget.baseUrl is null and the code will take the WS fallback. The token is used only to populate resolvedFileName and expectedSize — not for authorization on the WS call. If the token endpoint is unavailable (e.g., the daemon's HTTP API is wholly unreachable, not just the download host), this will fail with a misleading "request token failed" error instead of falling back cleanly to WS. Consider whether requestFileBytes should carry filename and size information directly, or whether the token request should be made lazily only when the direct path is taken.
1 participant
Linked issue
Related to #1350. No dedicated open issue was found for the relay-only/direct-host-unavailable fallback path.
Type of change
What does this PR do
File downloads currently depend on a direct HTTP download host. That breaks for relay-only or otherwise non-direct connections because the relay only carries encrypted WebSocket traffic, not
/api/files/downloadHTTP requests.This PR keeps the existing direct token download path when the active connection is direct TCP, and adds a WebSocket byte-transfer fallback when there is no direct HTTP download host. The fallback:
How did you verify it
Ran the required local checks from the PR branch:
npm run formatnpm run typechecknpm run lintnpm run test --workspace=@getpaseo/app -- src/stores/download-store.test.ts --bail=1The targeted app test file passed 8 tests covering:
I also checked the Git scope:
git log --oneline --left-right --cherry-pick origin/main...codex/websocket-file-downloadconfirms the branch contains one commit on top oforigin/main.git diff --stat origin/main..codex/websocket-file-downloadconfirms the PR scope is the download fallback change.Checklist
npm run typecheckpassesnpm run lintpassesnpm run formatran (Biome)