Fix crash when the Prepublishing sheet is restored without a post - #23148
Conversation
The sheet is a DialogFragment, so the FragmentManager restores it after a config change or process death regardless of whether the host activity has loaded a post, and PrepublishingHomeFragment then built its UI against an empty EditPostRepository. Dismiss the sheet instead, and stop EditPostRepository.status from laundering a null post through the unannotated PostStatus.fromPost. Fixes JETPACK-ANDROID-1EVE
Generated by 🚫 Danger |
|
|
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #23148 +/- ##
=======================================
Coverage 37.84% 37.84%
=======================================
Files 2345 2345
Lines 127511 127517 +6
Branches 17709 17713 +4
=======================================
+ Hits 48255 48261 +6
+ Misses 75298 75297 -1
- Partials 3958 3959 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The guard added in the previous commit only covered the HOME screen, but a restored sheet navigates to whatever screen was saved in KEY_SCREEN_STATE. On a restore into TAGS, CATEGORIES or PUBLISH the sheet stayed open over an empty EditPostRepository and still crashed on the first interaction, via requireNotNull(post) in updateAsync. Guarding in PrepublishingViewModel.start() instead covers every screen the sheet can restore into, and reuses the existing dismissBottomSheet channel, so the dismissSheet LiveData and the parentFragment cast are no longer needed. The PrepublishingHomeViewModel early return stays as a last line of defense, since the dismissal is asynchronous and the FragmentManager can restore that fragment before it commits. Also reverts the EditPostRepository.status leniency: with the guard in place its only reader is unreachable with a null post, so returning UNKNOWN just hid the missing post from the other callers.
PostsListActivity has no configChanges, so it is recreated on rotation with a freshly injected EditPostRepository, while PostListMainViewModel survives. Its start() returned early on isStarted before assigning the repository and before loading the post into it, so after any rotation the ViewModel kept writing to the previous activity's dead repository while the restored Prepublishing sheet read the new empty one. That is the state the sheet guard was catching: publishing stayed broken for the rest of the activity's life, silently once the sheet began dismissing itself instead of crashing. Rebind on every start instead, and keep currentBottomSheetPostId in sync so the id onSaveInstanceState reads survives a second restore. Also makes PrepublishingViewModel.start()'s hasPost parameter required, so the guard can't be bypassed by omission, and corrects the comment: the guard dismisses on every restore path but does not stop the saved child fragment from being restored and started first, since the dismissal is async.
oguzkocer
left a comment
oguzkocer
left a comment
There was a problem hiding this comment.
Looks good to me and works as expected. 
Claude being Claude, it shared a few ideas — I feel it always has something to say 🤷♂️ . I have discarded most because they're out of scope for this PR, but one might be worth considering, so I marked it as optional and shared below.
Even for that one, although I see Claude's point, I prefer the current implementation because it makes the function argument limited in scope and that in my opinion is more important than consistency.
Optional — hasPost: Boolean puts the invariant in the caller
WordPress/src/main/java/org/wordpress/android/ui/posts/prepublishing/PrepublishingViewModel.kt:66
Every sibling prepublishing ViewModel (PrepublishingHomeViewModel, PrepublishingTagsViewModel, PrepublishingCategoriesViewModel, PrepublishingPublishSettingsViewModel) receives the EditPostRepository. A snapshot boolean makes the guard only as good as the caller's getEditorHook().editPostRepository.hasPost(), and a future caller passing true disables it silently. PrepublishingBottomSheetFragment already resolves the hook, so passing the repository keeps the check inside the ViewModel and matches the rest of the package.
I agree with that. Thanks for the review! |
4 participants
Description
Fixes JETPACK-ANDROID-1EVE — 24 users, live on 26.9.
The Prepublishing sheet is a
DialogFragment, so the FragmentManager restores it after a config change or process death even when the host has no post loaded, and it then builds its UI against an emptyEditPostRepository.The guard goes in
PrepublishingViewModel.start(), which every restore passes through, so it covers whichever screen was saved inKEY_SCREEN_STATE— Tags, Categories and Publish crash on first interaction too, not just Home.PrepublishingHomeViewModelalso returns early, since that dismissal is async and the FragmentManager can restore the fragment before it commits.Testing instructions
With Developer options → Don't keep activities on, open the Prepublishing sheet from a post, background the app, then return. Repeat from the Tags, Categories and Publish screens.
trunk) and the sheet closes.