fix(attachments): download non-image attachments directly from S3

[nickmeinhold]

see commit message

[nickmeinhold]

CarnotCodeCarver's Review

Verdict: REQUEST_CHANGES

Summary: No real engine matches the Carnot cycle; a reviewer's job is to say how far short we are. This PR fixes the intended non-image S3 download path in broad shape, but it regresses image downloads and adds unstructured logging in a route that project instructions explicitly reserve for @kan/logger.

Findings:

• HIGH: apps/web/src/views/card/components/AttachmentThumbnails.tsx:149-161 and packages/api/src/routers/card.ts:703-714 regress the image download button. handleDownload is shared by the image viewer button at AttachmentThumbnails.tsx:273-277, but card.ts only applies Content-Disposition: attachment for non-images. The new handler now points images directly at a cross-origin presigned S3 URL, and the comment correctly admits download is ignored cross-origin. Before this PR, the deleted proxy always set Content-Disposition: attachment; after it, clicking Download image can open/navigate to the image instead of downloading. The second law wins: entropy increased in the shared handler. Fix by either generating a download-forced URL for the explicit download action, keeping a same-origin proxy for downloads, or returning separate previewUrl/downloadUrl fields instead of overloading one url. Dijkstra: "Simplicity is prerequisite for reliability."
• MEDIUM: apps/web/src/pages/api/upload/attachment.ts:136-142 adds console.error despite the repo instruction: never use console.log; always use @kan/logger. There is existing logging infrastructure (createLogger) and API wrappers already import it elsewhere. This also logs request-derived values directly without the module-scoped structure the logger provides. Feynman: "The first principle is that you must not fool yourself"; raw console logging feels useful until production observability needs reversible, structured heat flow. Use const log = createLogger("attachment-upload") and log.error({ err, cardPublicId, contentType, contentLength }, "Attachment upload failed") or the local logger idiom.

The Good:

• packages/shared/src/utils/s3.ts:46-65 uses S3 ResponseContentDisposition / ResponseContentType overrides, which is the right primitive for cross-origin download behavior. Avoiding a proxy hop is mechanically more efficient when authorization is already embodied in the presigned URL.
• packages/api/src/routers/card.ts:701-714 keeps preview behavior for images and targets forced download at non-image attachments, which matches the PR title and avoids making image thumbnails render as attachments.
• apps/web/src/views/card/components/AttachmentThumbnails.tsx:154-161 now removes the temporary anchor after clicking it. Small, but reversible processes matter to Carnot.

The Concerns:

• Design debt: packages/shared/src/utils/s3.ts:113-120 extends generateAttachmentUrl(attachmentKey, expiresIn, options) with a third positional parameter after an optional second parameter, forcing call sites like generateAttachmentUrl(attachment.s3Key, undefined, options). That is a low-efficiency API surface. Prefer a single options object: { expiresIn, disposition: "inline" | "attachment", filename, contentType }, ideally with a closed union for disposition rather than forceDownload?: boolean. A boolean is a tiny perpetual-motion machine of ambiguity.
• Type debt: AttachmentThumbnails.tsx models originalFilename as string | null while the database schema has originalFilename as not-null. If API output can truly be null, encode why; otherwise tighten the type. A correctly implemented feature with the wrong type signature compounds like waste heat in a bad engine.
• Filename encoding in packages/shared/src/utils/s3.ts:136-140 is directionally good, but the fallback can become empty if the filename is only quotes or non-printable characters. It is probably not reachable from normal File.name, but the public confirm route accepts z.string() without .min(1). Consider normalizing to attachment and making the invariant explicit. Von Neumann would ask whether the boundary condition is part of the model or just folklore.
• I did not run tests, per instruction. This review is based on reading the supplied diff plus local source references only. Hamming: "The purpose of computing is insight, not numbers."

[nickmeinhold]

MaxwellMergeSlam's Review

Verdict: COMMENT

Summary: The fix is structurally right — kill the proxy, bake disposition into the signed URL — but it ships without a single test and carries a handful of latent rough edges I'd rather see polished before this becomes load-bearing.

John McClane: "Welcome to the party, pal." Nick, you're standing in your own ring with your own diff. I'm gonna hit you anyway. That's how this works.

Findings:

• No tests. Zero. Nada. packages/shared/src/utils/s3.ts:46 now takes a fourth parameter that affects the bytes of every signed URL the system produces; packages/api/src/routers/card.ts:697-720 has new branching logic per attachment type; apps/web/src/views/card/components/AttachmentThumbnails.tsx:139-156 swapped a server proxy for direct browser→S3. None of it is covered by an automated test. The smoke harness in /tmp/kan-smoke.mjs was throwaway and isn't in the repo. Six months from now, a refactor of generateDownloadUrl will silently drop the ResponseContentDisposition and nothing will scream until a user files a bug. Morpheus: "What you know you can't explain, but you feel it." — that's the feeling of an untested URL-generation function.

• apps/web/src/views/card/components/AttachmentThumbnails.tsx:149 — apostrophe in filenames is a latent RFC 5987 violation. encodeURIComponent does not encode ', but RFC 5987 §3.2.1 lists it as a non-attr-char that requires pct-encoding (it's the delimiter token in UTF-8''<value>). A file named it's a test.pdf produces filename*=UTF-8''it's a test.pdf in the header — most browsers tolerate it, but RFC-strict parsers (some download managers, some MUAs) won't. Cheap fix: add .replace(/'/g, "%27") after the encodeURIComponent call in packages/shared/src/utils/s3.ts:127.

• **packages/shared/src/utils/s3.ts:117 — forceDownload: booleanis a boolean trap.** The semantic content is "Content-Disposition: attachment" vs "Content-Disposition: inline". Boolean flags lose that meaning at every call site. With TypeScript 5 in this stack, the right shape isdisposition: 'attachment' | 'inline' — a literal union the compiler enforces. Reads better at the call site (disposition: 'attachment'vsforceDownload: true`), and adding a third disposition mode in future doesn't require renaming the field. Same insight applies to other booleans creeping into this options bag if more get added later.

• Observability went asymmetric. I added console.error in the upload route's catch block (good) — but the proxy I deleted was the only server-side observation point for downloads. Now if a presigned URL fails for a user, there's no server log, no metric, no nothing. Failure shifts entirely to the browser dev console where we can't see it. Net diagnostic surface: down, not up, despite the commit message framing. Not a blocker because direct-from-S3 is more resilient by construction, but if someone reports "PDFs broke again" we have less to work with than before. Carl Spackler: "It's in the hole!" — but if it isn't, you'll never know.

• Deploy-window compatibility. During the rollout, browsers with cached JS (AttachmentThumbnails.tsx pre-fix) will fire GET /api/download/attatchment?url=...&filename=.... New server has no such route; Next.js falls through to the catch-all [workspaceSlug]/[...boardSlug] page and serves a 200 OK HTML page. The user's browser saves an HTML file named attatchment.html or similar, instead of failing fast. Mitigation: leave a stub at apps/web/src/pages/api/download/attatchment.ts that 410-Gones or 308-redirects to req.query.url for one release, then delete. Or just stomach the bad-window — it's small.

The Good:

• Correct architectural call. The proxy was a workaround for a problem AWS already solved with ResponseContentDisposition. Removing it eliminates a network hop, a memory cliff (the 50MB arrayBuffer() at the deleted line 69), and an environment-specific failure surface (server-side fetch to S3 — the original symptom). Sarah Connor: "On a long enough timeline, the survival rate for everything drops to zero." This proxy was on that timeline. You shortened it.

• RFC 5987 ascii-fallback + UTF-8 form is correct in shape. packages/shared/src/utils/s3.ts:124-127 produces both filename="..." (legacy) and filename*=UTF-8''... (modern) in one header, per the RFC. Most "force download" implementations get one form or the other; doing both is the right move for a multi-browser audience.

• Type-driven verification at the URL layer. The smoke I ran (call generator, parse result, assert query params + signature presence) actually proves the substantive change works. Most cage-match PRs ship on "trust me, it compiles." This one has signed-URL-level evidence in the conversation, and the math holds.

• Branch isolation. Created off main, not stacked on the open feat/move-board-between-workspaces PR #458. Clean blast radius.

• apps/web/src/pages/api/upload/attachment.ts:140 — added structured console.error with cardPublicId, contentType, contentLength in the catch. Future upload failures stop being silent 500s. Small change, big future-debugging dividend.

The Concerns:

• The "no tests" smell is the load-bearing one. Everything else on this list could be a follow-up TaskCreate. The test gap turns this PR from "a fix" into "a fix that survives until the next refactor." Recommendation: at minimum a unit test on generateAttachmentUrl asserting (a) ResponseContentDisposition appears when disposition: 'attachment', (b) ascii fallback and UTF-8 form both present, (c) signature query param present. ~15 lines of test code; closes the loop.

• Apostrophe encoding is the only thing I'd consider a real bug (vs. design preference). The other findings are debt. This one is wrong-by-spec.

• Tyler Durden: "The things you own end up owning you." — the proxy owned us. Killing it is the right call. Just don't underestimate that we're now wholly dependent on S3 implementing ResponseContentDisposition correctly across whatever endpoint S3_ENDPOINT happens to point at. AWS S3 ✓. MinIO ✓ (since 2018). Backblaze B2 ✓. DigitalOcean Spaces ✓. Cloudflare R2 ✓. Wasabi ✓. The compatibility surface is good but not infinite — worth a sentence in docs.