feat(chat): upload files from device alongside server attachments - #729
pascalandr wants to merge 2 commits into
Conversation
Add an Upload from device action beside the existing server-side file browser in every runtime context. Use the native file input and existing byte-backed prompt attachments so local files also work with remote servers and WSL without relying on device-local paths. Retain the 5 MB per-file limit, report oversized and unreadable selections, infer missing MIME types, and prevent submission while file reads are pending. Fence late reads against composer disposal and session changes. Translate the new labels in all ten locales. Validate the real SessionView and native client prompt serialization with nine Chromium scenarios across web and desktop runtime contexts, including multiple files, cancellation, reselection, server browsing, read failures and session changes. All 62 targeted attachment/persistence/submission tests, UI typecheck and diff checks pass. Closes #725.
There was a problem hiding this comment.
Gatekeeper verdict: BLOCK — not merge-ready
Issue relevance
#725 is valid and worth solving. The existing filesystem browser resolves on the CodeNomad server, so it cannot reach device-local files when the renderer and server are separated by a remote host, WSL, or a container. Keeping the server browser and adding a device-byte flow is the correct product model. A standard <input type="file"> is also preferable to separate Electron/Tauri adapters, and reusing byte-backed OpenCode prompt files avoids temporary-file lifecycle and path-translation problems.
Blocking regressions
-
The real transport rejects files far below the advertised 5 MiB limit. Production clients send prompts through
/workspaces/:id/instance/..., whose Fastify JSON parser retains the default ~1 MiB body limit. Base64 adds roughly 33% plus JSON overhead. A minimal reproduction using the exact parser configuration accepts a 700 KiB raw file but rejects an 800 KiB raw file with413 FST_ERR_CTP_BODY_TOO_LARGE. The desktop remote-proxy server independently uses the same default limit with its raw-buffer parser, so remote desktop requests can fail at two hops. Multiple smaller files fail on their combined encoded size as well. -
The selection has no aggregate count or byte budget.
multiplepermits an arbitrary number of individually valid 5 MiB files. Every file is read concurrently and retained as bothUint8Arrayand a base64 data URI before submission. A large selection can freeze or exhaust the renderer long before any server-side limit responds. Raising the proxy limit without first defining and enforcing a total budget would turn the accidental 1 MiB guard into a larger denial-of-service surface.
Required direction
- Define one explicit maximum file count and aggregate raw-byte budget, in addition to the per-file limit.
- Reject the excess before starting
FileReaders; use bounded/sequential reads, preserve picker order, and aggregate failures into one result. - Mirror the accepted request budget at the server seam. Raise body limits only on the necessary authenticated proxy paths, including the desktop remote proxy, with a finite value that accounts for base64/JSON overhead.
- Add a server integration regression that sends a near-limit prompt through the real CodeNomad instance proxy, plus remote-proxy coverage. Verify the supported payload against the real OpenCode endpoint too.
- Keep one explicit device-file interface rather than leaking a
requireDataboolean through the generic selection interface. That module should own limits, byte-only semantics, session/disposal fencing, ordering, and failure aggregation.
Product/UI follow-up
The rendered menu is mechanically sound, but Attach files versus Upload from device does not explain the essential origin distinction. In remote contexts use unambiguous labels such as Browse workspace/server files and Upload from this device.
The five “web/Tauri/Electron” browser cases all execute in Chromium and only change runtime globals; they do not exercise Electron, WebView2, WKWebView, or WebKitGTK file pickers. Since the implementation is intentionally host-agnostic, one shared browser behavior test plus an honest native smoke is more valuable than five duplicate scenarios.
Validation performed
- UI and Electron typechecks: pass.
git diff --check: pass.- New Playwright suite: 9/9 pass and rendered menu inspected.
- Instance proxy suite: 38/38 pass, but contains no large prompt-body case.
- Broader targeted attachment/session/client-state tests: 118 pass, 1 environment-specific skip.
- GitHub test/runtime jobs are green; package builds were still running at review time.
The green CI does not invalidate the blockers because the new browser fixture intercepts prompt traffic before either production proxy parser sees the payload.
| list: async () => ({ data: [info("source"), info("other")], cursor: {} }), get: async ({ sessionID }: any) => info(sessionID), | ||
| update: async ({ sessionID }: any) => info(sessionID), | ||
| instructions: { entry: { remove: async () => {}, put: async () => {} } }, | ||
| switchAgent: async () => {}, switchModel: async () => {}, prompt: native.session.prompt, |
There was a problem hiding this comment.
Blocker: this borrowed prompt closure is created with baseUrl: location.origin, whereas production clients use /workspaces/:id/instance/. The Playwright route interception then fulfills the request before either CodeNomad Fastify parser sees it. Consequently the suite passes with payloads that the application rejects: using the same parser configuration, 700 KiB raw succeeds but 800 KiB raw becomes a >1 MiB base64 JSON request and returns 413. Add a near-limit integration test through registerInstanceProxyRoutes; remote desktop also needs coverage through RemoteProxySessionManager because it has an independent default body limit.
| const nativePath = getFilePath(file) | ||
| // Device uploads must carry bytes even when the desktop exposes a local path: | ||
| // that path may not exist on the selected server or in its WSL environment. | ||
| const nativePath = selectionOptions?.requireData ? null : getFilePath(file) |
There was a problem hiding this comment.
Blocker: byte-only selection still needs a batch budget. The input is multiple, this loop accepts an unbounded number of files up to 5 MiB each, starts every FileReader concurrently, and keeps both raw bytes and a base64 URL. That can consume hundreds of MiB or more before submission. Validate a maximum count and aggregate raw size before starting reads, use bounded/sequential processing, and preserve FileList order (the current test sorts the submitted files and therefore hides completion-order reordering). The server must enforce the corresponding encoded request ceiling as well.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/35513182859 Artifacts expire in 7 days.
|
Clarify the two attachment origins as workspace browsing versus uploads from this device, and report a single localized result when selected files cannot be accepted. Centralize the 5 MiB per-file, 10-file, and 20 MiB aggregate contract. Preflight selections before reading, process bytes sequentially in picker order, fence late reads to their originating session, and never fall back to a renderer-local path. Raise request parsing only for prompt routes, including activated remote-window proxies, while enforcing the decoded inline-file budget at the server boundary. Cover exact limits, overages, proxy scoping, browser behavior, and a 5 MiB request through a real isolated OpenCode endpoint.
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Gatekeeper verdict: PASS — merge-ready once required CI is green
Reviewed HEAD 611cf1f9ac88dc8c2f1090f8f833637f24ee2679 against dev at ca94224b74f39abab8a65ee9ceb7ec0ba9334642.
Product and UX
#725 remains relevant: the workspace browser resolves on the CodeNomad server and cannot represent a file that exists only on the user's device. The implementation keeps the correct host-agnostic design (<input type="file"> plus byte-backed OpenCode prompt files), without temporary files or Electron/Tauri-specific upload adapters.
The source choice is now understandable in the rendered menu: Browse workspace files and Upload files from this device. The workspace dialog title follows the same vocabulary, all supported locales are updated, and an inspected real-component capture shows both actions clearly without layout regressions.
Previous blockers
-
Transport limit: resolved. The contract is now 5 MiB per inline file, 10 files, and 20 MiB decoded bytes total. A 32 MiB parser allowance accounts for base64 and JSON overhead. It is scoped to the generated prompt route in the instance proxy and the activated remote-window proxy; unrelated routes retain Fastify's default cap. The server independently validates decoded data-URI size/count/aggregate and rejects malformed data.
-
Unbounded/concurrent renderer reads: resolved. Device selection is a dedicated module. It rejects per-file/count/aggregate excess before reads, reads sequentially in picker order, aggregates failures, rechecks actual byte sizes at commit, blocks submission while pending, and discards the complete batch if its session or component becomes stale. Renderer-local paths are never used as a remote fallback.
-
Coverage seam: resolved. Browser coverage now tests one honest host-agnostic behavior instead of five runtime-global aliases. Separate integrations exercise an exact-limit body through the real CodeNomad instance proxy and HTTPS remote proxy. The isolated native fixture also sent exactly 5,242,880 bytes through the generated client, CodeNomad proxy, and real OpenCode 2.0.11 prompt endpoint.
Regression and architecture review
- Workspace/server browsing remains available and sends the expected bytes.
- Empty selections, repeated selection, unreadable files, oversized files, aggregate count, picker ordering, send fencing, and cross-session completion are covered.
- The limits are shared by UI and server; enforcement remains server-authoritative.
- Remote-proxy activation is checked in
onRequest, before the widened body parser reads the payload. - The focused selection and budget modules keep the new policy out of the already-large prompt and HTTP modules.
- No blocking correctness, security, architecture, or product findings remain.
Validation performed
- UI, Electron, and server typechecks: pass.
- UI production build: pass.
- Server suite: 607 pass, 2 expected platform skips.
- Device-upload browser suite: 6/6 pass.
- Remaining browser suite: 93 pass, 1 expected skip; the native browser-frame file then passed 14/14 after restoring the missing local Electron binary.
- Isolated OpenCode 2.0.11 native proxy/file contract: pass at the exact 5 MiB limit.
git diff --check: pass.
GitHub's new workflow run is still queued/in progress at review time. This PASS does not waive required CI; merge only after those checks complete successfully. No merge was performed.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/35517749759 Artifacts expire in 7 days.
|
|
On review, I think this adds too many overlapping attachment flows. The technical difference between server paths and device uploads does not justify separate neighboring actions in the composer. I would simplify this to:
I suggest revisiting the UX before merging this PR. I'll leave the implementation as it is for now and defer the next steps. |
1 participant
Summary
Closes #725.
I added Upload from device alongside the existing server-side Attach files browser. Both choices are available in web, local desktop and remote desktop windows.
Device selection uses the native file picker and sends file bytes through the existing prompt attachment flow. It never falls back to a device-local path that a remote server or WSL environment cannot read. I kept the existing 5 MB per-file limit, report unreadable or oversized files, and block sending while files are being read. Late reads cannot attach files to a different session or an unmounted composer.
I translated the new labels in all ten locales.
Validation
git diff --checkpassed.Maintenance
Existing large files touched:
packages/ui/src/components/prompt-input.tsx(~1,144 lines) andpackages/ui/src/components/prompt-input/usePromptAttachments.ts(~529 lines).