Repository navigation
feat(uploads): Fly.io infrastructure migration — S3/Tigris, presigned URLs, video, batch uploads - #1302
Merged
Conversation
Replaces all fs-based file operations with AWS SDK v3 S3 calls so the processor works correctly on Fly.io where multiple machines cannot share a local disk volume. Deduplication via HeadObject, large uploads via auto-multipart Upload, metadata stored as S3 JSON sidecars. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Task 2: Replace processor-proxied file reads with presigned S3 GET redirects. View/download routes now return 302 to Tigris instead of buffering the entire file through the web process. Task 3: Stub out memory-monitor (per-process checks are meaningless on multi-machine Fly.io), remove checkMemoryMiddleware calls from upload/ attachment routes, and simplify upload-semaphore to tier-based slot limits only. Remove file_storage/cache_storage Docker volumes and add Tigris env vars to docker-compose. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tasks 4–6 of the Fly.io upload infrastructure migration: - Task 4: Add video processing (ffmpeg thumbnail + metadata), raise storage tier limits (free 50MB, pro 250MB, founder 500MB, biz 1GB), add video MIME types to the processor allowlist. - Task 6: Batch upload support for channels and DMs — processAttachmentUploads handles multiple files in a single request; useAttachmentUpload stores an array of attachments; ChannelInput accepts multi-file paste/drop/pick; MessageInput fans out one message per attachment when multiple files are sent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 22 minutes and 35 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughMigrates storage to S3/Tigris, adds S3 client and presigned-url helpers, updates ContentStore/processor to S3-backed storage and temp uploads, adds batch attachment uploads and UI support, introduces video processing (ffmpeg/ffprobe), and replaces memory-aware throttling with fixed permits. ChangesS3-backed Upload & Delivery System
Sequence Diagram(s) sequenceDiagram
Client->>WebServer: POST /api/upload (multipart)
WebServer->>AttachmentService: processAttachmentUploads({ request, target, authContext })
AttachmentService->>UploadSemaphore: acquireSlot(userId, tier)
AttachmentService->>ProcessorService: upload file (+ integrity/hash)
ProcessorService->>ContentStore: saveOriginal(contentHash, stream)
ContentStore->>S3: PutObject(files/{hash}/original)
ProcessorService->>QueueManager: enqueue video-process (if video)
QueueManager->>VideoWorker: processVideo(job)
VideoWorker->>ContentStore: saveCache(contentHash, 'thumbnail.webp', bytes, 'image/webp')
VideoWorker->>S3: PutObject(cache/{hash}/thumbnail.webp)
WebServer-->>Client: JSON [ per-file results ] / 307 redirect for views
🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 4e1aa800e0
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
apps/processor/src/s3-client.ts (1)
5-5: ⚡ Quick winRegion default
'auto'is vendor-specific.The default region
'auto'is specific to Cloudflare R2 and Tigris. If users attempt to use this code with standard AWS S3 without settingAWS_REGION, requests will fail.Consider:
- Documenting this in a comment, or
- Validating the endpoint to ensure
'auto'is only used with known compatible providers, or- Defaulting to a standard AWS region like
'us-east-1'and requiring explicit'auto'for Tigris/R2region: process.env.AWS_REGION ?? ( process.env.AWS_ENDPOINT_URL_S3?.includes('tigris') ? 'auto' : 'us-east-1' ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/processor/src/s3-client.ts` at line 5, The region default uses the vendor-specific literal 'auto' in the S3 client config (see the region assignment using process.env.AWS_REGION ?? 'auto'), which will break with standard AWS S3; change the logic to prevent silently using 'auto' for unknown endpoints by validating the endpoint or environment: either default to a standard AWS region like 'us-east-1' when AWS_REGION is unset, or detect known compatible providers (e.g., AWS_ENDPOINT_URL_S3 includes 'tigris' or your R2 host) and only then allow 'auto'; update the region assignment and add a short comment describing the chosen behavior so callers know to set AWS_REGION for standard AWS usage.apps/processor/src/cache/content-store.ts (2)
185-194: ⚡ Quick winSilent error swallowing may hide operational issues.
Multiple methods catch all errors and return
nullor empty results (e.g.,getOriginalMetadata,saveCachemetadata read,getCache,getOriginal,getCacheMetadata). While this provides graceful degradation, it might hide legitimate S3 connectivity issues, permission errors, or transient failures that should be logged or alerted.Consider logging caught errors at debug/warn level before returning fallback values, so operators can detect S3 availability issues:
} catch (err) { loggers.processor.debug('Failed to read cache metadata', { contentHash, error: err }); return {}; }Also applies to: 296-308, 337-344, 447-454, 475-504
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/processor/src/cache/content-store.ts` around lines 185 - 194, Several methods (getOriginalMetadata, saveCache metadata read, getCache, getOriginal, getCacheMetadata) currently swallow all exceptions and return null/empty results; change each catch clause to capture the error (catch (err)) and log it via the existing logger (e.g., loggers.processor.debug or warn) including context such as normalizedHash or contentHash and the error object before returning the fallback value; update the catch blocks in getOriginalMetadata, saveCache metadata read, getCache, getOriginal, and getCacheMetadata to log a descriptive message and the err variable so S3 connectivity/permission issues are visible to operators.
80-89: 💤 Low valueS3 NotFound detection may be too permissive.
The
isS3NotFoundfunction checks multiple error properties but doesn't validate error types strictly. This might incorrectly classify network errors or permission issues as "not found."♻️ More precise error detection
function isS3NotFound(err: unknown): boolean { - if (!err || typeof err !== 'object') return false; - const e = err as Record<string, unknown>; - return ( - e['name'] === 'NotFound' || - e['Code'] === 'NoSuchKey' || - e['name'] === 'NoSuchKey' || - (e['$metadata'] as Record<string, unknown> | undefined)?.['httpStatusCode'] === 404 - ); + if (!err || typeof err !== 'object') return false; + const e = err as { name?: string; Code?: string; $metadata?: { httpStatusCode?: number } }; + // Only treat as NotFound if we have clear 404 signals + return ( + e.name === 'NoSuchKey' || + e.Code === 'NoSuchKey' || + e.$metadata?.httpStatusCode === 404 + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/processor/src/cache/content-store.ts` around lines 80 - 89, The isS3NotFound function is too permissive; tighten detection by first ensuring the error looks like an AWS S3 error (e.g., has a string 'Code' or 'name' property or a numeric $metadata.httpStatusCode) before treating it as NotFound, and avoid matching on generic/absent fields. Update isS3NotFound to: check that err is an object and that either typeof e['Code'] === 'string' || typeof e['name'] === 'string' || (typeof (e['$metadata'] as any)?.['httpStatusCode'] === 'number'); then return true only if e['Code'] === 'NoSuchKey' || e['name'] === 'NotFound' || (e['$metadata']?.['httpStatusCode'] === 404); ensure you do not treat missing/malformed properties as NotFound in function isS3NotFound.apps/processor/src/workers/__tests__/queue-manager.test.ts (1)
41-50: ⚡ Quick winExtend
JobDataMaptype contract coverage forvideo-process.You added the queue key contract, but the
JobDataMapcontract test still doesn’t assertvideo-processpayload typing.✅ Suggested test addition
-import type { QueueName, QueueStats, JobDataMap, IngestFileJobData, ImageOptimizeJobData, TextExtractJobData, OCRJobData } from '../../types'; +import type { QueueName, QueueStats, JobDataMap, IngestFileJobData, ImageOptimizeJobData, TextExtractJobData, OCRJobData, VideoProcessJobData } from '../../types'; it('given a QueueName, JobDataMap should resolve to the correct job data type', () => { expectTypeOf<JobDataMap['ingest-file']>().toEqualTypeOf<IngestFileJobData>(); expectTypeOf<JobDataMap['image-optimize']>().toEqualTypeOf<ImageOptimizeJobData>(); expectTypeOf<JobDataMap['text-extract']>().toEqualTypeOf<TextExtractJobData>(); expectTypeOf<JobDataMap['ocr-process']>().toEqualTypeOf<OCRJobData>(); + expectTypeOf<JobDataMap['video-process']>().toEqualTypeOf<VideoProcessJobData>(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/processor/src/workers/__tests__/queue-manager.test.ts` around lines 41 - 50, Extend the test coverage for the JobDataMap contract by adding an assertion that the 'video-process' key maps to the correct payload type: import or reference JobDataMap and create a compile-time/type-level check (e.g., declare a const typed as JobDataMap['video-process'] or use your existing assertType helper) to verify required properties for the video-process payload are present and typed correctly; update the test near the existing queue-key assertions (where status and Object.keys(status) are checked) to include this new type assertion for 'video-process' to ensure the payload typing is enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/processor/src/cache/content-store.ts`:
- Around line 71-78: streamToBuffer currently buffers entire S3 bodies into
memory which can OOM for large objects; modify streamToBuffer in
content-store.ts to accept a maxSize parameter (default e.g., 50MB), track
totalSize while iterating the AsyncIterable<Uint8Array>, and throw a clear error
if totalSize exceeds maxSize; update callers of streamToBuffer (or document that
it must only be used for small metadata/cache objects) so large/original file
paths use streaming or presigned URLs instead.
- Around line 456-464: The streamOriginalToFile function currently casts
resp.Body to NodeJS.ReadableStream which is incorrect for AWS SDK v3; remove the
cast and pass resp.Body directly to pipeline (or first convert it to an
AsyncIterable like in streamToBuffer) so the Smithy Body type is handled safely.
Update streamOriginalToFile to use this.normalizeContentHash, call
this.s3.send(new GetObjectCommand({ Bucket: this.bucket, Key:
this.originalKey(normalizedHash) })), check resp.Body exists, then await
pipeline(resp.Body, createWriteStream(destPath)) (or transform resp.Body to an
AsyncIterable<Uint8Array> using the same approach as streamToBuffer) to fix the
type-safety issue without forcing a NodeJS.ReadableStream cast.
In `@apps/processor/src/s3-client.ts`:
- Around line 7-11: The current credentials construction in s3-client.ts sets
secretAccessKey to an empty string when AWS_ACCESS_KEY_ID is present but
AWS_SECRET_ACCESS_KEY is missing, which leads to silent auth failures; update
the logic in the credentials creation (the block referencing
process.env.AWS_ACCESS_KEY_ID / secretAccessKey) to validate that if
AWS_ACCESS_KEY_ID is provided then AWS_SECRET_ACCESS_KEY must also be present
and, if not, throw an explicit Error (or otherwise fail fast) instead of
assigning '' so misconfiguration is caught early and clearly reported.
In `@apps/processor/src/types/index.ts`:
- Around line 72-79: Update the JobResult union and the ProcessingJob.result
typing to include the new VideoProcessResult so the queue result type covers the
'video-process' worker output; specifically add VideoProcessResult to the
JobResult union (or to the result mapping/type variants) and ensure
ProcessingJob.result (or its generic parameter) accepts the VideoProcessResult
shape (success, duration, width, height, thumbnailKey, error) so consumers get
full type safety for video-process jobs.
In `@apps/processor/src/workers/video-processor.ts`:
- Around line 52-59: The external ffmpeg and ffprobe calls use execFileAsync
without timeouts and can hang workers; update both call sites (the execFileAsync
invocation that runs 'ffmpeg' and the execFileAsync invocation that runs
'ffprobe') to pass a timeout option (e.g., { timeout: 30000 }) as the options
argument so the child process is killed after a sensible delay, and ensure any
timeout error is propagated/handled by the existing error path; apply the same
pattern to both execFileAsync('ffmpeg', [...]) and execFileAsync('ffprobe',
[...]) invocations.
In `@apps/web/src/lib/presigned-url.ts`:
- Around line 11-14: The credentials block currently builds an AWS credentials
object if only AWS_ACCESS_KEY_ID is present, risking an empty secret; update the
condition in apps/web/src/lib/presigned-url.ts so credentials are only created
when both process.env.AWS_ACCESS_KEY_ID and process.env.AWS_SECRET_ACCESS_KEY
are non-empty (optionally also check AWS_SESSION_TOKEN if you support it),
otherwise set credentials to undefined; locate the credentials assignment (the
ternary that sets accessKeyId/secretAccessKey) and change the guard to require
both env vars to avoid partial credential construction for presign calls.
In `@packages/lib/src/services/memory-monitor.ts`:
- Around line 50-51: The exported function setupMemoryProtection currently
returns setInterval cast to NodeJS.Timer; change the function signature to
return NodeJS.Timeout (not NodeJS.Timer), remove the unnecessary type cast, and
return the result of setInterval directly (use the existing _intervalMs
parameter or default value as before). Locate the setupMemoryProtection function
and update its return type to NodeJS.Timeout and remove the "as NodeJS.Timer"
cast so the native Node.js typing is used.
In `@packages/lib/src/services/upload-semaphore.ts`:
- Around line 20-21: The code sets this.globalLimit via
parseInt(process.env.UPLOAD_MAX_PERMITS || '20') without validating the result,
which can yield NaN or non-positive values; update the initialization in
upload-semaphore (affecting this.globalLimit and this.globalPermits) to parse
the env var, ensure it is a finite positive integer (e.g., parseInt then check
Number.isFinite/Number.isInteger and >0), and fall back to a safe default (20)
when invalid, then assign this.globalPermits = this.globalLimit.
---
Nitpick comments:
In `@apps/processor/src/cache/content-store.ts`:
- Around line 185-194: Several methods (getOriginalMetadata, saveCache metadata
read, getCache, getOriginal, getCacheMetadata) currently swallow all exceptions
and return null/empty results; change each catch clause to capture the error
(catch (err)) and log it via the existing logger (e.g., loggers.processor.debug
or warn) including context such as normalizedHash or contentHash and the error
object before returning the fallback value; update the catch blocks in
getOriginalMetadata, saveCache metadata read, getCache, getOriginal, and
getCacheMetadata to log a descriptive message and the err variable so S3
connectivity/permission issues are visible to operators.
- Around line 80-89: The isS3NotFound function is too permissive; tighten
detection by first ensuring the error looks like an AWS S3 error (e.g., has a
string 'Code' or 'name' property or a numeric $metadata.httpStatusCode) before
treating it as NotFound, and avoid matching on generic/absent fields. Update
isS3NotFound to: check that err is an object and that either typeof e['Code']
=== 'string' || typeof e['name'] === 'string' || (typeof (e['$metadata'] as
any)?.['httpStatusCode'] === 'number'); then return true only if e['Code'] ===
'NoSuchKey' || e['name'] === 'NotFound' || (e['$metadata']?.['httpStatusCode']
=== 404); ensure you do not treat missing/malformed properties as NotFound in
function isS3NotFound.
In `@apps/processor/src/s3-client.ts`:
- Line 5: The region default uses the vendor-specific literal 'auto' in the S3
client config (see the region assignment using process.env.AWS_REGION ??
'auto'), which will break with standard AWS S3; change the logic to prevent
silently using 'auto' for unknown endpoints by validating the endpoint or
environment: either default to a standard AWS region like 'us-east-1' when
AWS_REGION is unset, or detect known compatible providers (e.g.,
AWS_ENDPOINT_URL_S3 includes 'tigris' or your R2 host) and only then allow
'auto'; update the region assignment and add a short comment describing the
chosen behavior so callers know to set AWS_REGION for standard AWS usage.
In `@apps/processor/src/workers/__tests__/queue-manager.test.ts`:
- Around line 41-50: Extend the test coverage for the JobDataMap contract by
adding an assertion that the 'video-process' key maps to the correct payload
type: import or reference JobDataMap and create a compile-time/type-level check
(e.g., declare a const typed as JobDataMap['video-process'] or use your existing
assertType helper) to verify required properties for the video-process payload
are present and typed correctly; update the test near the existing queue-key
assertions (where status and Object.keys(status) are checked) to include this
new type assertion for 'video-process' to ensure the payload typing is enforced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbc1349d-9e27-44ae-b3ae-f5addc56a7d8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (50)
.env.exampleapps/processor/Dockerfileapps/processor/package.jsonapps/processor/src/api/__tests__/upload.test.tsapps/processor/src/api/upload.tsapps/processor/src/cache/__tests__/content-store.test.tsapps/processor/src/cache/content-store.tsapps/processor/src/s3-client.tsapps/processor/src/server.tsapps/processor/src/services/content-detector.tsapps/processor/src/types/index.tsapps/processor/src/workers/__tests__/ocr-processor.test.tsapps/processor/src/workers/__tests__/queue-manager-full.test.tsapps/processor/src/workers/__tests__/queue-manager.test.tsapps/processor/src/workers/__tests__/text-extractor.test.tsapps/processor/src/workers/__tests__/video-processor.test.tsapps/processor/src/workers/ocr-processor.tsapps/processor/src/workers/queue-manager.tsapps/processor/src/workers/text-extractor.tsapps/processor/src/workers/video-processor.tsapps/web/package.jsonapps/web/src/app/api/channels/[pageId]/upload/__tests__/route.test.tsapps/web/src/app/api/channels/[pageId]/upload/route.tsapps/web/src/app/api/files/[id]/download/__tests__/route.test.tsapps/web/src/app/api/files/[id]/download/route.tsapps/web/src/app/api/files/[id]/view/__tests__/route.test.tsapps/web/src/app/api/files/[id]/view/route.tsapps/web/src/app/api/messages/[conversationId]/upload/__tests__/route.test.tsapps/web/src/app/api/messages/[conversationId]/upload/route.tsapps/web/src/app/api/storage/check/route.tsapps/web/src/app/api/upload/route.tsapps/web/src/app/dashboard/dms/[conversationId]/__tests__/page.test.tsxapps/web/src/components/layout/middle-content/page-views/channel/ChannelInput.tsxapps/web/src/components/layout/middle-content/page-views/channel/__tests__/ChannelInput.test.tsxapps/web/src/components/layout/middle-content/page-views/thread/__tests__/ThreadPanel.test.tsxapps/web/src/components/shared/MessageInput.tsxapps/web/src/components/shared/__tests__/MessageInput.test.tsxapps/web/src/hooks/__tests__/useAttachmentUpload.test.tsapps/web/src/hooks/useAttachmentUpload.tsapps/web/src/lib/presigned-url.tsdocker-compose.ymlpackages/lib/src/services/__tests__/memory-monitor.test.tspackages/lib/src/services/__tests__/storage-limits.test.tspackages/lib/src/services/__tests__/upload-semaphore.test.tspackages/lib/src/services/attachment-upload.tspackages/lib/src/services/memory-monitor.tspackages/lib/src/services/storage-limits.tspackages/lib/src/services/upload-semaphore.tsplan.mdtasks/fly-upload-infra.md
💤 Files with no reviewable changes (2)
- apps/web/src/app/api/upload/route.ts
- apps/web/src/app/api/storage/check/route.ts
…ent removal - video-processor: use fileId (unique per upload record) for temp file names instead of contentHash, preventing concurrent jobs on the same deduplicated content from racing on the same temp paths (P1 from Codex review) - useAttachmentUpload: add instanceId (client-generated cuid) to FileAttachment so removeAttachment removes by slot identity, not content hash — two files with identical bytes no longer clobber each other in the pending-attachment list (P2 from Codex review) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- content-store: streamToBuffer now enforces a 50 MB cap to prevent OOM for large S3 reads; streamOriginalToFile drops the incorrect NodeJS.ReadableStream cast in favour of Readable.from(AsyncIterable) - s3-client + presigned-url: credentials object is only built when BOTH AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY are present, preventing silent auth failures from an empty secret - types/index.ts: VideoProcessResult added to JobResult union so video-process job results are fully typed for consumers - video-processor: ffmpeg/ffprobe calls now carry explicit timeouts (120s / 30s) with SIGKILL so hung processes can't block workers indefinitely - memory-monitor: return type changed from deprecated NodeJS.Timer to NodeJS.Timeout; unnecessary cast removed - upload-semaphore: UPLOAD_MAX_PERMITS parsed with radix 10 and validated (falls back to 20 on NaN/non-positive) to guard against misconfiguration Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…p path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
|
Fixed the video-processor test failure: the assertion at line 62 expected the temp file path to contain |
…etadata, instance keys - Raise `.env.example` file size cap from 50 MB to 1 GB to match business-tier limit - Parameterise `streamToBuffer` with separate 50 MB metadata cap and 2 GB original cap so paid-tier files don't hit a silent ceiling - Restore `isDangerousMimeType` / `ResponseContentType` guards on presigned view URLs (security regression from S3 migration) - Persist video metadata (duration, dimensions, thumbnailKey) via `setPageVideoProcessed` after PgBoss job completes - Key pending attachment bubbles by `instanceId` (not content hash) to prevent React reconciliation collisions on duplicate uploads - Update view-route tests to assert 5-arg `generatePresignedUrl` signature and mock `file-security` Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers: - Files empty state renders upload CTA for a new drive - Upload button triggers POST /api/upload with correct form fields (mocked) - GET /api/files/:id/view returns 401 for unauthenticated requests - GET /api/files/:id/view returns 404 for unknown IDs - POST /api/upload returns 400 when file or driveId is missing - Presigned URL redirect shape (mocked — requires full Docker stack for live run) - Dangerous MIME (SVG) gets attachment disposition in the Location header Requires pnpm dev:services + pnpm dev (or Docker Compose) to run live. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolve merge conflicts: - text-extractor.ts: take master's lazy pdfjs import fix; keep S3-based contentStore.saveCache() from our branch - plan.md: include both Fly.io upload entry and master's new active epics - pnpm-lock.yaml: accept deletion (bun migration landed on master) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Add lockfile entries for S3, ffmpeg/fluent-ffmpeg, and related deps introduced by the uploads branch (missed in master merge). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…UCKET `fly storage attach` injects BUCKET_NAME automatically — no manual secret needed. The bucket resolver now checks BUCKET_NAME first so Fly deployments work without an extra secret, while TIGRIS_BUCKET / S3_BUCKET remain as fallbacks for other environments. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixes EACCES errors on Fly.io where the container filesystem is read-only.
Page body content is now stored in S3 under the same key structure
(page-content/{prefix}/{ref}) so both Fly and VPS deployments share
the Tigris bucket. Reads use Range requests for compression checks.
Tests updated to mock @aws-sdk/client-s3 with an in-memory store.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/e2e/tests/07-file-uploads.spec.ts (1)
68-73: ⚡ Quick winAvoid hardcoded host/port in the unauthenticated API check.
Line 72 hardcodes
http://localhost:3000, which can break when PlaywrightbaseURLchanges. Prefer deriving from configured base URL (or using a relative URL with a request context configured with baseURL).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/e2e/tests/07-file-uploads.spec.ts` around lines 68 - 73, The test "GET /api/files/:id/view returns 401 for unauthenticated request" currently hardcodes "http://localhost:3000" when calling req.get; change the request to use a relative URL so it picks up Playwright's configured baseURL instead—e.g., use req.get('/api/files/some-file-id/view') (or use page.request.get('/api/...') if you prefer the existing page request) so the test uses the configured baseURL rather than a fixed host/port.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/e2e/tests/07-file-uploads.spec.ts`:
- Around line 68-73: The test "GET /api/files/:id/view returns 401 for
unauthenticated request" currently hardcodes "http://localhost:3000" when
calling req.get; change the request to use a relative URL so it picks up
Playwright's configured baseURL instead—e.g., use
req.get('/api/files/some-file-id/view') (or use page.request.get('/api/...') if
you prefer the existing page request) so the test uses the configured baseURL
rather than a fixed host/port.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ee39722-2392-4e87-869a-d6a3f3ab153d
📒 Files selected for processing (18)
.env.exampleapps/e2e/tests/07-file-uploads.spec.tsapps/processor/Dockerfileapps/processor/package.jsonapps/processor/src/cache/content-store.tsapps/processor/src/db.tsapps/processor/src/s3-client.tsapps/processor/src/server.tsapps/processor/src/types/index.tsapps/processor/src/workers/__tests__/text-extractor.test.tsapps/processor/src/workers/__tests__/video-processor.test.tsapps/processor/src/workers/queue-manager.tsapps/processor/src/workers/text-extractor.tsapps/processor/src/workers/video-processor.tsapps/web/package.jsonapps/web/src/app/api/files/[id]/view/__tests__/route.test.tsapps/web/src/app/api/files/[id]/view/route.tsapps/web/src/app/dashboard/dms/[conversationId]/__tests__/page.test.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/processor/src/types/index.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/web/src/app/dashboard/dms/[conversationId]/tests/page.test.tsx
- apps/web/package.json
- apps/processor/src/workers/text-extractor.ts
- apps/processor/src/workers/queue-manager.ts
- apps/processor/src/s3-client.ts
- apps/processor/package.json
- apps/processor/Dockerfile
- apps/processor/src/workers/video-processor.ts
- apps/processor/src/workers/tests/text-extractor.test.ts
- apps/processor/src/workers/tests/video-processor.test.ts
- apps/processor/src/server.ts
- apps/web/src/app/api/files/[id]/view/route.ts
- apps/processor/src/cache/content-store.ts
Replace local disk I/O in the avatar pipeline with S3/Tigris so avatar
reads/writes work across multiple machines without a shared volume.
- apps/processor/src/api/avatar.ts: swap fs.mkdir/readFile/writeFile/rm
with PutObjectCommand/GetObjectCommand/ListObjectsV2+DeleteObjects.
Old-avatar cleanup is best-effort (ListObjectsV2 → DeleteObjects before
each upload). Key pattern: avatars/{userId}/avatar.{ext}.
- apps/web/src/app/api/avatar/[userId]/[filename]/route.ts: remove the
PROCESSOR_URL proxy + FILE_STORAGE_PATH filesystem read; replace with
a direct GetObjectCommand. Validation tightened: SAFE_USER_ID regex
(min 3 chars, alphanumeric+hyphen), isValidFilename() guards against
path traversal, double-dots, slashes, hidden files, and disallowed
extensions — all return 400.
- apps/web/src/lib/presigned-url.ts: export getS3Client + getS3Bucket so
the avatar route can reuse the lazy singleton.
- apps/processor/src/api/__tests__/avatar.test.ts: replace fs mocks with
S3 mock (ListObjectsV2/DeleteObjects/PutObject/GetObject via mockS3Send).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/processor/src/api/avatar.ts`:
- Around line 81-92: The S3 GetObject handling in this block uses a non-null
assertion on response.Body which may be undefined and crash; update the try
block in the avatar handler that calls s3().send(new GetObjectCommand(...)) to
defensively check if response.Body is present before calling
transformToByteArray (i.e., if (!response.Body) return res.status(404).end()),
then proceed to read bytes, determine extension via ext and CONTENT_TYPE_MAP,
and send the buffer as before; ensure the catch remains to handle other errors.
In `@apps/web/src/app/api/avatar/`[userId]/[filename]/route.ts:
- Around line 46-59: The code currently uses a non-null assertion on
response.Body after sending GetObjectCommand (getS3Client, GetObjectCommand,
response.Body, transformToByteArray) which can be undefined; change it to
defensively check if response.Body is present before calling
transformToByteArray and handle the missing-body case (e.g., return a 404 or
appropriate error NextResponse with a descriptive message and status) instead of
using the "!" assertion so you avoid runtime crashes when S3 returns no Body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a5fd129-b6fe-40bf-8481-033e5696a261
📒 Files selected for processing (4)
apps/processor/src/api/__tests__/avatar.test.tsapps/processor/src/api/avatar.tsapps/web/src/app/api/avatar/[userId]/[filename]/route.tsapps/web/src/lib/presigned-url.ts
Both the processor GET endpoint and the web app avatar read route now check `response.Body` before calling `transformToByteArray()`, returning 404 instead of crashing on an unexpected undefined body. Addresses CodeRabbit review comments on f6141a1. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Pass `baseURL` from the Playwright config to the fresh browser context so the request URL is not hardcoded to localhost:3000 and respects whatever base URL is configured for the test environment. Addresses CodeRabbit nitpick on apps/e2e/tests/07-file-uploads.spec.ts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ent routes - page-content-store.ts: replace Body! assertions with explicit checks. readPageContent throws a descriptive error; isContentCompressed returns false on empty body (treats it as uncompressed). - avatar route (processor): log non-404 S3 errors rather than silently swallowing them; still returns 404 to clients. - avatar route (web): use safe pop() fallback instead of non-null assertion. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces objects.map(o => ({ Key: o.Key! })) with flatMap to skip any
objects with undefined Key (technically possible per AWS SDK types).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- docker-compose: remove 100 m tmpfs size cap; business tier allows 1 GB files — any upload over 100 MB would ENOSPC mid-stream. noexec/nosuid security options preserved; kernel manages /tmp ceiling. - attachment-upload: consolidate two auditRequest calls into one (merged channel_upload/dm_upload event carries targetId in details). Remove redundant releaseSlot() from the catch block; the finally block alone is sufficient, matching reviewer preference for a single release site. - video-processor: replace bare path.join(TEMP_ROOT, ...) with resolvePathWithin() guard, consistent with every other temp-path construction in the upload router. fileId is a DB UUID so the guard always passes; the check is for defence-in-depth consistency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
Review response (commit
|
| # | Finding | Plan |
|---|---|---|
| 4 | Content-Disposition not RFC 5987 encoded |
Follow-up issue |
| 5 | Avatar GET proxies S3 bytes instead of presigned redirect | Follow-up issue |
| 6 | saveOriginal omits ContentType on PutObject |
Follow-up issue |
| 7 | 302 vs 307 for presigned redirect | Nit — follow-up |
| 8 | Triplicated S3 client factory | Follow-up refactor |
| 9 | Per-process semaphore limit not documented | Will add comment in follow-up |
| 11 | filePath column semantically misnamed |
Schema migration planned |
| 12 | getOriginal() 2 GB in-memory ceiling undocumented |
Will add doc comment |
| 13 | Content-store test patches private method via unknown cast |
Follow-up test refactor |
| 14 | E2E tests mock the presigned redirect | Follow-up — needs LocalStack sidecar |
| 15 | useAttachmentUpload concurrent-guard test has dangling promise |
Follow-up test fix |
- download route: Content-Disposition now uses both filename= (ASCII fallback with non-ASCII stripped to _) and filename*=UTF-8'' (RFC 5987 percent-encoded). Non-ASCII filenames like 中文.pdf produced a malformed quoted-string in the legacy format. - view route: same RFC 5987 fix applied to the dangerous-MIME attachment disposition. The content-hash path (always hex ASCII) is unchanged. - Both routes: 302 → 307 for presigned URL redirects. Routes are GET-only so there is no method-safety difference, but 307 is semantically correct (Temporary Redirect) for time-limited presigned URLs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ntType - upload-semaphore: add comment noting globalLimit is per-process; with horizontal scaling, effective concurrency is limit × replica count. - content-store: add comments on saveOriginal/saveOriginalFromFile explaining why ContentType is intentionally omitted (presigned URLs override via ResponseContentType) and flagging the latent risk for any future direct-serve path. - content-store: add comment on getOriginal warning that it buffers the full file in memory (up to 2 GB); callers processing large files should use streamOriginalToFile instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Download and view route tests expected 302; updated to 307 to match the semantic-redirect fix in the previous commit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…resolves The concurrent-guard test fired the first upload but never resolved it, leaving a dangling promise and no assertion on the editing-store lifecycle. Now asserts endEditing has not been called before resolution, then resolves the first promise and verifies endEditing fires exactly once. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- page-content-store: NoSuchKey fallback to filesystem for pre-cutover
content not yet backfilled to S3; removes regression risk during migration
- view/download routes: normalize legacy storagePath ('files/{hash}/original')
via toContentHash() before passing to generatePresignedUrl()
- view route: return {url} JSON for Accept:application/json callers so
fetch-based viewers can retrieve the presigned URL then fetch without
forwarding credentials to Tigris (fixes CORS on cross-origin redirect)
- ImageViewer: switch to <img src> which follows 307 redirect without
sending auth headers to the storage origin
- PDFViewer/DocxViewer/CodeViewer: use Accept:application/json + two-step
fetch (get URL, then plain fetch for binary) to avoid credentialed
cross-origin redirect
- ChannelInput: rename uploadFile->uploadFiles on ChannelInputRef
- MessageDropZone: pass all dropped files to uploadFiles() instead of
only the first; update overlay copy and tests to match
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
P1/P2 findings from latest review — all addressed in b57e4b1[P1] Pre-cutover page-content reads —
|
- view route: add tests for Accept:application/json returning {url} JSON
for both file-page and attachment-file paths
- view/download routes: add test that legacy 'files/{hash}/original'
storagePath is normalized to bare hash before presigning
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e dedup Tests the three key behaviors added in this PR: - S3 cache hit returns content without filesystem access - NoSuchKey triggers filesystem fallback using PAGE_CONTENT_STORAGE_PATH - Non-NoSuchKey S3 errors propagate without fallback - writePageContent skips PutObject when HeadObject succeeds (dedup) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The processor was on the internal-only network which has no internet egress. S3 uploads to Tigris failed with ENETUNREACH. Adding it to the frontend network restores external connectivity. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- E2E_BASE_URL / PLAYWRIGHT_BASE_URL env vars override the default localhost:3000, enabling runs against pagespace.team or any tunnel - global-setup sets session cookie domain and secure flag from the resolved base URL instead of hardcoding localhost - getSeedState() lazy-loads .seed-state.json (avoids file-not-found at import time when running individual test files) - 08-real-storage-upload.spec.ts: opt-in real S3 upload test (requires E2E_REAL_STORAGE=1); verifies upload, storage accounting, presigned view/download redirects, and cleans up S3 objects after Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
z.string().url().optional() rejects '' (empty string) — docker-compose
env vars set to VAR= produce an empty string, not undefined. Add
.or(z.literal('')) so unset-but-present env vars pass validation.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/src/services/page-content-store.ts (1)
166-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the migration fallback consistent for metadata helpers.
readPageContent()now supports pre-cutover refs from disk, butisContentCompressed()still does an S3-only lookup, andgetContentMetadata()depends on that plus an S3-onlyHeadObject. During the backfill window, a legacy ref can be readable and still fail metadata inspection.Possible direction
+async function readStoredPageContentFromFilesystem(ref: string): Promise<string> { + const base = process.env.PAGE_CONTENT_STORAGE_PATH + ?? process.env.FILE_STORAGE_PATH + ?? join(process.cwd(), 'storage'); + const filePath = join(base, CONTENT_SUBDIR, ref.slice(0, 2), ref); + return fs.readFile(filePath, 'utf8'); +} + async function readPageContentFromFilesystem(ref: string): Promise<string> { - const base = process.env.PAGE_CONTENT_STORAGE_PATH - ?? process.env.FILE_STORAGE_PATH - ?? join(process.cwd(), 'storage'); - const filePath = join(base, CONTENT_SUBDIR, ref.slice(0, 2), ref); - const storedContent = await fs.readFile(filePath, 'utf8'); + const storedContent = await readStoredPageContentFromFilesystem(ref); if (storedContent.startsWith(COMPRESSION_MAGIC)) { const compressedData = storedContent.slice(COMPRESSION_MAGIC.length); return decompressIfNeeded(compressedData, true); } return storedContent; } export async function isContentCompressed(ref: string): Promise<boolean> { const key = getS3Key(ref); - const response = await s3().send(new GetObjectCommand({ - Bucket: getBucket(), - Key: key, - Range: `bytes=0-${COMPRESSION_MAGIC.length - 1}`, - })); - if (!response.Body) return false; - const bytes = await response.Body.transformToByteArray(); - return Buffer.from(bytes).toString('utf8') === COMPRESSION_MAGIC; + try { + const response = await s3().send(new GetObjectCommand({ + Bucket: getBucket(), + Key: key, + Range: `bytes=0-${COMPRESSION_MAGIC.length - 1}`, + })); + if (!response.Body) return false; + const bytes = await response.Body.transformToByteArray(); + return Buffer.from(bytes).toString('utf8') === COMPRESSION_MAGIC; + } catch (err: unknown) { + const code = (err as { Code?: string; name?: string }).Code ?? (err as { name?: string }).name; + if (code !== 'NoSuchKey') throw err; + return (await readStoredPageContentFromFilesystem(ref)).startsWith(COMPRESSION_MAGIC); + } }
getContentMetadata()should use the same fallback path so legacy-only refs reportstoredSizeinstead of throwing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lib/src/services/page-content-store.ts` around lines 166 - 175, isContentCompressed does an S3-only read which breaks the migration fallback used by readPageContent, causing legacy refs that live on disk to fail metadata checks; update the metadata helpers so they mirror readPageContent's disk fallback: modify isContentCompressed (and/or getContentMetadata) to catch S3 misses/HeadObject failures and fall back to the same disk-read path used by readPageContent (use the same helper that reads pre-cutover refs) to determine storedSize and compression status; ensure you reference and reuse the existing readPageContent/disk helper rather than only calling S3 GetObject/HeadObject so legacy refs report storedSize instead of throwing.
🧹 Nitpick comments (2)
docker-compose.yml (1)
130-161: Consider pinningUPLOAD_MAX_PERMITSin compose as well.With the hard
/tmpcap removed, the processor’s temp-file budget is now entirely host-RAM-dependent. Setting an explicit per-replica permit count here will make local and self-hosted behavior much more predictable on smaller machines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.yml` around lines 130 - 161, Add a pinned UPLOAD_MAX_PERMITS environment variable to the processor service so replicas have a predictable temp-file budget; in the same environment block where DATABASE_URL, PORT, and PROCESSOR_UPLOAD_RATE_LIMIT are declared, add an entry like UPLOAD_MAX_PERMITS=${UPLOAD_MAX_PERMITS:-20} (adjust default as needed) so the processor code that reads UPLOAD_MAX_PERMITS will have a sensible per-replica limit when tmpfs /tmp sizing is host-RAM dependent.apps/web/src/app/api/files/[id]/view/__tests__/route.test.ts (1)
56-60: ⚡ Quick winAssert the MIME-specific presign TTL here.
This suite now exercises the redirect contract, but it still hardcodes
getPresignedUrlTtl()to3600and only checksexpect.any(Number). A regression that gives PDFs the image/video expiry would still pass.Possible tightening
+const mockGetPresignedUrlTtl = vi.fn((mimeType?: string) => + mimeType === 'application/pdf' ? 900 : 3600 +); + vi.mock('`@/lib/presigned-url`', () => ({ generatePresignedUrl: (...args: unknown[]) => mockGeneratePresignedUrl(...args), - getPresignedUrlTtl: vi.fn().mockReturnValue(3600), + getPresignedUrlTtl: mockGetPresignedUrlTtl, }));- expect(mockGeneratePresignedUrl).toHaveBeenCalledWith(VALID_HASH, 'original', expect.any(Number), undefined, 'application/pdf'); + expect(mockGeneratePresignedUrl).toHaveBeenCalledWith(VALID_HASH, 'original', 900, undefined, 'application/pdf');Also applies to: 99-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/api/files/`[id]/view/__tests__/route.test.ts around lines 56 - 60, The test currently stubs getPresignedUrlTtl() to always return 3600 and only asserts expect.any(Number), which won't catch MIME-specific TTL regressions; update the vi.mock for '`@/lib/presigned-url`' so getPresignedUrlTtl inspects the passed MIME/type (or the file's mimetype argument used by the code under test) and returns distinct TTLs (e.g., image/video vs. PDF), then change assertions in route.test.ts (including the similar block at the other test around lines 99-106) to expect the exact TTL for the tested MIME (use the specific number instead of expect.any(Number)) and keep mockGeneratePresignedUrl as is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/e2e/tests/08-real-storage-upload.spec.ts`:
- Around line 17-19: The safety check is only invoked when cleanup credentials
exist, letting runs with E2E_REAL_STORAGE='1' bypass assertSafeE2EBucket; update
the logic so that assertSafeE2EBucket() is executed whenever a real-storage run
is requested (use hasRealStorageEnv or check process.env.E2E_REAL_STORAGE ===
'1') but keep canCleanupS3Objects() only for deciding whether to attempt object
cleanup; specifically, change hasRealStorageEnv and/or the test setup to not tie
the safety guard to requiredEnv/cleanup creds and call assertSafeE2EBucket()
unconditionally for real-storage runs while leaving canCleanupS3Objects() to
control cleanup behavior.
In `@apps/web/src/app/api/files/`[id]/view/route.ts:
- Around line 16-19: toContentHash currently returns the captured legacy hash
exactly as matched, which can preserve uppercase hex; update the function so
when the regex capture succeeds (in function toContentHash) you return the
captured group normalized to lowercase (e.g., m[1].toLowerCase()) so legacy
hashes are canonicalized and avoid case-mismatch issues.
In
`@apps/web/src/components/layout/middle-content/page-views/file/viewers/CodeViewer.tsx`:
- Line 91: In CodeViewer, don't call r.text() unconditionally on fetch(url);
first check the Response.ok (or status) and handle non-2xx responses by throwing
or setting an error state so the viewer can render an error instead of raw HTML
JSON from a 403/404/5xx; locate the fetch call that assigns to text (const text
= await fetch(url).then(r => r.text())) and change it to inspect r.ok (or
r.status), read r.text() only when ok, and propagate a meaningful error
(including status and statusText) to the component's error rendering path.
In
`@apps/web/src/components/layout/middle-content/page-views/file/viewers/DocxViewer.tsx`:
- Around line 53-54: The fetch of the presigned URL in DocxViewer is not
checking HTTP status and can pass 403/404/5xx bodies into the DOCX parser;
update the promise chain that starts with ".then(({ url }) => fetch(url))" to
inspect the Response (check response.ok or status) and throw a descriptive Error
(including response.status and response.statusText) when not ok before calling
response.arrayBuffer(), so downstream DOCX rendering only receives valid buffers
or a clear load error.
In
`@apps/web/src/components/layout/middle-content/page-views/file/viewers/ImageViewer.tsx`:
- Line 3: The ImageViewer component keeps isLoading and error in state across
file/page switches causing stale error/loading UI; add a useEffect that watches
the page.id prop (or other unique file id) and on change resets isLoading to
true (or false as appropriate) and clears error (setError(null)) and any
per-file state (e.g., imageSrc) so each new file starts fresh; update the hook
that initiates loading (inside functions referenced in ImageViewer and any
handlers around the current isLoading/error usage) to rely on this reset to
avoid persisting previous failure states.
In
`@apps/web/src/components/layout/middle-content/page-views/file/viewers/PDFViewer.tsx`:
- Around line 54-60: Before starting the PDF load, clear any stale error state
(call setError(null) or setError(undefined) at the top of the load path
alongside setIsLoading(true)); after obtaining the presigned URL (const { url }
= ...), validate the presigned fetch by awaiting fetch(url) and checking
response.ok (throw a descriptive Error including response.status if not ok)
before calling response.arrayBuffer(); keep these checks around the existing
setIsLoading and existing error handling to ensure failed responses don't get
treated as valid PDF bytes (refer to setIsLoading, setError, fetchWithAuth,
page.id, and the fetch(url).then(...) call).
---
Outside diff comments:
In `@packages/lib/src/services/page-content-store.ts`:
- Around line 166-175: isContentCompressed does an S3-only read which breaks the
migration fallback used by readPageContent, causing legacy refs that live on
disk to fail metadata checks; update the metadata helpers so they mirror
readPageContent's disk fallback: modify isContentCompressed (and/or
getContentMetadata) to catch S3 misses/HeadObject failures and fall back to the
same disk-read path used by readPageContent (use the same helper that reads
pre-cutover refs) to determine storedSize and compression status; ensure you
reference and reuse the existing readPageContent/disk helper rather than only
calling S3 GetObject/HeadObject so legacy refs report storedSize instead of
throwing.
---
Nitpick comments:
In `@apps/web/src/app/api/files/`[id]/view/__tests__/route.test.ts:
- Around line 56-60: The test currently stubs getPresignedUrlTtl() to always
return 3600 and only asserts expect.any(Number), which won't catch MIME-specific
TTL regressions; update the vi.mock for '`@/lib/presigned-url`' so
getPresignedUrlTtl inspects the passed MIME/type (or the file's mimetype
argument used by the code under test) and returns distinct TTLs (e.g.,
image/video vs. PDF), then change assertions in route.test.ts (including the
similar block at the other test around lines 99-106) to expect the exact TTL for
the tested MIME (use the specific number instead of expect.any(Number)) and keep
mockGeneratePresignedUrl as is.
In `@docker-compose.yml`:
- Around line 130-161: Add a pinned UPLOAD_MAX_PERMITS environment variable to
the processor service so replicas have a predictable temp-file budget; in the
same environment block where DATABASE_URL, PORT, and PROCESSOR_UPLOAD_RATE_LIMIT
are declared, add an entry like UPLOAD_MAX_PERMITS=${UPLOAD_MAX_PERMITS:-20}
(adjust default as needed) so the processor code that reads UPLOAD_MAX_PERMITS
will have a sensible per-replica limit when tmpfs /tmp sizing is host-RAM
dependent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80c3e7fa-479b-4f00-aafd-87c33a76132f
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
apps/e2e/fixtures/auth.fixture.tsapps/e2e/fixtures/data.fixture.tsapps/e2e/fixtures/seed-state.tsapps/e2e/global-setup.tsapps/e2e/package.jsonapps/e2e/playwright.config.tsapps/e2e/tests/07-file-uploads.spec.tsapps/e2e/tests/08-real-storage-upload.spec.tsapps/e2e/tsconfig.jsonapps/processor/src/api/avatar.tsapps/processor/src/cache/content-store.tsapps/processor/src/s3-client.tsapps/processor/src/workers/video-processor.tsapps/web/src/app/api/avatar/[userId]/[filename]/route.tsapps/web/src/app/api/files/[id]/download/__tests__/route.test.tsapps/web/src/app/api/files/[id]/download/route.tsapps/web/src/app/api/files/[id]/view/__tests__/route.test.tsapps/web/src/app/api/files/[id]/view/route.tsapps/web/src/components/layout/middle-content/page-views/channel/ChannelInput.tsxapps/web/src/components/layout/middle-content/page-views/channel/MessageDropZone.tsxapps/web/src/components/layout/middle-content/page-views/channel/__tests__/MessageDropZone.test.tsxapps/web/src/components/layout/middle-content/page-views/file/viewers/CodeViewer.tsxapps/web/src/components/layout/middle-content/page-views/file/viewers/DocxViewer.tsxapps/web/src/components/layout/middle-content/page-views/file/viewers/ImageViewer.tsxapps/web/src/components/layout/middle-content/page-views/file/viewers/PDFViewer.tsxapps/web/src/hooks/__tests__/useAttachmentUpload.test.tsapps/web/src/lib/presigned-url.tsdocker-compose.ymlpackages/lib/src/services/__tests__/page-content-store.test.tspackages/lib/src/services/attachment-upload.tspackages/lib/src/services/page-content-store.tspackages/lib/src/services/upload-semaphore.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/web/src/lib/presigned-url.ts
- apps/processor/src/s3-client.ts
- apps/web/src/app/api/avatar/[userId]/[filename]/route.ts
- apps/web/src/app/api/files/[id]/download/tests/route.test.ts
- apps/processor/src/workers/video-processor.ts
- apps/web/src/app/api/files/[id]/download/route.ts
- apps/processor/src/api/avatar.ts
- apps/e2e/tests/07-file-uploads.spec.ts
- packages/lib/src/services/attachment-upload.ts
- apps/processor/src/cache/content-store.ts
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- e2e: assertSafeE2EBucket() runs unconditionally for all real-storage
runs, not only when cleanup credentials are present
- view/download routes: normalize legacy storagePath hash to lowercase
in toContentHash() to avoid case-mismatch on presigned URL generation
- CodeViewer: validate presigned URL fetch response before reading body
- DocxViewer: fail fast on non-OK presigned URL fetch before arrayBuffer
- ImageViewer: reset isLoading/error on page.id change via useEffect;
add key={page.id} to <img> so React remounts on file switch
- PDFViewer: clear stale error at loadPdf start; validate presigned
fetch response before reading arrayBuffer
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- PDFViewer: reset pdfData to null at the start of each load so a stale previous PDF does not remain visible while a new file is loading or when the new load errors - content-store: replace obj.Key! non-null assertion in deleteCacheContent with flatMap null guard, consistent with the avatar deletion pattern Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This branch was previously deployed
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
Migrates all upload/file/avatar/page-body storage from VPS-local filesystem to Fly.io-compatible S3/Tigris object storage, removes single-machine constraints, adds video support, raises tier limits, and enables batch multi-file uploads.
Storage migrations (no more local filesystem for user data)
@aws-sdk/client-s3replaces localfscalls. Files keyed atfiles/{contentHash}/original; cache atcache/{contentHash}/{preset}. Deduplication viaHeadObject.FILE_STORAGE_PATH-based writes. Key pattern:page-content/{ref[0:2]}/{ref}. Compression magic header preserved.NoSuchKeyfallback to filesystem for pre-cutover content not yet backfilled — remove after migration sync completes.PutObject/GetObject/ListObjectsV2+DeleteObjectsatavatars/{userId}/avatar.{ext}. Web avatar read route reads S3 directly — no more filesystem proxy conditional.Feature additions
/api/files/[id]/viewand/api/files/[id]/downloadreturn 307 redirects to time-limited Tigris presigned URLs. Images/video: 3600s TTL; documents: 900s.{ url }JSON forAccept: application/jsoncallers. Viewers use two-step fetch (get URL, then plain fetch for binary). ImageViewer uses<img src>so the browser follows the redirect natively without forwarding auth headers cross-origin.memory-monitorstubbed to always returnnormal;upload-semaphoredrops its memory-polling interval;docker-compose.ymlremoves volume mounts andmem_limitcaps.attachmentMeta. Free tier 50 MB / 3 concurrent, Pro 250 MB / 5, Founder 500 MB / 5, Business 1 GB / 10.processAttachmentUploadsaccepts multiplefilefields in one FormData.useAttachmentUploadhook stores anattachments[]array with uniqueinstanceIdper slot;ChannelInputsupports multi-file paste/drop/picker.Review fixes applied (CodeRabbit + Codex)
response.Bodybefore.transformToByteArray()in all three S3 read pathsAWS_ACCESS_KEY_ID+AWS_SECRET_ACCESS_KEYmust be presentstreamToBuffercapped at 50 MB (cache/metadata objects only — originals stream to disk)SIGKILL;maxBuffersetVideoProcessResultadded toJobResultunionUPLOAD_MAX_PERMITSparsing validated (finite + positive, fallback 20)fileIdto avoid races on deduplicated contentinstanceIdfor removal-by-slot, not content hashbaseURLfixture instead of hardcodedlocalhost:3000Review fixes applied (round 2)
size=100mtmpfs cap — business tier allows 1 GB files; 100 MB caused ENOSPC mid-upload.noexec/nosuidpreserved.auditRequestcalls into one; removed redundantreleaseSlot()from catch block (finally is the single release site)path.join→resolvePathWithinfor temp file paths, consistent with rest of upload routerfilename*=UTF-8''...encoding for non-ASCII filenames inContent-DispositiongetOriginal()2 GB in-memory ceiling; documented whyContentTypeis intentionally omitted onPutObjectfor originalsReview fixes applied (round 3 — P1/P2 findings)
readPageContentaddsNoSuchKeyfallback to legacy filesystem path so pre-cutover content keeps working during the S3 migration windowtoContentHash()normalizes legacystoragePath = 'files/{hash}/original'before callinggeneratePresignedUrl()— prevents double-nested keyfiles/files/{hash}/original/original<img src>(browser follows redirect natively); PDF/Docx/Code useAccept: application/json→ get presigned URL → plainfetch()for binary, avoiding forwardingAuthorizationheader to TigrisChannelInputRef.uploadFiles(File[])replacesuploadFile(File);MessageDropZonepasses all dropped files instead of just the firstTest plan
bun run test:unit— 513 test files passing (3 pre-existing failures unrelated to this PR)fly.storage.tigris.dev)attachmentMetahas duration/dimensionsVPS migration notes (post-merge operational steps)
Before switching VPS to Tigris, sync existing data:
Then add env vars to Docker Compose:
BUCKET_NAME,AWS_ACCESS_KEY_ID,AWS_SECRET_ACCESS_KEY,AWS_ENDPOINT_URL_S3.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements