Persist projected message image attachments and serve them via /attachments#113
Merged
juliusmarminge merged 13 commits intomainfrom Feb 28, 2026
Merged
Persist projected message image attachments and serve them via /attachments#113juliusmarminge merged 13 commits intomainfrom
/attachments#113juliusmarminge merged 13 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces end-to-end image attachment support by adding database persistence, orchestration-layer materialization to disk, HTTP serving, and client-side URL resolution. Changes span database migrations, persistence and orchestration layers, web server file serving, and web client integration, along with supporting tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Server:<br/>Orchestration
participant FS as FileSystem
participant HttpServer as Server:<br/>HTTP Handler
participant WebClient as Web Client
Client->>Server: Send message with image<br/>(dataUrl attachment)
Server->>Server: Materialize attachment:<br/>parse dataUrl
Server->>FS: Write PNG bytes to<br/>/attachments/thread-id/msg-id/0.png
FS-->>Server: File persisted
Server->>Server: Update attachment ref<br/>to URL path
Server->>Server: Upsert to projection<br/>with URL reference
WebClient->>HttpServer: GET /attachments/thread-id/msg-id/0.png
HttpServer->>FS: Resolve & read file
FS-->>HttpServer: PNG content
HttpServer-->>WebClient: 200 + image/png + bytes
WebClient->>WebClient: Render attachment<br/>via resolved URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts (1)
162-248: Always clean upstateDirinfinallyfor this test.At Line 246, cleanup only runs on the happy path. If any assertion fails earlier, temp directories are leaked.
♻️ Proposed adjustment
it.effect("materializes message image attachments into stateDir and stores URL references", () => Effect.gen(function* () { const projectionPipeline = yield* OrchestrationProjectionPipeline; const eventStore = yield* OrchestrationEventStore; const sql = yield* SqlClient.SqlClient; const now = new Date().toISOString(); const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "t3-projection-attachments-")); + try { const serverConfig = { mode: "web", port: 0, @@ assert.equal(fs.existsSync(attachmentPath), true); assert.deepEqual(fs.readFileSync(attachmentPath), Buffer.from("SGVsbG8=", "base64")); - fs.rmSync(stateDir, { recursive: true, force: true }); + } finally { + fs.rmSync(stateDir, { recursive: true, force: true }); + } }), );Based on learnings: For integration tests, clean up external resources in
finallyblocks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts` around lines 162 - 248, The test "materializes message image attachments into stateDir and stores URL references" creates a temporary stateDir but only removes it at the end of the happy path; wrap the Effect.gen body in a try/finally (inside the it.effect callback) so that fs.rmSync(stateDir, { recursive: true, force: true }) runs in the finally block regardless of failures. Locate the stateDir variable and the final fs.rmSync call in this test and move the removal into the finally block (or use an Effect.acquireRelease/Resource pattern) so cleanup always occurs even when assertions fail.apps/server/src/wsServer.ts (1)
323-333: Consider returning 404 for missing extension assets.Line 323-Line 333 falls back to
index.htmleven when a file-like path (e.g..js,.css) is missing. Limiting SPA fallback to extensionless paths improves failure clarity and cache behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/wsServer.ts` around lines 323 - 333, The current handler falls back to serving index.html whenever fileInfo is missing or not a File (the block around fileInfo/type, indexPath, staticDir, fileSystem.readFile and respond), which incorrectly serves the SPA shell for requests that look like asset files (e.g. .js/.css) instead of returning 404; change the logic so that you only serve index.html for extensionless paths (or paths that do not contain a file extension) and for requests with an extension (detect via path.extname or checking for a dot after the last slash) return respond(404, ...) when the asset is missing rather than falling back to index.html. Ensure fileInfo/fileInfo.type checks remain for valid files but gate the SPA fallback behind the “no extension” condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/orchestration/Layers/ProjectionPipeline.ts`:
- Around line 89-101: The inferImageExtension function and the data-URL parsing
path must only accept known image MIME types and restrict fallback extensions to
a safe whitelist: in inferImageExtension, first ensure attachment.mimeType
exists and startsWith("image/") before mapping via IMAGE_EXTENSION_BY_MIME_TYPE;
if unmapped, only allow a limited set of safe extensions (e.g., .jpg, .jpeg,
.png, .gif, .webp, .bmp, .tiff, .svg, .ico) from the filename and otherwise
return ".bin"; likewise, in the data-URL parsing routine (the code around the
lines that parse data URLs) validate that the parsed MIME type
startsWith("image/") before treating it as an image and materializing a file,
and fall back to ".bin" or reject non-image MIME types.
In `@apps/server/src/persistence/Layers/ProjectionThreadMessages.ts`:
- Around line 45-46: The upsert currently writes NULL when row.attachments is
undefined and then overwrites existing attachments on conflict; update the SQL
in ProjectionThreadMessages so the inserted value uses COALESCE for attachments
and the ON CONFLICT ... DO UPDATE clause sets attachments =
COALESCE(EXCLUDED.attachments, <table>.attachments) (i.e. keep existing
attachments when EXCLUDED/row.attachments is NULL). Modify the expression
referencing row.attachments and the conflict-update assignment to use COALESCE
with the table's attachments column to preserve prior attachment metadata on
partial upserts.
In `@apps/server/src/wsServer.ts`:
- Around line 314-345: The static file-serving branch currently joins untrusted
url.pathname into staticDir (building filePath) without normalization or
containment checks, allowing path traversal; to fix, replicate the attachments
branch hardening: resolve staticDir to an absolute staticRoot, validate and
normalize the incoming url.pathname (reject empty, null-bytes, or a leading ".."
segment), build the candidate path with path.resolve(staticRoot,
normalizedRelative) and then verify containment by checking the resolved
filePath startsWith(`${staticRoot}${path.sep}`); only after those checks proceed
to fileSystem.stat/readFile and use respond as before (refer to symbols
filePath, staticDir, url.pathname, staticRoot, fileSystem, respond, Mime).
In `@apps/web/src/store.ts`:
- Around line 211-214: The code that builds the base URL from wsCandidate forces
any non-"wss:" protocol to "http:", which downgrades "https:" to "http:" and can
break previews; update the logic around wsUrl and protocol (the block using
wsCandidate, wsUrl.protocol and protocol) to preserve existing "https:"/ "http:"
and only translate WebSocket schemes: map "wss:" -> "https:" and "ws:" ->
"http:", otherwise use wsUrl.protocol as-is (or derive by replacing "ws"/"wss"
prefixes), then return `${protocol}//${wsUrl.host}` so HTTPS is not downgraded.
---
Nitpick comments:
In `@apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts`:
- Around line 162-248: The test "materializes message image attachments into
stateDir and stores URL references" creates a temporary stateDir but only
removes it at the end of the happy path; wrap the Effect.gen body in a
try/finally (inside the it.effect callback) so that fs.rmSync(stateDir, {
recursive: true, force: true }) runs in the finally block regardless of
failures. Locate the stateDir variable and the final fs.rmSync call in this test
and move the removal into the finally block (or use an
Effect.acquireRelease/Resource pattern) so cleanup always occurs even when
assertions fail.
In `@apps/server/src/wsServer.ts`:
- Around line 323-333: The current handler falls back to serving index.html
whenever fileInfo is missing or not a File (the block around fileInfo/type,
indexPath, staticDir, fileSystem.readFile and respond), which incorrectly serves
the SPA shell for requests that look like asset files (e.g. .js/.css) instead of
returning 404; change the logic so that you only serve index.html for
extensionless paths (or paths that do not contain a file extension) and for
requests with an extension (detect via path.extname or checking for a dot after
the last slash) return respond(404, ...) when the asset is missing rather than
falling back to index.html. Ensure fileInfo/fileInfo.type checks remain for
valid files but gate the SPA fallback behind the “no extension” condition.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/server/src/orchestration/Layers/ProjectionPipeline.test.tsapps/server/src/orchestration/Layers/ProjectionPipeline.tsapps/server/src/orchestration/Layers/ProjectionSnapshotQuery.tsapps/server/src/persistence/Layers/ProjectionThreadMessages.tsapps/server/src/persistence/Migrations.tsapps/server/src/persistence/Migrations/007_ProjectionThreadMessageAttachments.tsapps/server/src/persistence/Services/ProjectionThreadMessages.tsapps/server/src/wsServer.test.tsapps/server/src/wsServer.tsapps/web/src/store.ts
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
juliusmarminge
force-pushed
the
codething/9de4002b
branch
from
fb05afc to
e9c254f
Compare
- Store message attachments in `projection_thread_messages.attachments_json` - Materialize base64 image attachments to `stateDir/attachments` during projection - Serve `/attachments/*` from the server and resolve preview URLs from WebSocket origin - Add migration and tests for projection materialization and attachment serving
- Materialize only valid image data URLs and restrict inferred file extensions to a safe whitelist - Preserve existing message attachments on upsert unless explicitly replaced or cleared - Serve static `/` via `index.html` and block traversal attempts in static file requests
- Stage attachment file writes until projector transactions commit, then apply write/prune/delete side effects - Remove orphaned attachment files on thread revert and delete thread attachment dirs on thread deletion - Serve URL-encoded attachment paths in `wsServer` and stream file responses - Add regression tests for rollback safety, revert/delete cleanup, and encoded attachment URLs
- Open preview from the full image set instead of a single image - Add previous/next controls with ArrowLeft/ArrowRight keyboard support - Keep Escape-to-close behavior and show current image position
juliusmarminge
force-pushed
the
codething/9de4002b
branch
from
e903021 to
7b39369
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Treat `attachments: []` as an explicit update instead of dropping the field - Add regression test covering attachment clearing for an existing thread message
- Make projection attachment writes use last-write-wins per file path - Always write attachment files so updated message attachments replace stale bytes - Add regression test covering same message/attachment index update behavior
- Read attachment route paths without URI decoding before normalization - Align test fixture paths with encoded thread/message/file names
- Write projected attachments under the thread directory using `messageSegment-fileName` - Avoid nested per-message folders while preserving unique, deterministic file paths - Update projection pipeline tests to match new attachment URLs and on-disk paths
Comment on lines
+606
to
+612
| Effect.catch((cause) => | ||
| Effect.logWarning("failed to persist message attachments", { | ||
| threadId: event.payload.threadId, | ||
| messageId: event.payload.messageId, | ||
| cause, | ||
| }).pipe(Effect.as(event.payload.attachments)), | ||
| ), |
Contributor
There was a problem hiding this comment.
🟡 Medium Layers/ProjectionPipeline.ts:606
If materializeAttachmentsForProjection fails mid-iteration, some writes may already be staged in attachmentSideEffects.writes. The catch block returns the original attachments for DB persistence but doesn't clear the staged writes, causing orphaned files to be written by runAttachmentSideEffects. Consider clearing staged writes (e.g., attachmentSideEffects.writes.length = 0) before returning the fallback.
- Effect.catch((cause) =>
- Effect.logWarning("failed to persist message attachments", {
- threadId: event.payload.threadId,
- messageId: event.payload.messageId,
- cause,
- }).pipe(Effect.as(event.payload.attachments)),
- ),
+ Effect.catch((cause) => {
+ attachmentSideEffects.writes.length = 0;
+ return Effect.logWarning("failed to persist message attachments", {
+ threadId: event.payload.threadId,
+ messageId: event.payload.messageId,
+ cause,
+ }).pipe(Effect.as(event.payload.attachments));
+ }),🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/orchestration/Layers/ProjectionPipeline.ts around lines 606-612:
If `materializeAttachmentsForProjection` fails mid-iteration, some writes may already be staged in `attachmentSideEffects.writes`. The `catch` block returns the original attachments for DB persistence but doesn't clear the staged writes, causing orphaned files to be written by `runAttachmentSideEffects`. Consider clearing staged writes (e.g., `attachmentSideEffects.writes.length = 0`) before returning the fallback.
Evidence trail:
apps/server/src/orchestration/Layers/ProjectionPipeline.ts lines 104-152 (materializeAttachmentsForProjection - iterates attachments, calls stageFileWrite at line 145), lines 597-612 (catch block returns original attachments but doesn't clear writes), lines 601-602 (stageFileWrite pushes to attachmentSideEffects.writes), lines 1121-1125 (runAttachmentSideEffects called after projection), lines 363-377 in runAttachmentSideEffects (writes files from sideEffects.writes).
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because the spend limit has been reached. To enable Bugbot Autofix, raise your spend limit in the Cursor dashboard.
| } | ||
| const ext = Mime.getExtension(attachment.mimeType); | ||
| if (ext && SAFE_IMAGE_FILE_EXTENSIONS.has(ext)) return ext; | ||
| return ".bin"; |
Contributor
There was a problem hiding this comment.
Mime.getExtension fallback dead due to dot mismatch
Medium Severity
Mime.getExtension (wrapping mime@4.1.0) returns extensions without a leading dot (e.g., "png"), but SAFE_IMAGE_FILE_EXTENSIONS contains entries with dots (e.g., ".png"). The .has(ext) check on line 100 will therefore always be false, making the entire Mime.getExtension fallback path dead code. Any image MIME type not found in IMAGE_EXTENSION_BY_MIME_TYPE always falls through to ".bin". Additionally, if the check ever passed, the returned ext without a dot would produce malformed filenames like 0png instead of 0.png.
Additional Locations (1)
Comment on lines
+48
to
+55
| COALESCE( | ||
| ${nextAttachmentsJson}, | ||
| ( | ||
| SELECT attachments_json | ||
| FROM projection_thread_messages | ||
| WHERE message_id = ${row.messageId} | ||
| ) | ||
| ), |
Contributor
There was a problem hiding this comment.
🟢 Low Layers/ProjectionThreadMessages.ts:48
The sub-SELECT in VALUES reads attachments_json before the row is locked by ON CONFLICT, so a concurrent update can be lost. Consider passing NULL directly in VALUES and relying solely on the COALESCE in the DO UPDATE clause to preserve existing attachments.
Suggested change
| COALESCE( | |
| ${nextAttachmentsJson}, | |
| ( | |
| SELECT attachments_json | |
| FROM projection_thread_messages | |
| WHERE message_id = ${row.messageId} | |
| ) | |
| ), | |
| ${nextAttachmentsJson}, |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/persistence/Layers/ProjectionThreadMessages.ts around lines 48-55:
The sub-SELECT in `VALUES` reads `attachments_json` before the row is locked by `ON CONFLICT`, so a concurrent update can be lost. Consider passing `NULL` directly in `VALUES` and relying solely on the `COALESCE` in the `DO UPDATE` clause to preserve existing attachments.
Evidence trail:
apps/server/src/persistence/Layers/ProjectionThreadMessages.ts lines 40-72 (REVIEWED_COMMIT) - sub-SELECT in VALUES at lines 51-55 reads `attachments_json` before conflict detection; DO UPDATE clause at lines 63-67 uses `excluded.attachments_json` first in COALESCE, preserving the potentially stale value. SQLite usage confirmed by apps/server/package.json:22 (@effect/sql-sqlite-bun) and apps/server/src/persistence/Layers/Sqlite.ts.
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
1 participant
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.
Summary
dataUrlpayloads intostateDir/attachments/...and storing URL references in projected message records.attachments_jsoncolumn, repository reads/writes, snapshot decoding, and schema updates).007_ProjectionThreadMessageAttachmentsto extendprojection_thread_messages.stateDirunder/attachments/*with MIME-aware responses.5733to5173.Testing
apps/server/src/orchestration/Layers/ProjectionPipeline.test.ts: verifies image attachment data URLs are materialized to disk and projected as/attachments/{threadId}/{messageId}/{index}.{ext}.apps/server/src/wsServer.test.ts: verifies HTTPGET /attachments/thread-a/message-a/0.pngserves persisted bytes with an image content type.Note
Medium Risk
Adds filesystem writes/cleanup and a new HTTP file-serving route tied to projection events, plus a DB migration; bugs here could cause data loss, broken rendering, or path traversal if validations are wrong.
Overview
Persists projected message image attachments to disk and serves them via
GET /attachments/*. The projection pipeline now rewrites base64 imagedataUrls into file-backed URLs understateDir/attachments, stages writes to run only after the projector transaction commits, and prunes or deletes per-thread attachment files onthread.reverted/thread.deletedwith safety checks against unsafe thread IDs.Adds
attachments_jsontoprojection_thread_messages(migration007), updates the message projection repository and snapshot query to read/write attachments (including preserving existing attachments when omitted and allowing explicit clearing with[]). The HTTP server switches static/attachment serving toFileSystem/Pathwith MIME-based responses and explicit path traversal rejection, and the web client now resolves/attachments/...to an absolute preview URL and supports multi-image navigation in the expanded preview modal.Expands test coverage for attachment materialization, rollback behavior, cleanup, repository upsert semantics, and HTTP serving of persisted attachment bytes; also adjusts several runtime layers/tests to provide
NodeServicesconsistently.Written by Cursor Bugbot for commit cd7081b. This will update automatically on new commits. Configure here.
Note
Persist image message attachments to disk and serve them under
/attachmentsviaServer.createServerin wsServer.tsAdd attachment materialization and filesystem side-effects to the projection pipeline, write image bytes to
stateDir/attachments, rewrite attachment URLs to routed paths, and expose an HTTP route for attachment files with traversal checks and MIME detection.📍Where to Start
Start with the projection flow in
makeOrchestrationProjectionPipelinein ProjectionPipeline.ts, then review HTTP serving inServer.createServerin wsServer.ts.Macroscope summarized cd7081b.
Summary by CodeRabbit
Release Notes
New Features