Skip to content

fix: memory leaks, error handling, and validation bugs #1156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vivekyadav-3 wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

fix: memory leaks, error handling, and validation bugs #1156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 88 additions & 73 deletions packages/api/src/EmbeddedChatApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1065,15 +1065,15 @@ export default class EmbeddedChatApi {
"description",
fileDescription.length !== 0 ? fileDescription : ""
);
const response = fetch(`${this.host}/api/v1/rooms.upload/${this.rid}`, {
const response = await fetch(`${this.host}/api/v1/rooms.upload/${this.rid}`, {
method: "POST",
body: form,
headers: {
"X-Auth-Token": authToken,
"X-User-Id": userId,
},
}).then((r) => r.json());
return response;
});
return await response.json();
} catch (err) {
console.log(err);
}
Expand Down Expand Up @@ -1121,7 +1121,7 @@ export default class EmbeddedChatApi {
try {
const { userId, authToken } = (await this.auth.getCurrentUser()) || {};
const response = await fetch(
`${this.host}/api/v1/chat.search?roomId=${this.rid}&searchText=${text}`,
`${this.host}/api/v1/chat.search?roomId=${this.rid}&searchText=${encodeURIComponent(text)}`,
{
headers: {
"Content-Type": "application/json",
Expand Down Expand Up @@ -1188,17 +1188,20 @@ export default class EmbeddedChatApi {
}

async getCommandsList() {
const { userId, authToken } = (await this.auth.getCurrentUser()) || {};
const response = await fetch(`${this.host}/api/v1/commands.list`, {
headers: {
"Content-Type": "application/json",
"X-Auth-Token": authToken,
"X-User-Id": userId,
},
method: "GET",
});
const data = await response.json();
return data;
try {
const { userId, authToken } = (await this.auth.getCurrentUser()) || {};
const response = await fetch(`${this.host}/api/v1/commands.list`, {
headers: {
"Content-Type": "application/json",
"X-Auth-Token": authToken,
"X-User-Id": userId,
},
method: "GET",
});
return await response.json();
} catch (err) {
console.error(err);
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getCommandsList now catches errors and only logs them, which resolves the promise as undefined on failure. Callers expecting a response object (e.g., accessing data.commands) will then throw a secondary TypeError, and the original fetch error is effectively swallowed. Prefer rethrowing after logging or returning a consistent fallback shape so callers can handle failures predictably.

Suggested change
console.error(err);
console.error(err);
throw err;

Copilot uses AI. Check for mistakes.
}
}

async execCommand({
Expand All @@ -1210,74 +1213,86 @@ export default class EmbeddedChatApi {
params: string;
tmid?: string;
}) {
const { userId, authToken } = (await this.auth.getCurrentUser()) || {};
const response = await fetch(`${this.host}/api/v1/commands.run`, {
headers: {
"Content-Type": "application/json",
"X-Auth-Token": authToken,
"X-User-Id": userId,
},
method: "POST",
body: JSON.stringify({
command,
params,
tmid,
roomId: this.rid,
triggerId: Math.random().toString(32).slice(2, 20),
}),
});
const data = await response.json();
return data;
}

async getUserStatus(reqUserId: string) {
const { userId, authToken } = (await this.auth.getCurrentUser()) || {};
const response = await fetch(
`${this.host}/api/v1/users.getStatus?userId=${reqUserId}`,
{
method: "GET",
try {
const { userId, authToken } = (await this.auth.getCurrentUser()) || {};
const response = await fetch(`${this.host}/api/v1/commands.run`, {
headers: {
"Content-Type": "application/json",
"X-Auth-Token": authToken,
"X-User-Id": userId,
},
}
);
const data = response.json();
return data;
method: "POST",
body: JSON.stringify({
command,
params,
tmid,
roomId: this.rid,
triggerId: Math.random().toString(32).slice(2, 20),
}),
});
return await response.json();
} catch (err) {
console.error(err);
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

execCommand now swallows network/API errors (logs and returns undefined). This can make command execution appear successful to the UI (since the awaiting code won’t throw), potentially leaving the app in an inconsistent state. Prefer propagating the error (rethrow) or returning a structured error result so callers can surface failures.

Suggested change
console.error(err);
console.error(err);
throw err;

Copilot uses AI. Check for mistakes.
}
}

async getUserStatus(reqUserId: string) {
try {
const { userId, authToken } = (await this.auth.getCurrentUser()) || {};
const response = await fetch(
`${this.host}/api/v1/users.getStatus?userId=${reqUserId}`,
{
method: "GET",
headers: {
"Content-Type": "application/json",
"X-Auth-Token": authToken,
"X-User-Id": userId,
},
}
);
return await response.json();
} catch (err) {
console.error(err);
}
}

async userInfo(reqUserId: string) {
const { userId, authToken } = (await this.auth.getCurrentUser()) || {};
const response = await fetch(
`${this.host}/api/v1/users.info?userId=${reqUserId}`,
{
method: "GET",
headers: {
"Content-Type": "application/json",
"X-Auth-Token": authToken,
"X-User-Id": userId,
},
}
);
const data = response.json();
return data;
try {
const { userId, authToken } = (await this.auth.getCurrentUser()) || {};
const response = await fetch(
`${this.host}/api/v1/users.info?userId=${reqUserId}`,
{
method: "GET",
headers: {
"Content-Type": "application/json",
"X-Auth-Token": authToken,
"X-User-Id": userId,
},
}
);
return await response.json();
} catch (err) {
console.error(err);
}
}

async userData(username: string) {
const { userId, authToken } = (await this.auth.getCurrentUser()) || {};
const response = await fetch(
`${this.host}/api/v1/users.info?username=${username}`,
{
method: "GET",
headers: {
"Content-Type": "application/json",
"X-Auth-Token": authToken,
"X-User-Id": userId,
},
}
);
const data = response.json();
return data;
try {
const { userId, authToken } = (await this.auth.getCurrentUser()) || {};
const response = await fetch(
`${this.host}/api/v1/users.info?username=${encodeURIComponent(username)}`,
{
method: "GET",
headers: {
"Content-Type": "application/json",
"X-Auth-Token": authToken,
"X-User-Id": userId,
},
}
);
return await response.json();
} catch (err) {
console.error(err);
}
}
}
10 changes: 7 additions & 3 deletions packages/auth/src/RocketChatAuth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,14 @@ class RocketChatAuth {
* Add a callback that will be called when user login status changes
* @param callback
*/
async onAuthChange(callback: (user: object | null) => void) {
onAuthChange(callback: (user: object | null) => void) {
this.authListeners.push(callback);
const user = await this.getCurrentUser();
callback(user);
this.getCurrentUser().then((user) => {
callback(user);
});
Comment on lines +38 to +40
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onAuthChange calls getCurrentUser().then(...) without a .catch(...). If getCurrentUser() rejects (e.g., unexpected refresh/login errors), this can create an unhandled promise rejection and the listener callback may never be invoked. Add error handling (catch + log and/or invoke callback with null) to keep auth listeners reliable.

Suggested change
this.getCurrentUser().then((user) => {
callback(user);
});
this.getCurrentUser()
.then((user) => {
callback(user);
})
.catch((error) => {
console.error("Failed to fetch current user in onAuthChange:", error);
callback(null);
});

Copilot uses AI. Check for mistakes.
return () => {
this.removeAuthListener(callback);
};
}

async removeAuthListener(callback: (user: object | null) => void) {
Expand Down
5 changes: 3 additions & 2 deletions packages/react-native/src/views/LoginView/LoginView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,12 @@ const LoginView = () => {
};

useEffect(() => {
RCInstance.auth.onAuthChange((user) => {
const unsubscribe = RCInstance.auth.onAuthChange((user) => {
if (user) {
navigate('chat-room');
}
})
});
return () => unsubscribe();
}, [RCInstance])

return (
Expand Down
7 changes: 7 additions & 0 deletions packages/react/src/hooks/useRCAuth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ export const useRCAuth = () => {
const handleLogin = async (userOrEmail, password, code) => {
try {
const res = await RCInstance.login(userOrEmail, password, code);
if (!res) {
dispatchToastMessage({
type: 'error',
message: 'An unexpected error occurred. Please try again.',
});
return;
}
if (res.error === 'Unauthorized' || res.error === 403) {
dispatchToastMessage({
type: 'error',
Expand Down
11 changes: 7 additions & 4 deletions packages/react/src/views/ChatBody/ChatBody.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ const ChatBody = ({
);

useEffect(() => {
RCInstance.auth.onAuthChange((user) => {
const unsubscribe = RCInstance.auth.onAuthChange((user) => {
if (user) {
RCInstance.addMessageListener(addMessage);
RCInstance.addMessageDeleteListener(removeMessage);
Expand All @@ -160,29 +160,32 @@ const ChatBody = ({
RCInstance.removeMessageDeleteListener(removeMessage);
RCInstance.removeActionTriggeredListener(onActionTriggerResponse);
RCInstance.removeUiInteractionListener(onActionTriggerResponse);
unsubscribe();
};
}, [RCInstance, addMessage, removeMessage, onActionTriggerResponse]);

useEffect(() => {
RCInstance.auth.onAuthChange((user) => {
const unsubscribe = RCInstance.auth.onAuthChange((user) => {
if (user) {
getMessagesAndRoles();
setHasMoreMessages(true);
} else {
getMessagesAndRoles(anonymousMode);
}
});
return () => unsubscribe();
}, [RCInstance, anonymousMode, getMessagesAndRoles]);

useEffect(() => {
RCInstance.auth.onAuthChange((user) => {
const unsubscribe = RCInstance.auth.onAuthChange((user) => {
if (user) {
fetchAndSetPermissions();
} else {
permissionsRef.current = null;
}
});
}, []);
return () => unsubscribe();
}, [RCInstance, fetchAndSetPermissions]);

// Expose clearUnreadDivider function via ref for ChatInput to call
useEffect(() => {
Expand Down
6 changes: 5 additions & 1 deletion packages/react/src/views/ChatInput/ChatInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ const ChatInput = ({ scrollToBottom, clearUnreadDividerRef }) => {
);

useEffect(() => {
RCInstance.auth.onAuthChange((user) => {
const unsubscribe = RCInstance.auth.onAuthChange((user) => {
if (user) {
RCInstance.getCommandsList()
.then((data) => setCommands(data.commands || []))
Expand All @@ -155,8 +155,12 @@ const ChatInput = ({ scrollToBottom, clearUnreadDividerRef }) => {
setMembersHandler(channelMembers.members || [])
)
.catch(console.error);
} else {
setCommands([]);
}
});

return () => unsubscribe();
}, [RCInstance, isChannelPrivate, setMembersHandler]);

useEffect(() => {
Expand Down
3 changes: 2 additions & 1 deletion packages/react/src/views/EmbeddedChat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ const EmbeddedChat = (props) => {
}, [RCInstance, auth, setIsLoginIn]);

useEffect(() => {
RCInstance.auth.onAuthChange((user) => {
const unsubscribe = RCInstance.auth.onAuthChange((user) => {
if (user) {
RCInstance.connect()
.then(() => {
Expand All @@ -152,6 +152,7 @@ const EmbeddedChat = (props) => {
setIsUserAuthenticated(false);
}
});
return () => unsubscribe();
}, [
RCInstance,
setAuthenticatedName,
Expand Down
3 changes: 3 additions & 0 deletions packages/react/src/views/LoginForm/LoginForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ export default function LoginForm() {
const handleSubmit = () => {
if (!userOrEmail) setUserOrEmail('');
if (!password) setPassword('');
if (!userOrEmail || !password || userOrEmail.trim() === '' || password.trim() === '') {
return;
}
handleLogin(userOrEmail, password);
};
const handleClose = () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/views/ReportMessage/MessageReportWindow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import ReportWindowButtons from './ReportWindowButtons';
import styles from './ReportMessage.styles';

const MessageReportWindow = ({ messageId, message }) => {
const [reportDescription, setDescription] = useState(' ');
const [reportDescription, setDescription] = useState('');
return (
Comment on lines 8 to 10
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After changing the initial reportDescription to '' (and with the submit button now disabling on reportDescription.trim() === ''), the inline validation message later in this component still checks reportDescription === ''. That means whitespace-only input won’t show the error even though submission is blocked. Update the validation check to use .trim() for consistency.

Copilot uses AI. Check for mistakes.
<ReportWindowButtons
variant="danger"
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/views/ReportMessage/ReportWindowButtons.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ const ReportWindowButtons = ({
<Button
onClick={handleReportMessage}
type="destructive"
disabled={reportDescription === ''}
disabled={reportDescription.trim() === ''}
>
{confirmText}
</Button>
Expand Down
Loading