Fixed this issue: https://linear.app/comm/issue/ENG-2090/overlay-navigator-crashes-when-we-open-too-many-overlays-at-the-same
Talked with Ashoat and added if statement to prevent cleaning scene data few times.
Left invariant inside if statement just for sanity checks. If should be deleted let me know.
Details
Details
Build app.
Tried to reproduce crash, but unsuccessfully.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- fix/app_crash
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/navigation/overlay-navigator.react.js | ||
|---|---|---|
| 289 | Early exit please!! We don't need to increase indentation | |
| 290–293 | This doesn't need to exist anymore | |
| native/navigation/overlay-navigator.react.js | ||
|---|---|---|
| 289–291 ↗ | (On Diff #17797) | This doesn't seem to be correct. We were making sure that newVisibleOverlays.length < curVisibleOverlays.length - if that was the case, we were continuing. Otherwise, we were crashing the app. |
Comment Actions
It seems like the initial revision got this right and then updated revision inverted the logic. It is always a good idea to test the code after every change.
| native/navigation/overlay-navigator.react.js | ||
|---|---|---|
| 289 ↗ | (On Diff #17799) | It doesn't seem possible that filtering an array would result in array longer than the original one, so it should be enough to test for equality. |
Comment Actions
@przemek, strongly suggest reviewing your diff via git diff before putting it on Phabricator in the future. All 4 of these issues (all of them except my first feedback) could probably have been spotted by you if you just closely looked at the code you are submitting.
One inline comment left... please address before landing
| native/ios/Podfile.lock | ||
|---|---|---|
| 1325 ↗ | (On Diff #17801) | Please revert this change and upgrade your version of Cocoapods |