fix: memory leaks, error handling, and validation bugs#1156
fix: memory leaks, error handling, and validation bugs#1156vivekyadav-3 wants to merge 1 commit intoRocketChat:developfrom
Conversation
There was a problem hiding this comment.
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.onAuthChangelisteners 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.
| this.getCurrentUser().then((user) => { | ||
| callback(user); | ||
| }); |
There was a problem hiding this comment.
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.
| 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); | |
| }); |
| }); | ||
| return await response.json(); | ||
| } catch (err) { | ||
| console.error(err); |
There was a problem hiding this comment.
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.
| console.error(err); | |
| console.error(err); | |
| throw err; |
| }); | ||
| return await response.json(); | ||
| } catch (err) { | ||
| console.error(err); |
There was a problem hiding this comment.
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.
| console.error(err); | |
| console.error(err); | |
| throw err; |
| const MessageReportWindow = ({ messageId, message }) => { | ||
| const [reportDescription, setDescription] = useState(' '); | ||
| const [reportDescription, setDescription] = useState(''); | ||
| return ( |
There was a problem hiding this comment.
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.
"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!"