Make scanner lifecycle explicit and production-safe - #32
Conversation
|
Follow-up fixes are pushed at 054e55c. This head restores manual-install routing, preserves surviving manual scanners, blocks queued resume callbacks after uninstall, gates trusted triggers in the documentation, and adds regression coverage. Validation passed: :scanner-ui:test, :scanner-ui:lint, :sample:assembleRelease, and git diff --check. Please re-review the latest head. |
mohdaquib
left a comment
There was a problem hiding this comment.
Re-reviewed the latest PR head (63f521f). The manual-install fallback and queued-resume/uninstall race identified in the earlier review are now addressed by selectEntry() and the pending/suppressed lifecycle state.
Two remaining recommendations:
-
Medium — make trusted-build gating fail closed. The README example
endpoint != PRODenables the scanner for unknown, uninitialized, or newly introduced endpoint values. Since this path ships scanner code in a non-debuggable app, prefer a positive allowlist (for example, an internal build flavor and an explicitly trusted endpoint). Where possible, recommendinternalImplementation/src/internalinstead of broadimplementation/src/mainplacement. -
Test coverage — add integration coverage around the helper. The new unit tests validate
ScannerLifecycleandselectEntry()in isolation, but an Activity/Robolectric or instrumentation test should verify that a posted resume cannot recreate a detached overlay and that manualinstall()remains the target oftriggerScan()/notifyScreenChanged()after automatic entries pause or are removed.
|
Final two Medium findings are fixed at fd33739: repeated manual install is again a true no-op, and built-in long-press/shake callbacks atomically no-op when scanner permission has been disabled. Tests, lint, release assembly, and diff checks pass. Please review the latest head. |
mohdaquib
left a comment
There was a problem hiding this comment.
Follow-up on the latest head (fd33739): the repeated-install routing fix and the new disabled-trigger no-op behavior look valid. Two earlier review requests remain unresolved:
-
Medium — use fail-closed trusted-build gating.
README.mdstill showstoggleScanner(enabled = endpoint != PROD). Unknown, uninitialized, or newly added endpoint values therefore enable the scanner. Please use a positive allowlist and prefer an internal flavor/source set, for exampleinternalImplementationwithsrc/internal, rather than broadly recommendingimplementation/src/main. -
Test coverage — add integration-level lifecycle/routing coverage. The existing tests exercise
ScannerLifecycleandselectEntry()in isolation, but do not verify the actual Activity/overlay/controller wiring. Please cover queued resume after explicit uninstall, manual routing throughtriggerScan()/notifyScreenChanged(), automatic-to-manual fallback, and detach cleanup. The latest commit also needs regression tests proving repeatedinstall()does not steal active routing and default long-press/shake triggers quietly no-op after disable or before installation.
|
@mohdaquib Explicit follow-up to your review #32 (review):
The latest head is |
|
@mohdaquib Explicit follow-up to your review #32 (review): Both remaining requests are addressed on the current head:
The final behavioral contract is now documented in the PR body and README: untouched policy follows build defaults, |
Summary
toggleScanner(enabled)policy for trusted non-debuggable usefalseoverride the debuggable default, remove every overlay, and block installation and scan APIsuninstall()cleanup-safe and idempotent after disableScanner availability contract
toggleScanner()calltoggleScanner(true)toggleScanner(false)toggleScanner(false)is a runtime control: it removes overlays and blocks scanner APIs while retaining lightweight AndroidX Startup callbacks and resumed-activity tracking so re-enable can install immediately. Variants requiring no scanner startup, metadata reads, lifecycle callbacks, or activity tracking can removeA11yScannerInitializerfrom the merged manifest as documented in the README.Because AndroidX Startup runs before
Application.onCreate, policies that default to disabled must calltoggleScanner(false)synchronously fromApplication.onCreatebefore the first activity resumes. Asynchronous endpoint or remote-config resolution is explicitly discouraged.Safety and compatibility
toggleScanner(false)is an intentional behavior correction relative to fork release2.1.0-rt.3: debug scanners now respect the explicit disableuninstall()now always performs main-thread cleanup, even after scanner permission is disabledValidation
testlintcreateDebugUnitTestCoverageReportscanner-coreandscanner-rules(required 80%):sample:assembleReleasedokkaGeneratepublishToMavenLocalwithJITPACK_TAG=2.1.0-rt.42.1.0-rt.4for all modules and inter-module dependenciesgit diff --checkFinal isolated Gradle validation completed successfully: 458 actionable tasks.
Review feedback addressed