fix: Prevent "Maximum update depth exceeded" crash when selecting the same workspace while creating consecutive reports - #96720
Conversation
…the same workspace while creating consecutive reports
|
@jayeshmangwani Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid.movAndroid: mWeb Chromemweb-chrome.moviOS: HybridAppios.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.mov |
|
|
||
| // We use a ref because `resolveModal` is called on demand, so it doesn't need to re-render whenever `modalStack` changes. | ||
| // This keeps the `ModalContext.Provider` value stable and prevents unnecessary updates, avoiding an infinite re-render loop (#96411). | ||
| const modalStackRef = useRef(modalStack); |
There was a problem hiding this comment.
Lets use usePrevious here please
There was a problem hiding this comment.
@JS00001 I'm not sure I understand what you mean here. If we use const modalStackPrev = usePrevious(modalStack) and then use modalStackPrev in resolveModal, the issue would still occur because resolveModal would still re-render whenever modalStackPrev changes. Since modalStackPrev changes whenever modalStack changes, I don't think that would solve the problem.
If I've misunderstood your suggestion, please let me know.
Also, if you'd prefer not to use a ref, I have a slightly hacky alternative: wrap the entire resolveModal logic inside the functional updater of setModalStack.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
I meant that we have a helper hook for this: Code here:
|
|
@JS00001 Ah, I understand your suggestion now. You're right that this pattern is repeated in several places. However, I think we need to use the Would it make sense to introduce a new helper to avoid this repetition? For example: function useSyncedRef<T>(value: T) {
const ref = useRef<T>(value);
useEffect(() => {
ref.current = value;
}, [value]);
return ref;
}Then we could use it in both places. |
|
@dukenv0307 we only use 😕 |
|
So this would just be: |
Just to make sure I understand correctly: |
|
no, because its a ref |
|
Its the same code that you've already added in this PR, but its using the helper that already exists for this pattern |
But |
|
Ah right, you're reading it within a callback so we need .current instead of the stored value, sorry! |
|
Here is the compiled code for In contrast, this PR ensures that
let t4;
if ($[3] !== modalStack.modals) {
t4 = (t5) => {
const data = t5 === undefined ? { action: ModalActions.CONFIRM } : t5;
const lastModalId = modalStack.modals.at(-1)?.id;
if (!lastModalId) {
return;
}
const lastModalPromise = modalPromisesStack.current?.[lastModalId];
if (lastModalPromise) {
lastModalPromise.resolve(data);
delete modalPromisesStack.current[lastModalId];
}
};
$[3] = modalStackValue.modals;
$[4] = t4;
} else {
t4 = $[4];
}
const resolveModal = t4;
const modalStackValue = usePrevious(modalStack);
let t4;
if ($[3] !== modalStackValue.modals) {
t4 = (t5) => {
const data = t5 === undefined ? { action: ModalActions.CONFIRM } : t5;
const lastModalId = modalStackValue.modals.at(-1)?.id;
if (!lastModalId) {
return;
}
const lastModalPromise = modalPromisesStack.current?.[lastModalId];
if (lastModalPromise) {
lastModalPromise.resolve(data);
delete modalPromisesStack.current[lastModalId];
}
};
$[3] = modalStackValue.modals;
$[4] = t4;
} else {
t4 = $[4];
}
const resolveModal = t4;
useEffect(t4, t5);
let t6;
if ($[6] === Symbol.for("react.memo_cache_sentinel")) {
t6 = (t7) => {
const data = t7 === undefined ? { action: ModalActions.CONFIRM } : t7;
const lastModalId = modalStackRef.current.modals.at(-1)?.id;
if (!lastModalId) {
return;
}
const lastModalPromise = modalPromisesStack.current?.[lastModalId];
if (lastModalPromise) {
lastModalPromise.resolve(data);
delete modalPromisesStack.current[lastModalId];
}
};
$[6] = t6;
} else {
t6 = $[6];
}
const resolveModal = t6; |
No worries at all! Thanks for taking another look. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 JS00001 has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.4.43-0 🚀
|
|
🤖 I reviewed the changes in this PR and no help site documentation updates are required. Why: This is a purely internal technical fix. The change in There is no change to any user-facing feature, UI element, tab, setting, label, button, or workflow — so nothing maps to an article under @dmkt9, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.4.43-1 🚀
Bundle Size Analysis (Sentry): |
5 participants
Explanation of Change
modalStackto avoid unnecessary changes to theModalContext.Providervalue.DynamicNewReportWorkspaceSelectionPage) opens a confirmation modal from auseEffectthat is triggered by changes to theshowModalorcloseModalfunctions from the modal context.Fixed Issues
$ #96411
PROPOSAL: #96411 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
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
chrome.mp4