Conversation
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
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:
WalkthroughAdds a server-version-aware avatar upload flow: new REST endpoint declaration, client routing to POST avatars (multipart or URL) for >=8.0.0, preserves legacy DDP for older servers, implements multipart multipart upload (including Android fieldName handling), updates UI base64 usage, and adds unit and E2E tests. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant View as ChangeAvatarView
participant Service as setAvatarFromService
participant Upload as uploadUserAvatarMultipart
participant SDK as SDK/REST API
User->>View: Select or capture image
View->>View: read serverVersion, decide includeBase64
View->>Service: call setAvatarFromService(payload)
alt Server < 8.0.0
Service->>SDK: methodCallWrapper('setAvatarFromService', data, contentType, service)
SDK-->>Service: result
else Server >= 8.0.0 and payload is HTTP(S) URL
Service->>SDK: POST users.setAvatar { avatarUrl }
SDK-->>Service: result
else Server >= 8.0.0 and payload is file:// URL
Service->>Upload: uploadUserAvatarMultipart(fileUri, mimeType, filename)
Upload->>Upload: copy file to cache, prepare multipart form (name: "image")
Upload->>SDK: multipart POST users.setAvatar (file field)
SDK-->>Upload: result
else Server >= 8.0.0 and payload is base64 (when enabled)
Service->>Upload: uploadUserAvatarBase64(data, contentType)
Upload->>Upload: write base64 to cache, prepare multipart form
Upload->>SDK: multipart POST users.setAvatar (cached file)
SDK-->>Upload: result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
OtavioStasiak
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
|
Android Build Available Rocket.Chat Experimental 4.72.0.108507 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNQxZApafMyT_9-tfOcBN_Vxr8RAtZ7kpb4exxfEO-x4GtD-aCrxxtfrzJO_tfFVWw3qdO9NwTu1lNnGEWvh |
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/lib/services/restApi.ts (1)
747-750: Consider handling edge case whenservice === 'url'butdatais not a string.The condition
service === 'url' && typeof data === 'string'safely guards against non-string data, but ifservice === 'url'with undefined/non-stringdatais passed, execution falls through to subsequent checks and eventually throws "Invalid avatar payload".This is technically correct, but the error message could be misleading. Consider adding an explicit check for this case with a more descriptive error, or document that
datamust be a string whenservice === 'url'.💡 Optional: Add explicit validation
// RC 8.0.0 — REST users.setAvatar (multipart image or JSON avatarUrl) if (service === 'url' && typeof data === 'string') { await sdk.post('users.setAvatar', { avatarUrl: data }); return; } + + if (service === 'url') { + throw new Error('Avatar URL service requires data to be a string'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/restApi.ts` around lines 747 - 750, When service === 'url' but data is not a string, add explicit validation before falling through: check the branch around the existing if (service === 'url' && typeof data === 'string') and handle the case service === 'url' && typeof data !== 'string' by throwing or returning a clear error (e.g., "avatar URL must be a string when service==='url'") instead of allowing execution to continue; update the logic near the sdk.post('users.setAvatar', { avatarUrl: data }) call so the code either calls sdk.post for valid string data or raises a descriptive error for non-string/undefined data..maestro/tests/assorted/change-avatar.yaml (1)
96-102: Test approach inconsistency across test cases.This test case now uses a tap-and-wait-visible pattern to verify the submit button doesn't proceed when no avatar changes have been made. However, the other test cases ("take a photo" at lines 130-134 and "URL" at lines 163-167) still use the
enabled: false/enabled: trueassertion pattern.Consider aligning all test cases to use the same approach for consistency and maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.maestro/tests/assorted/change-avatar.yaml around lines 96 - 102, The change-avatar test is inconsistent with the other avatar flows: instead of using tapOn + extendedWaitUntil for 'change-avatar-view-submit', switch to the same enabled assertions used in the "take a photo" and "URL" cases (use enabled: false to assert the submit is disabled before changes, and enabled: true when an avatar is selected) so the test pattern is consistent and maintainable; update the steps referencing 'change-avatar-view-submit' to replace the tapOn/extendedWaitUntil pattern with the enabled:false / enabled:true assertions and mirror the enabling assertion where the other flows enable the button.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.maestro/tests/assorted/change-avatar.yaml:
- Around line 96-102: The change-avatar test is inconsistent with the other
avatar flows: instead of using tapOn + extendedWaitUntil for
'change-avatar-view-submit', switch to the same enabled assertions used in the
"take a photo" and "URL" cases (use enabled: false to assert the submit is
disabled before changes, and enabled: true when an avatar is selected) so the
test pattern is consistent and maintainable; update the steps referencing
'change-avatar-view-submit' to replace the tapOn/extendedWaitUntil pattern with
the enabled:false / enabled:true assertions and mirror the enabling assertion
where the other flows enable the button.
In `@app/lib/services/restApi.ts`:
- Around line 747-750: When service === 'url' but data is not a string, add
explicit validation before falling through: check the branch around the existing
if (service === 'url' && typeof data === 'string') and handle the case service
=== 'url' && typeof data !== 'string' by throwing or returning a clear error
(e.g., "avatar URL must be a string when service==='url'") instead of allowing
execution to continue; update the logic near the sdk.post('users.setAvatar', {
avatarUrl: data }) call so the code either calls sdk.post for valid string data
or raises a descriptive error for non-string/undefined data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97333bde-ee27-4141-a052-4e0311001510
📒 Files selected for processing (6)
.maestro/tests/assorted/change-avatar.yamlapp/lib/methods/helpers/fileUpload/Upload.android.tsapp/lib/methods/uploadAvatar/uploadAvatar.test.tsapp/lib/methods/uploadAvatar/uploadAvatar.tsapp/lib/services/restApi.tsapp/lib/services/setAvatarFromService.test.ts
✅ Files skipped from review due to trivial changes (2)
- app/lib/methods/uploadAvatar/uploadAvatar.test.ts
- app/lib/services/setAvatarFromService.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/methods/uploadAvatar/uploadAvatar.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). (4)
- GitHub Check: E2E Build iOS / ios-build
- GitHub Check: E2E Build Android / android-build
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Files:
app/lib/services/restApi.tsapp/lib/methods/helpers/fileUpload/Upload.android.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase configuration including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/restApi.tsapp/lib/methods/helpers/fileUpload/Upload.android.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict mode enabled and configure baseUrl to app/ for import resolution
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/lib/services/restApi.tsapp/lib/methods/helpers/fileUpload/Upload.android.ts
app/lib/services/restApi.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use REST API via fetch in app/lib/services/restApi.ts for HTTP requests
Files:
app/lib/services/restApi.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/services/restApi.tsapp/lib/methods/helpers/fileUpload/Upload.android.ts
🧠 Learnings (1)
📚 Learning: 2026-03-05T14:28:10.004Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6997
File: .maestro/tests/room/message-markdown-click.yaml:28-39
Timestamp: 2026-03-05T14:28:10.004Z
Learning: In Maestro YAML selector fields (text, id) within the Rocket.Chat React Native repository, use the contains pattern '.*keyword.*' (leading and trailing '.*') for matching text. The pattern '.*keyword*.' is incorrect and will fail to match cases where the keyword appears at the end of the element's text. This guideline applies to all Maestro YAML selector fields across the codebase.
Applied to files:
.maestro/tests/assorted/change-avatar.yaml
🔇 Additional comments (5)
app/lib/methods/helpers/fileUpload/Upload.android.ts (3)
12-12: Good state extension for multipart field propagation.Adding
fieldNameto the internal file payload cleanly supports caller-defined multipart keys without affecting existing file metadata handling.
41-41: Correct mapping fromIFormData.nameto upload field name.This correctly carries the caller’s multipart field key (for example,
imagefor avatar upload) through the Android upload path.
60-60: Nice backward-compatible multipart field selection.Using
this.file.fieldName || 'file'enables the new avatar REST contract while retaining existing upload flows that depend onfile.app/lib/services/restApi.ts (2)
725-726: LGTM!Clean type guard implementation with proper type narrowing for HTTP/HTTPS URL detection.
728-763: LGTM on the version-aware routing logic.The implementation correctly:
- Preserves legacy DDP behavior for servers < 8.0.0
- Routes URL-based avatars to the REST endpoint
- Routes local file uploads to the multipart uploader
- Guards against HTTP URLs being misrouted to the multipart upload path with the
!isHttpUrlcheck- Provides a clear error for invalid payloads
The structure aligns well with the REST endpoint definition in
app/definitions/rest/v1/users.tsand the multipart helper inapp/lib/methods/uploadAvatar/uploadAvatar.ts.
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
official_android_build
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/methods/helpers/ImagePicker/ImagePicker.mock.ts`:
- Around line 33-48: The createMockImage async function should guard the
FileSystem.writeAsStringAsync call with a try/catch so a write failure doesn't
reject openPicker/openCamera; wrap the await FileSystem.writeAsStringAsync(...)
in try/catch inside createMockImage and on error return the original in-memory
image (the existing image object) as the fallback, optionally logging the caught
error via an available logger or console, ensuring callers still receive a valid
mock image instead of a rejected promise.
- Around line 50-58: Change the duplicated mock implementations to use the
upstream Options type and a single shared implementation: import { Options }
from "react-native-image-crop-picker", change the signatures of openPicker and
openCamera from options: any to options: Options, and factor the common logic
(the await createMockImage() + return options?.multiple ? [mockedImage] :
mockedImage) into one helper (e.g., openImageMock or resolveMockImage) which
both openPicker and openCamera call (or export a single unified function if
preferred); ensure return types remain Promise<any> (or tighten if you know the
mock image type) and remove the duplicated wrapper logic around
createMockImage().
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17835849-88a9-42f5-a136-e07cbda4071b
📒 Files selected for processing (2)
.maestro/tests/assorted/change-avatar.yamlapp/lib/methods/helpers/ImagePicker/ImagePicker.mock.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .maestro/tests/assorted/change-avatar.yaml
📜 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: E2E Build iOS / ios-build
- GitHub Check: E2E Build Android / android-build
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Files:
app/lib/methods/helpers/ImagePicker/ImagePicker.mock.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase configuration including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/methods/helpers/ImagePicker/ImagePicker.mock.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict mode enabled and configure baseUrl to app/ for import resolution
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/lib/methods/helpers/ImagePicker/ImagePicker.mock.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/methods/helpers/ImagePicker/ImagePicker.mock.ts
🔇 Additional comments (1)
app/lib/methods/helpers/ImagePicker/ImagePicker.mock.ts (1)
3-3: Good change: file-system import supports realistic multipart test paths.This is a solid setup for returning file-backed images instead of only in-memory data URIs.
OtavioStasiak
requested a deployment
to
upload_official_ios
GitHub Actions
Waiting
2 participants
Proposed changes
Use REST to upload avatars to workspaces > 8.0.
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-2063
How to test or reproduce
Case I
Case II
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit