Guard pending chat members against missing account IDs#96747
Draft
wildan-m wants to merge 4 commits into
Draft
Conversation
…g-chat-members-undefined-toString
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
A crash reaches Sentry from the report route as an unhandled promise rejection:
TypeError: undefined is not an object (evaluating 'p.toString'). The helper that assembles a report's pending chat members converts each incoming account ID to a string without first checking that the ID is there. Its parameter is declared as a list of numbers, but nothing enforces that at runtime — the member add and remove flows derive those IDs from personal-detail lookups, and an entry that has not finished loading contributes a missing value. Converting that missing value throws, and because the member actions are dispatched from optimistic-update code the throw surfaces as an unhandled rejection instead of a render error, which is why the production stack has no useful frame.Dropping the missing IDs before the conversion removes the only expression that can throw, and it also keeps a member entry with an empty account ID out of the report's metadata — the alternative of falling back to an empty string would have stored one. Putting the guard in the shared helper rather than at each call site covers the room invite and removal flows and the workspace member flows uniformly, and it leaves behavior for well-formed input unchanged.
Fixed Issues
$ #95348
PROPOSAL: #95348 (comment)
Tests
The Sentry report has no reproduction steps — the failure depends on a personal-detail entry that has not finished loading, so it cannot be triggered on demand. The unit tests added in this PR cover the missing-ID case directly (they throw the same
TypeErrorwithout this change). The steps below confirm the member flows that call the helper are unaffected:Direct check of the guard (console script)
Because the crash depends on runtime data that cannot be produced on demand through the UI, this script calls the real bundled
getPendingChatMembersout of the running dev build and hands it a missing account ID. It is the same script on both branches — only the result differs. It needs a local dev build (npm run web); it will not work against a production bundle.Open any chat (so
ReportUtilsis loaded), then paste this into the DevTools console:On
mainit returnsCRASHES — unpatched (this is main)and logs:On this branch it returns
NO CRASH — guard present (this is the PR branch)and logs:The control case passing on both builds is the point — the only behavioural difference is the missing-ID case. The
TypeErrortext above is Chrome's wording; Safari words the same errorundefined is not an object (evaluating 'p.toString'), which is the form recorded in Sentry.Scope note: this exercises the guard directly by passing the value in. It does not demonstrate the app producing that state on its own, and the reporting Sentry stack is minified, so it is not proof that this helper is the frame that crashed for the reporter.
Offline tests
The change touches only the optimistic pending-member list and adds no network calls. Inviting or removing a member while offline queues the action and shows the member in the pending state exactly as it did before.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
There is no visual change to capture — this is a defensive guard on a value that is normally present.
Verified on the local dev server against the steps above: inviting a member raised the workspace total from 1 to 2 and the member rendered with its role, removing the member returned the total to 1 and posted the removal message to the workspace chat, and the JS console stayed clean throughout.