Guard pending chat members against missing account IDs#96747
Draft
wildan-m wants to merge 5 commits into
Draft
Conversation
…g-chat-members-undefined-toString
…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 must be run against a local dev build (npm run web): minified staging/production bundles omit the module cache and mangle export names, so the script cannot reach the function there and will say so rather than fail silently.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.
Platform scope — the console script is web-only. It reaches the function through the dev build's bundler module registry, which only a local web dev build exposes. It will not run on Android or iOS native (those bundle through Metro, with no such registry), and it will not run against staging or production (minified builds omit the module cache and mangle export names — the script detects this and says so rather than failing silently).
Nothing about the change is platform-specific: it is a single null check in shared TypeScript that executes identically on every platform, with no UI, styling, or native code involved. So on the other platforms there is nothing extra to reproduce — the numbered steps above are the whole check. Invite a member and then remove one, and confirm the member list and the workspace chat's system message still behave exactly as they do today. The unit tests added in this PR cover the missing-ID case itself on every platform, since they run in the shared Jest suite.
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
Do not run the console script from the Tests section — that is a developer-only check that needs a local dev build. For QA this is purely a regression check on the member flows, since the change is a defensive guard with no visible behaviour of its own:
Everything above should behave exactly as it does on production — there is no user-visible change to look for.
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.