Conversation
| // Freezing the array ensures that it cannot be unintentionally modified. | ||
| const EMPTY_ARRAY = Object.freeze([]); | ||
| const EMPTY_OBJECT = Object.freeze({}); | ||
| const EMPTY_SET = new Set<string>(); |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The EMPTY_SET constant should be frozen like EMPTY_ARRAY and EMPTY_OBJECT to prevent unintentional modification. The comment on lines 19-20 states: "Freezing the array ensures that it cannot be unintentionally modified."
Suggested fix:
const EMPTY_SET = Object.freeze(new Set<string>());This ensures consistency with the existing EMPTY_ARRAY and EMPTY_OBJECT constants and prevents accidental mutation.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Object.freeze does not prevent mutations on Set
There was a problem hiding this comment.
Dang, the power of true Senior Engineer!
Although +1 in general for the AI about pin-pointing EMPTY_SET should truly stay immutable.
There was a problem hiding this comment.
Should we consider overriding add, clear and delete ? To throw, so that any changes to the EMPTY_SET can be prevented.
There was a problem hiding this comment.
I think that can be a follow up
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Starting review based on this: https://expensify.slack.com/archives/C07NMDKEFMH/p1769720156540859?thread_ts=1769119091.846219&cid=C07NMDKEFMH |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
ShridharGoel
left a comment
There was a problem hiding this comment.
Approving with this comment for consideration if it makes sense: #80888 (comment)
| // Freezing the array ensures that it cannot be unintentionally modified. | ||
| const EMPTY_ARRAY = Object.freeze([]); | ||
| const EMPTY_OBJECT = Object.freeze({}); | ||
| const EMPTY_SET = new Set<string>(); |
There was a problem hiding this comment.
I think that can be a follow up
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.11-16 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.12-1 🚀
|
Explanation of Change
Every time the component renders, the expression
new Set()is evaluated. If it's used as a default value foruseOnyxcalls, it breaks the react compiler, leading to a leak. This change introduces stable reference of empty Set, and making components using it compiled.On copilot account on iOS, it leads to big improvement in time spent on
getSectionsfunction when opening a Report from Reports tab:Before: 2.3s
After: 900ms
Fixed Issues
$ #80469
PROPOSAL:
Tests
n/a
Offline tests
n/a
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
n/a
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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