Open
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36659 +/- ##
===========================================
+ Coverage 70.76% 70.86% +0.09%
===========================================
Files 3195 3208 +13
Lines 113106 113426 +320
Branches 20522 20558 +36
===========================================
+ Hits 80041 80379 +338
+ Misses 31018 31000 -18
Partials 2047 2047
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Member
|
This one seems to be working, it's just conflicting. |
ricardogarim
force-pushed
the
chore/aws-sdk-v3
branch
from
416bf7e to
5930ce4
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMigrate AWS S3 integration from AWS SDK v2 to v3: replace S3 with S3Client and v3 commands, use Changes
Sequence Diagram(s)sequenceDiagram
participant Client as UploadFS / Caller
participant Store as AmazonS3Store
participant SDK as S3Client (v3)
participant S3 as AWS S3
Client->>Store: request signed URL / read stream / write stream
alt Read flow
Store->>SDK: send(GetObjectCommand(params))
SDK->>S3: HTTP GET
S3-->>SDK: Object stream
SDK-->>Store: Response with Body stream
Store-->>Client: readable stream / signed URL (via getSignedUrl(GetObjectCommand, expiresIn))
else Write flow
Client->>Store: write to PassThrough stream
Store->>SDK: Upload(lib-storage) with PutObjectCommandInput (stream body)
SDK->>S3: multipart upload / PUT
S3-->>SDK: upload progress / completion
SDK-->>Store: finish or error
Store-->>Client: emit real_finish or error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ricardogarim
previously approved these changes
Mar 2, 2026
![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: 2
🧹 Nitpick comments (3)
apps/meteor/app/file-upload/server/config/AmazonS3.ts (1)
73-75: Consider removing commented-out code.The
SignatureVersionis deprecated in AWS SDK v3, so the commented line can be removed entirely rather than left as a comment. The CDN comment (line 75) appears to be a long-standing TODO that could also be addressed or removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/file-upload/server/config/AmazonS3.ts` around lines 73 - 75, Remove the dead/commented config lines in AmazonS3.ts: delete the commented-out SignatureVersion retrieval (the deprecated SignatureVersion variable) and the CDN TODO/comment (the RocketChat.settings.get('FileUpload_S3_CDN') comment); keep the active ForcePathStyle = settings.get<boolean>('FileUpload_S3_ForcePathStyle') line intact and ensure no other code references the removed commented symbols (SignatureVersion, CDN) so the file contains only live configuration retrievals.apps/meteor/app/file-upload/ufs/AmazonS3/server.ts (2)
188-190: Consider removingContentLengthfor streaming uploads.The
@aws-sdk/lib-storageUploadclass handles multipart uploads and automatically manages content length. SettingContentLengthexplicitly on a streaming body can cause issues if the actual byte count differs fromfile.size. The Upload helper doesn't require this parameter.♻️ Proposed fix
if (file.type) { uploadParams.ContentType = file.type; } - -if (file.size) { - uploadParams.ContentLength = file.size; -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts` around lines 188 - 190, The upload code sets uploadParams.ContentLength = file.size which can break streaming multipart uploads handled by `@aws-sdk/lib-storage`'s Upload helper; remove the explicit ContentLength assignment (or only set it for non-streaming/buffered bodies) so uploadParams does not include ContentLength when passing a streaming body to Upload. Update the block that references uploadParams, ContentLength, and file.size accordingly to avoid supplying ContentLength for streams.
150-160: Consider narrowing theresponse.Bodytype instead of using implicit property check.The check
'readable' in response.Bodyworks in Node.js but is fragile—Bodyis typed as a cross-platform union (Readable | ReadableStream | Blob). For proper type safety and AWS SDK v3 best practices, either:
- Cast the S3 client to
NodeJsClient<S3Client>at initialization to explicitly narrowBodyto Node.js streams, or- Handle the response.Body type explicitly rather than relying on property existence checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts` around lines 150 - 160, The current check "'readable' in response.Body" is fragile because GetObjectCommand returns Body as a union; fix by narrowing the type explicitly: when creating the S3 client instance (s3) cast it to NodeJsClient<S3Client> so GetObjectCommand responses have Node.js stream Body, or alternatively perform explicit type narrowing on the response.Body returned from send(new GetObjectCommand(params)) (e.g., check and assert it is a Node Readable stream) before returning; update the code around GetObjectCommand/response.Body to use the chosen explicit cast or type guard rather than the property-existence check.
🤖 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/meteor/app/file-upload/ufs/AmazonS3/server.ts`:
- Around line 178-195: The upload parameters never include the configured ACL,
so update the logic that builds uploadParams (the PutObjectCommandInput object
used to construct Upload) to set uploadParams.ACL = classOptions.params.ACL when
that value exists; ensure you reference the same uploadParams used to
instantiate new Upload({ client: s3, params: uploadParams }) so uploaded objects
receive the intended ACL (validate type matches S3 ACL strings and preserve
existing ContentType/ContentLength handling).
In `@apps/meteor/package.json`:
- Around line 69-71: The AWS SDK v3 packages "@aws-sdk/client-s3",
"@aws-sdk/lib-storage", and "@aws-sdk/s3-request-presigner" in package.json are
vulnerable via a transitive "@smithy/config-resolver < 4.4.0"; update the
resolved dependency to >=4.4.0 by bumping these packages to a recent v3 release
(e.g., ^3.1000.0) or running the package manager to refresh the lockfile (npm
update / yarn upgrade) and then verify the lockfile/resolution includes
"@smithy/config-resolver@>=4.4.0"; ensure tests pass and commit the updated
package.json and lockfile.
---
Nitpick comments:
In `@apps/meteor/app/file-upload/server/config/AmazonS3.ts`:
- Around line 73-75: Remove the dead/commented config lines in AmazonS3.ts:
delete the commented-out SignatureVersion retrieval (the deprecated
SignatureVersion variable) and the CDN TODO/comment (the
RocketChat.settings.get('FileUpload_S3_CDN') comment); keep the active
ForcePathStyle = settings.get<boolean>('FileUpload_S3_ForcePathStyle') line
intact and ensure no other code references the removed commented symbols
(SignatureVersion, CDN) so the file contains only live configuration retrievals.
In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts`:
- Around line 188-190: The upload code sets uploadParams.ContentLength =
file.size which can break streaming multipart uploads handled by
`@aws-sdk/lib-storage`'s Upload helper; remove the explicit ContentLength
assignment (or only set it for non-streaming/buffered bodies) so uploadParams
does not include ContentLength when passing a streaming body to Upload. Update
the block that references uploadParams, ContentLength, and file.size accordingly
to avoid supplying ContentLength for streams.
- Around line 150-160: The current check "'readable' in response.Body" is
fragile because GetObjectCommand returns Body as a union; fix by narrowing the
type explicitly: when creating the S3 client instance (s3) cast it to
NodeJsClient<S3Client> so GetObjectCommand responses have Node.js stream Body,
or alternatively perform explicit type narrowing on the response.Body returned
from send(new GetObjectCommand(params)) (e.g., check and assert it is a Node
Readable stream) before returning; update the code around
GetObjectCommand/response.Body to use the chosen explicit cast or type guard
rather than the property-existence check.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
apps/meteor/app/file-upload/server/config/AmazonS3.tsapps/meteor/app/file-upload/ufs/AmazonS3/server.tsapps/meteor/package.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.tsapps/meteor/app/file-upload/server/config/AmazonS3.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.tsapps/meteor/app/file-upload/server/config/AmazonS3.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.tsapps/meteor/app/file-upload/server/config/AmazonS3.ts
🧬 Code graph analysis (1)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts (1)
packages/core-services/src/index.ts (1)
Upload(178-178)
🔇 Additional comments (5)
apps/meteor/app/file-upload/server/config/AmazonS3.ts (1)
82-94: LGTM! Configuration correctly adapted for AWS SDK v3.The configuration properly migrates to v3 patterns:
forcePathStylereplaces the v2s3ForcePathStylefollowRegionRedirects: trueenables automatic region redirect handling- Credentials are correctly consolidated into a single object
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts (4)
84-94: LGTM! Pre-signed URL generation correctly migrated to v3.The
getSignedUrlusage withGetObjectCommandandexpiresInfollows the v3 pattern correctly. TheResponseContentDispositionheader handling for forced downloads is properly preserved.
119-135: LGTM! Delete operation correctly migrated.The
DeleteObjectCommandusage is correct. The error handling behavior (log but don't throw) appears to match the expected semantics.
197-205: LGTM! Upload completion and error handling properly implemented.The
Upload.done()promise handling correctly emitsreal_finishon success and propagates errors to the stream. This maintains the expected stream lifecycle behavior.
59-62: ThecustomUserAgentproperty is the correct configuration for AWS SDK v3's S3Client. It's inherited fromUserAgentInputConfigand is the recommended way to set custom user agents in the SDK.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts (1)
92-92: Remove inline implementation comment.Please remove the
// secondsinline comment at Line 92 to match repository style for implementation code.As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts` at line 92, Remove the inline implementation comment by deleting the trailing "// seconds" after the expiresIn assignment (expiresIn: classOptions.URLExpiryTimeSpan) in the AWS S3 upload code; locate the expiresIn property usage in the same block where classOptions.URLExpiryTimeSpan is referenced (around the presigned URL/put/get options) and ensure the line contains only the assignment without any inline comment.
🤖 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/meteor/app/file-upload/ufs/AmazonS3/server.ts`:
- Around line 146-148: The Range assignment is skipped when options.start or
options.end is 0 because the condition uses truthiness; change the check to
explicitly test for presence (e.g., options.start != null && options.end != null
or typeof options.start !== 'undefined' && typeof options.end !== 'undefined')
so params.Range = `bytes=${options.start}-${options.end}` runs for zero offsets
as well; update the conditional around params.Range in the block that references
options.start, options.end and params.Range accordingly.
- Around line 126-134: The catch block in the S3 delete path currently logs
errors and swallows them—modify the catch in the method that calls s3.send(new
DeleteObjectCommand({...})) so that after logging via SystemLogger.error({
error, key: this.getPath(file), bucket: classOptions.params.Bucket }) the error
is rethrown (or returned as a rejected Promise) so callers of
s3.send/DeleteObjectCommand can observe failures; ensure you preserve the
original error type when rethrowing to avoid masking failure details.
---
Nitpick comments:
In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts`:
- Line 92: Remove the inline implementation comment by deleting the trailing "//
seconds" after the expiresIn assignment (expiresIn:
classOptions.URLExpiryTimeSpan) in the AWS S3 upload code; locate the expiresIn
property usage in the same block where classOptions.URLExpiryTimeSpan is
referenced (around the presigned URL/put/get options) and ensure the line
contains only the assignment without any inline comment.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
🔇 Additional comments (1)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts (1)
178-190: ACL is now correctly applied in upload params.Good fix:
classOptions.params.ACLis propagated intoPutObjectCommandInput, which preserves expected S3 object access behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/file-upload/ufs/AmazonS3/server.ts">
<violation number="1" location="apps/meteor/app/file-upload/ufs/AmazonS3/server.ts:183">
P2: ContentLength is omitted for zero-byte files because the check is truthy-based. Use an undefined check so size 0 still sets ContentLength, otherwise empty uploads can be sent with unknown length and fail or behave inconsistently.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts (1)
126-134:⚠️ Potential issue | 🟠 MajorPropagate S3 delete failures instead of swallowing them.
this.deletelogs errors but does not rethrow, so failed deletes can be observed as success by callers.Suggested fix
} catch (error) { - SystemLogger.error({ error }); + SystemLogger.error({ error, key: this.getPath(file), bucket: classOptions.params.Bucket }); + throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts` around lines 126 - 134, The delete method currently catches S3 DeleteObjectCommand failures, logs them via SystemLogger.error, and then swallows the error; update the catch in the method that calls s3.send(new DeleteObjectCommand(...)) (referenced by DeleteObjectCommand, this.getPath, classOptions.params.Bucket) to rethrow the caught error (or throw a new Error with added context) after logging so callers can observe failures instead of seeing successful resolution.
🧹 Nitpick comments (2)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts (2)
92-92: Remove inline implementation comment.
// secondsshould be removed to align with repository implementation-style guidance.As per coding guidelines: "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts` at line 92, Remove the inline implementation comment after the expiresIn assignment; in the object property where expiresIn is set to classOptions.URLExpiryTimeSpan (see the expiresIn: classOptions.URLExpiryTimeSpan line), delete the trailing "// seconds" comment so the code follows the repository's "no implementation comments" style.
25-28: Tighten ACL typing to AWS v3 type instead of plainstring.
S3Options.params.ACLasstringallows invalid values until runtime. Type it asPutObjectCommandInput['ACL']and drop the cast at use site.Suggested fix
export type S3Options = StoreOptions & { connection: S3ClientConfig; params: { Bucket: string; - ACL: string; + ACL?: PutObjectCommandInput['ACL']; }; URLExpiryTimeSpan: number; getPath: (file: OptionalId<IUpload>) => string; }; @@ - ...(classOptions.params.ACL && { ACL: classOptions.params.ACL as PutObjectCommandInput['ACL'] }), + ...(classOptions.params.ACL && { ACL: classOptions.params.ACL }),Also applies to: 184-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts` around lines 25 - 28, The ACL property in the S3Options.params interface is currently typed as string which allows invalid values; change its type to PutObjectCommandInput['ACL'] by importing PutObjectCommandInput from `@aws-sdk/client-s3` and updating the interface (e.g., S3Options.params.ACL = PutObjectCommandInput['ACL']), then remove any runtime casts where ACL is used (the cast sites referenced in this file, including the later occurrence around the code at the other reported location). Ensure the import is added and both occurrences (the initial params definition and the later one at the second occurrence) are updated to use the AWS v3 ACL type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts`:
- Around line 126-134: The delete method currently catches S3
DeleteObjectCommand failures, logs them via SystemLogger.error, and then
swallows the error; update the catch in the method that calls s3.send(new
DeleteObjectCommand(...)) (referenced by DeleteObjectCommand, this.getPath,
classOptions.params.Bucket) to rethrow the caught error (or throw a new Error
with added context) after logging so callers can observe failures instead of
seeing successful resolution.
---
Nitpick comments:
In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts`:
- Line 92: Remove the inline implementation comment after the expiresIn
assignment; in the object property where expiresIn is set to
classOptions.URLExpiryTimeSpan (see the expiresIn:
classOptions.URLExpiryTimeSpan line), delete the trailing "// seconds" comment
so the code follows the repository's "no implementation comments" style.
- Around line 25-28: The ACL property in the S3Options.params interface is
currently typed as string which allows invalid values; change its type to
PutObjectCommandInput['ACL'] by importing PutObjectCommandInput from
`@aws-sdk/client-s3` and updating the interface (e.g., S3Options.params.ACL =
PutObjectCommandInput['ACL']), then remove any runtime casts where ACL is used
(the cast sites referenced in this file, including the later occurrence around
the code at the other reported location). Ensure the import is added and both
occurrences (the initial params definition and the later one at the second
occurrence) are updated to use the AWS v3 ACL type.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
🧠 Learnings (10)
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-23T17:53:18.785Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
🔇 Additional comments (1)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts (1)
146-147: Nice fix for zero-offset range requests.Using
!= nullcorrectly preserves valid0bounds.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts (1)
126-134:⚠️ Potential issue | 🟠 MajorDo not swallow S3 delete failures.
Errors are logged but not rethrown, allowing callers to treat a failed delete as success, potentially leaving orphaned S3 objects.
Proposed fix
} catch (error) { SystemLogger.error({ error }); + throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts` around lines 126 - 134, The catch block that logs S3 delete failures currently swallows errors; update the delete flow in server.ts (the code that calls s3.send(new DeleteObjectCommand({...})) using this.getPath(file) and classOptions.params.Bucket) to log the error with context via SystemLogger.error and then rethrow the error (or throw a new Error wrapping it) so callers don't treat a failed delete as success; ensure you preserve the existing logged details (path and bucket) when rethrowing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts`:
- Around line 126-134: The catch block that logs S3 delete failures currently
swallows errors; update the delete flow in server.ts (the code that calls
s3.send(new DeleteObjectCommand({...})) using this.getPath(file) and
classOptions.params.Bucket) to log the error with context via SystemLogger.error
and then rethrow the error (or throw a new Error wrapping it) so callers don't
treat a failed delete as success; ensure you preserve the existing logged
details (path and bucket) when rethrowing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-23T17:53:18.785Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
🔇 Additional comments (6)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts (6)
3-12: LGTM!The AWS SDK v3 imports are correctly structured, bringing in the necessary commands, client, and utilities for the migration.
83-95: LGTM!The presigned URL generation correctly uses the v3 pattern with
getSignedUrl,GetObjectCommand, andexpiresInin seconds.
140-161: LGTM!The read stream implementation correctly uses v3 commands with proper range handling (null checks) and validates the response body is a readable stream.
178-200: LGTM!The write stream implementation correctly uses
@aws-sdk/lib-storageUpload with:
- Proper ContentLength handling for zero-byte files (
file.size != null)- ACL inclusion with appropriate type casting
- Error propagation via stream events
205-207: LGTM!Simple and correct accessor for the URL expiry configuration.
57-62: No action needed. ThecustomUserAgentparameter in AWS SDK v3S3Clientacceptsstring | UserAgent, so passing a trimmed environment variable string directly is valid and fully supported.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts (1)
24-27: Typeparams.ACLas the SDK ACL type instead ofstring+ cast.Using
stringat Line 27 and casting at Line 185 bypasses compile-time validation for S3 canned ACL values. ChangeACLtoACL?: PutObjectCommandInput['ACL'];to ensure only valid ACL values are accepted at the type level.Also, remove the inline comment
// secondsat Line 92 per the repository guidelines for TypeScript implementation files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts` around lines 24 - 27, Change the params.ACL type from string to the SDK canned-ACL type by replacing the ACL declaration in the params object with ACL?: PutObjectCommandInput['ACL']; (import PutObjectCommandInput from `@aws-sdk/client-s3` if not already imported) so TypeScript enforces valid ACL values instead of relying on casts, and remove the inline comment "// seconds" from the TypeScript implementation file (the comment adjacent to the timeout/expiry variable) to comply with repository guidelines.
🤖 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/meteor/app/file-upload/ufs/AmazonS3/server.ts`:
- Line 92: Remove the inline implementation comment "// seconds" on the
expiresIn property so the code follows the repo rule against implementation
comments; locate the object literal where expiresIn:
classOptions.URLExpiryTimeSpan is set (reference symbol:
classOptions.URLExpiryTimeSpan and the expiresIn property in the AmazonS3
server.ts implementation) and delete only the trailing comment, leaving the
property assignment unchanged.
---
Nitpick comments:
In `@apps/meteor/app/file-upload/ufs/AmazonS3/server.ts`:
- Around line 24-27: Change the params.ACL type from string to the SDK
canned-ACL type by replacing the ACL declaration in the params object with ACL?:
PutObjectCommandInput['ACL']; (import PutObjectCommandInput from
`@aws-sdk/client-s3` if not already imported) so TypeScript enforces valid ACL
values instead of relying on casts, and remove the inline comment "// seconds"
from the TypeScript implementation file (the comment adjacent to the
timeout/expiry variable) to comply with repository guidelines.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-23T17:53:18.785Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts
🔇 Additional comments (3)
apps/meteor/app/file-upload/ufs/AmazonS3/server.ts (3)
126-135: Delete flow now correctly logs and rethrows S3 failures.Good fix: the error is no longer swallowed, so callers can handle delete failures deterministically.
179-201: Upload path migration is solid (Upload+ typed params + ACL forwarding).This preserves streaming behavior and correctly propagates upload errors through the stream.
147-161: Read-stream range handling and body guards look correct.The explicit null checks for byte ranges and response body validation improve correctness for partial reads.
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
4 participants
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.
Proposed changes (including videos or screenshots)
The AWS SDK for JavaScript v2 has entered maintenance mode on September 8 2024 and will be reaching end-of-support on September 8 2025.
https://aws.amazon.com/blogs/developer/announcing-end-of-support-for-aws-sdk-for-javascript-v2/
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-1287
Steps to test or reproduce
Further comments
AWS SDK v3 deprecates signature versions, so the setting might be removed.
Summary by CodeRabbit