Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
Rohit3523
had a problem deploying
to
official_android_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 (3)
app/views/ChangeAvatarView/index.tsx (1)
82-90: Minor: Consider renaming selector parameter to avoid shadowing.The selector uses
stateas its parameter name, which shadows thestatefromuseReduceron line 79. While this works correctly, it could be confusing during code review or debugging.♻️ Rename selector parameter for clarity
-const { userId, username, server, user } = useAppSelector( - state => ({ - userId: getUserSelector(state).id, - username: getUserSelector(state).username, - server: state.server.server, - user: state.login.user +const { userId, username, server, user } = useAppSelector( + appState => ({ + userId: getUserSelector(appState).id, + username: getUserSelector(appState).username, + server: appState.server.server, + user: appState.login.user }), shallowEqual );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/ChangeAvatarView/index.tsx` around lines 82 - 90, The selector callback passed to useAppSelector shadows the local reducer variable named state; rename the selector parameter (e.g., from state to rootState or appState) to avoid confusion and make the intent clearer—update the call using useAppSelector(state => ({ ... })) to use useAppSelector(rootState => ({ userId: getUserSelector(rootState).id, username: getUserSelector(rootState).username, server: rootState.server.server, user: rootState.login.user }), shallowEqual) and keep references to getUserSelector, useAppSelector and shallowEqual intact.app/lib/services/restApi.ts (2)
732-737: Headers include redundant optional chaining.Lines 735-736 use
user?.tokenanduser?.id, but these are already verified as truthy on line 721. This is harmless but slightly redundant.♻️ Remove redundant optional chaining
const headers = { ...RocketChatSettings.customHeaders, 'Content-Type': 'multipart/form-data', - 'X-Auth-Token': user?.token, - 'X-User-Id': user?.id + 'X-Auth-Token': user.token, + 'X-User-Id': user.id };🤖 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 732 - 737, The headers object in restApi.ts uses redundant optional chaining for user?.token and user?.id even though user is already validated earlier; update the headers construction (const headers = { ...RocketChatSettings.customHeaders, 'Content-Type': 'multipart/form-data', 'X-Auth-Token': user?.token, 'X-User-Id': user?.id }) to use user.token and user.id instead, keeping RocketChatSettings.customHeaders and the other header keys unchanged.
721-730: Extension extraction may produce invalid MIME types for certain URLs.The extension extraction
url.split('.').pop()could produce unexpected results:
- URLs with query strings:
image.png?v=123→png?v=123→ invalid MIME type- URLs without extensions or data URIs would default to
pngwhich may not match actual contentFor this avatar upload flow via ImagePicker, the URL should typically be a clean file path. However, consider adding sanitization for robustness.
♻️ Optional: Sanitize extension extraction
if (service === 'upload' && url && server && user?.id && user?.token) { - const extension = url.split('.').pop() || 'png'; + const rawExtension = url.split('.').pop() || 'png'; + const extension = rawExtension.split('?')[0].toLowerCase(); // Remove query params const formData = [🤖 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 721 - 730, The extension extraction for the upload branch (the block that sets extension and formData) can produce invalid MIME types when url contains query strings or no real extension; update the logic in that service === 'upload' branch to sanitize the extension by stripping query/hash fragments from url (remove anything after ? or #), validate the resulting extension against a whitelist of known image extensions (e.g., png,jpg,jpeg,gif,webp) and fall back to a safe default like "png" if validation fails; optionally, if available, prefer deriving MIME from an explicit content-type or from the ImagePicker-provided file info instead of the url string.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/definitions/rest/v1/users.tsapp/lib/methods/helpers/fileUpload/Upload.android.tsapp/lib/services/restApi.tsapp/views/ChangeAvatarView/index.tsxapp/views/ChangeAvatarView/submitServices.ts
🧰 Additional context used
🧬 Code graph analysis (3)
app/views/ChangeAvatarView/submitServices.ts (2)
app/definitions/IProfile.ts (1)
IAvatar(21-26)app/lib/services/restApi.ts (1)
setAvatarFromService(702-745)
app/lib/services/restApi.ts (1)
app/lib/methods/helpers/fetch.ts (2)
url(42-54)headers(23-27)
app/views/ChangeAvatarView/index.tsx (3)
app/lib/hooks/useAppSelector.ts (1)
useAppSelector(6-6)app/selectors/login.ts (1)
getUserSelector(20-20)app/views/ChangeAvatarView/submitServices.ts (1)
changeUserAvatar(15-21)
⏰ 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). (2)
- GitHub Check: format
- GitHub Check: ESLint and Test / run-eslint-and-test
🔇 Additional comments (6)
app/definitions/rest/v1/users.ts (1)
72-74: LGTM!The new
users.setAvatarendpoint type definition is correctly structured and aligns with the Rocket.Chat API documentation. It properly defines the POST method withavatarUrlparameter for URL-based avatar setting.app/lib/methods/helpers/fileUpload/Upload.android.ts (1)
13-13: LGTM!The dynamic
fieldNameimplementation correctly allows different multipart field names per upload use case. The default'file'value maintains backward compatibility, while avatar uploads will use'image'as expected by the Rocket.Chat API.Also applies to: 23-23, 40-47, 62-62
app/views/ChangeAvatarView/submitServices.ts (1)
15-21: LGTM!The updated signature correctly passes
serverandusercontext tosetAvatarFromService, enabling the new upload-based avatar setting path. The spread operator cleanly merges the avatar data with authentication context.app/lib/services/restApi.ts (2)
1-2: LGTM on new imports.The imports for
RocketChatSettingsfrom the SDK andFileUploadhelper are correctly added to support the new avatar upload functionality.Also applies to: 30-30
713-716: Theserverparameter is actually always provided by the only caller in the codebase.While
server: stringis indeed required and is not used in the fallback path (line 744), the only caller (app/views/ChangeAvatarView/submitServices.ts:17) always provides it via{ ...avatarUpload, server, user }. There are no other callers in the codebase that would fail to provide this parameter, so backward compatibility is not an issue.app/views/ChangeAvatarView/index.tsx (1)
164-166: LGTM on updated avatar submission.The
changeUserAvatarcall correctly passes the avatar state, server URL, and user authentication context needed for the new upload-based avatar API endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/lib/services/restApi.ts`:
- Around line 732-737: The headers object in restApi.ts uses redundant optional
chaining for user?.token and user?.id even though user is already validated
earlier; update the headers construction (const headers = {
...RocketChatSettings.customHeaders, 'Content-Type': 'multipart/form-data',
'X-Auth-Token': user?.token, 'X-User-Id': user?.id }) to use user.token and
user.id instead, keeping RocketChatSettings.customHeaders and the other header
keys unchanged.
- Around line 721-730: The extension extraction for the upload branch (the block
that sets extension and formData) can produce invalid MIME types when url
contains query strings or no real extension; update the logic in that service
=== 'upload' branch to sanitize the extension by stripping query/hash fragments
from url (remove anything after ? or #), validate the resulting extension
against a whitelist of known image extensions (e.g., png,jpg,jpeg,gif,webp) and
fall back to a safe default like "png" if validation fails; optionally, if
available, prefer deriving MIME from an explicit content-type or from the
ImagePicker-provided file info instead of the url string.
In `@app/views/ChangeAvatarView/index.tsx`:
- Around line 82-90: The selector callback passed to useAppSelector shadows the
local reducer variable named state; rename the selector parameter (e.g., from
state to rootState or appState) to avoid confusion and make the intent
clearer—update the call using useAppSelector(state => ({ ... })) to use
useAppSelector(rootState => ({ userId: getUserSelector(rootState).id, username:
getUserSelector(rootState).username, server: rootState.server.server, user:
rootState.login.user }), shallowEqual) and keep references to getUserSelector,
useAppSelector and shallowEqual intact.
1 participant
Proposed changes
This introduces the new endpoint for uploading avatar https://developer.rocket.chat/apidocs/set-user-avatar
which is also being used by web.
Pending: E2E Test for avatar upload
Issue(s)
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit