Skip to content

Conversation

MohitMaliFtechiz
Copy link
Collaborator

@MohitMaliFtechiz MohitMaliFtechiz commented Sep 2, 2025

Issue seen in #4405

  • Fixed: testPreviouslyLoadedArticleLoadsAgainWhenSwitchingToAnotherScreen failing in Custom apps.
  • The failure was caused by immutable arguments in navBackStackEntry that could not be cleared. To work around this earlier, we replaced the reader fragment with a new one before navigating to another screen so that arguments could be reset. However, this approach had side effects: reopening the same fragment in the background triggered extra computation, since everything in the reader screen renders on the IO thread. As a result, the previously opened page was sometimes only partially rendered, and when the test navigated back, it failed to load correctly.
  • To resolve this, we have completely removed the use of navArguments, as they made handling different scenarios more difficult. Instead, we now rely on the savedStateHandle and set arguments directly on the backStackEntry. These can be cleared immediately after use, ensuring they don’t affect other scenarios. This approach not only fixes the failing test but also eliminates redundant fragment rendering, reducing unnecessary work on the IO thread.
  • Refactored the test cases according to savedStateHandle approach.

…reen` failing in Custom apps.

* The failure was caused by immutable arguments in navBackStackEntry that could not be cleared. To work around this earlier, we replaced the reader fragment with a new one before navigating to another screen so that arguments could be reset. However, this approach had side effects: reopening the same fragment in the background triggered extra computation, since everything in the reader screen renders on the IO thread. As a result, the previously opened page was sometimes only partially rendered, and when the test navigated back, it failed to load correctly.
* To resolve this, we have completely removed the use of navArguments, as they made handling different scenarios more difficult. Instead, we now rely on the savedStateHandle and set arguments directly on the backStackEntry. These can be cleared immediately after use, ensuring they don’t affect other scenarios. This approach not only fixes the failing test but also eliminates redundant fragment rendering, reducing unnecessary work on the IO thread.
Copy link

codecov bot commented Sep 2, 2025

Codecov Report

❌ Patch coverage is 54.54545% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.49%. Comparing base (28964f5) to head (941a2e3).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...r/fileselectView/effects/OpenFileWithNavigation.kt 0.00% 4 Missing ⚠️
...rg/kiwix/kiwixmobile/core/main/CoreMainActivity.kt 0.00% 4 Missing ⚠️
...bile/nav/destination/reader/KiwixReaderFragment.kt 70.00% 0 Missing and 3 partials ⚠️
...va/org/kiwix/kiwixmobile/main/KiwixMainActivity.kt 66.66% 2 Missing ⚠️
.../destination/library/local/LocalLibraryFragment.kt 75.00% 0 Missing and 1 partial ⚠️
.../kiwixmobile/core/extensions/ActivityExtensions.kt 0.00% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (54.54%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4408      +/-   ##
============================================
+ Coverage     57.25%   57.49%   +0.23%     
- Complexity     1454     1463       +9     
============================================
  Files           328      328              
  Lines         16859    16809      -50     
  Branches       2061     2057       -4     
============================================
+ Hits           9653     9664      +11     
+ Misses         5856     5796      -60     
+ Partials       1350     1349       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@MohitMaliFtechiz MohitMaliFtechiz marked this pull request as ready for review September 2, 2025 17:04
@kelson42 kelson42 merged commit ef453cc into main Sep 2, 2025
34 of 35 checks passed
@kelson42 kelson42 deleted the fixed_many_scenario_of_reader_screen branch September 2, 2025 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants