Skip to content

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

Open
vivekyadav-3 wants to merge 1 commit intoRocketChat:developfrom
vivekyadav-3:fix/memory-leaks-and-error-handling
Open

fix: memory leaks, error handling, and validation bugs#1156
vivekyadav-3 wants to merge 1 commit intoRocketChat:developfrom
vivekyadav-3:fix/memory-leaks-and-error-handling

Conversation

@vivekyadav-3
Copy link

"Basically, we’ve given EmbeddedChat a major tune-up to make it run smoother, more reliably, and a whole lot safer. Here’s the human-readable version of what we fixed:

First and most importantly, we tackled some hidden performance drains. We discovered that the app was accidentally leaving 'ghost' listeners running in the background every time it updated. Over time, these would pile up and slow down the user's browser (a classic memory leak). We've now made sure the app properly 'cleans up after itself,' which keeps it fast and responsive no matter how long the chat stays open.

We also focused on reliability. We found several spots where the code was essentially 'guessing' that a server task was finished before it actually was, or failing to have a backup plan if the internet cut out. We’ve smoothed out those interactions, added safety checks, and made sure the app handles errors gracefully instead of simply crashing.

From a security and stability standpoint, we’ve improved how the app handles usernames and search queries. By adding proper encoding, we’ve ensured that special characters won’t break the app’s connection or leave it vulnerable to common web exploits.

Finally, we polished the user experience by adding some common-sense checks to our forms. For example, the app will no longer let you try to log in with empty fields or send a 'blank' report that's just a bunch of spaces. This prevents frustrating errors and makes the whole interface feel much more professional and 'solid.'

In short: the app is now faster, won't crash as easily, and is much better at handling the messy reality of the web!"

Copilot AI review requested due to automatic review settings February 14, 2026 15:23
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves EmbeddedChat’s reliability and safety by preventing auth listener leaks, tightening user input validation, and hardening a few API calls (URL encoding + awaiting fetches) to behave more predictably.

Changes:

  • Add unsubscribe-based cleanup for auth.onAuthChange listeners across React and React Native views to prevent listener accumulation.
  • Improve client-side validation for login and message reporting to block whitespace-only submissions.
  • Harden API request handling by encoding query params (search/user lookup) and awaiting fetch responses.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/react/src/views/ReportMessage/ReportWindowButtons.js Disables “Report” when description is whitespace-only.
packages/react/src/views/ReportMessage/MessageReportWindow.js Initializes report description as empty string (ties into improved validation).
packages/react/src/views/LoginForm/LoginForm.js Prevents login submit when fields are empty/whitespace.
packages/react/src/views/EmbeddedChat.js Stores and cleans up auth-change subscription to avoid leaks.
packages/react/src/views/ChatInput/ChatInput.js Cleans up auth-change subscription; clears commands on logout.
packages/react/src/views/ChatBody/ChatBody.js Cleans up multiple auth-change subscriptions; fixes effect dependencies.
packages/react/src/hooks/useRCAuth.js Adds guard/toast when login returns an unexpected empty result.
packages/react-native/src/views/LoginView/LoginView.js Cleans up auth-change subscription to avoid leaks.
packages/auth/src/RocketChatAuth.ts Changes onAuthChange to return an unsubscribe function (enables cleanup).
packages/api/src/EmbeddedChatApi.ts Awaits fetch/json; encodes search/username query params; adds try/catch around several calls.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +38 to +40
this.getCurrentUser().then((user) => {
callback(user);
});
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 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.
});
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.
Comment on lines 8 to 10
const MessageReportWindow = ({ messageId, message }) => {
const [reportDescription, setDescription] = useState(' ');
const [reportDescription, setDescription] = useState('');
return (
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments