area: upload: Fix duplicate validation bypass and thread starvation

[Lame-Fortuna]

Description
Fixes #6401
Duplicate validation was silently failing on fresh multi-uploads. The ViewPager was skipping per-image checks, and concurrent SHA-1 lookups were starving the IO thread. This PR ensures images are validated exactly when the user swipes to them.

What changes did you make and why?

• IO Thread Starvation: Swapped .blockingGet() for .await() in CustomSelectorUtils.kt to unblock the coroutine during SHA-1 network lookups.
• Per-Image Validation: Overloaded checkImageQuality in UploadMediaDetailsContract and UploadMediaPresenter to allow single-parameter (index) validation without breaking existing call sites.
• Lifecycle Trigger: Called checkImageQuality in onBecameVisible (UploadMediaDetailFragment) so validation fires reliably on each ViewPager swipe.

Additional Notes / Out of Scope:

• Delayed popups: Duplicate warnings sometimes arrive late via the network, causing stacked popups. Left this sync/race condition out of scope to keep this PR focused.
• Context leak: Found a static context leak with UploadMediaPresenter.presenterCallback while debugging. Leaving it untouched here as it's already tracked in Fix potential Context leak and remove StaticFieldLeak suppression in UploadController #6863.

Tests performed (required)
Tested prodDebug on XI M2101K7BI (API 33).
Selected multiple existing Commons images and verified the duplicate popup triggers correctly for each specific image as it becomes visible in the ViewPager.

Screenshots (for UI changes only)
Before

After

[RitikaPahwa4444]

@Lame-Fortuna are all these changes absolutely necessary to solve the linked issue? If not, could you please keep the diff minimal, it helps review the changes quickly. Thanks!

[RitikaPahwa4444]

Also, could you please add the Before and After screencasts to the PR description?

[Lame-Fortuna]

@RitikaPahwa4444 Well, these changes are necessary for unblocking the thread and triggering checks for duplicates for all images:

swapping .blockingGet() for .await() in CustomSelectorUtils.kt

adding the new method in UploadMediaPresenter.kt and UploadMediaDetailsContract.kt, and triggering it via onBecameVisible in UploadMediaDetailFragment.kt

The changes in strings.xml and the step_count string were just fixing linting issues and typos. Let me know if you want me to revert those.

P.S. Added the screenshots

[RitikaPahwa4444]

The changes in strings.xml and the step_count string were just fixing linting issues and typos. Let me know if you want me to revert those.

Yes please, we can take it up separately 🙂

[Copilot]

Pull request overview

This PR targets the upload flow’s duplicate/quality validation reliability during multi-image uploads by reducing IO thread blocking during SHA-1 existence checks and by triggering per-item validation when a ViewPager page becomes visible.

Changes:

• Replaced an RxJava blockingGet() with coroutine await() during SHA-1 existence checks to avoid blocking threads.
• Extended the media details contract/presenter with verifyImageQuality(index) and invoked it from UploadMediaDetailFragment.onBecameVisible().
• Minor contract comment cleanup and added lint suppression around duplicate popup formatting.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
app/src/main/java/fr/free/nrw/commons/utils/CustomSelectorUtils.kt	Switches SHA-1 duplicate lookup from blocking to suspending await.
app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaPresenter.kt	Adds verifyImageQuality API and implementation to re-run stored quality checks by index.
app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailsContract.kt	Exposes verifyImageQuality(index) on the presenter contract.
app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailFragment.kt	Triggers validation on page visibility, but also removes step title binding and adds a new lint suppression.

Comments suppressed due to low confidence (2)

app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaDetailFragment.kt:244

• initializeFragment() no longer sets tvTitle (step count/title), and there doesn’t appear to be any other assignment in this fragment. This likely removes the step indicator from the UI. Please restore setting tvTitle (and fix the underlying string formatting issue in resources if that’s why it was removed).

    internal fun initializeFragment() {
        if (_binding == null) {
            return
        }
        binding.tooltip.setOnClickListener {
            showInfoAlert(
                R.string.media_detail_step_title,
                R.string.media_details_tooltip
            )
        }

app/src/main/java/fr/free/nrw/commons/utils/CustomSelectorUtils.kt:96

• Now that this uses await(), coroutine cancellation will throw CancellationException, which is currently caught by the broad catch (e: Exception) and converted into ImageLoader.Result.ERROR. That can break structured cancellation and lead to work continuing after the calling coroutine is cancelled. Consider rethrowing CancellationException (or catching it separately) and only mapping non-cancellation failures to ERROR.

                } catch (e: Exception) {
                    if (e is UnknownHostException) {
                        Timber.e(e, "Network Connection Error")
                    }
                    result = ImageLoader.Result.ERROR
                    e.printStackTrace()
                }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Lame-Fortuna]

@RitikaPahwa4444 Reverted those unnecessary changes and applied those Copilot suggestions.

[RitikaPahwa4444]

@Lame-Fortuna do you have any performance improvements stats or profiling data for the the thread starvation that you've fixed for the custom selector?

The linked issue is for the normal selector, I tested it out and it works great :)

I'll have to test it out for the custom selector, are there any steps I could follow in case there's visible improvement?

[github-actions]

✅ Generated APK variants!

• 🤖 Download prodDebugAPK
• 🤖 Download betaDebugAPK

[Lame-Fortuna]

@RitikaPahwa4444 No, I do not have exact performance stats or profiling data.

The change was a preventative fix, swapping .blockingGet() for .await() stops the underlying IO thread from freezing during concurrent SHA-1 network lookups and allows the coroutine to suspend properly in the Custom Selector.

If needed, testing it by selecting a large batch of images should feel noticeably more responsive. However, there is still the issue with delayed popups (duplicate warnings sometimes arriving late via the network and stacking). I left that specific sync/race condition out of scope of this PR, as that requires digging deeper.

[chrisdebian]

Traced this carefully since the title specifically calls out a duplicate-validation bypass, and I think there's a real gap left in the fix for image index 0.

onBecameVisible() is only ever invoked as fragments[index +/- 1].onBecameVisible() from UploadActivity (confirmed by checking every call site) — it fires when navigating to a page, never for the page that's already showing when the ViewPager first appears. So index 0 never gets this hook at all, which is presumably why the new code explicitly skips it (if (indexOfFragment > 0)).

The only other place checkImageQuality(0) is called is inside UploadActivity.showAlertDialogForCategories()'s positive-button handler. That dialog only shows when:

• more than one file is being uploaded, and
• hasAlreadyLaunchedCategoriesDialog is false in the KV store — which becomes permanently true the first time a user checks the dialog's "don't show again" checkbox, so it never shows again for that user on any future multi-upload, and
• the user actually presses the labelled "OK" button — the negative button is unlabelled (setNegativeButton("", null)) and dismisses without calling checkImageQuality(0), same as back-press or tapping outside.

So for a user who's ever dismissed that dialog permanently (or who dismisses it via anything other than OK on a given upload), image index 0 in every subsequent multi-upload skips quality/duplicate validation entirely — the exact bug class this PR is fixing, just left in place for the first image specifically.

Given the fix already threads an index through cleanly, might be simplest to just drop the indexOfFragment > 0 guard and always call checkImageQuality(indexOfFragment) in onBecameVisible() — worth checking whether that double-checks index 0 in the "dialog OK pressed" case, but a redundant check seems safer than a silently-skipped one.

[Lame-Fortuna]

Thanks for the review @chrisdebian, and I agree with redundant check part. I'll update the PR soon.

[Lame-Fortuna]

@chrisdebian The PR has been updated to remove the index 0 guard as suggested.