feat: extract on-device review app and tools to :ApiDemos:reviewer module by dkhawk · Pull Request #2428 · googlemaps-samples/android-samples

dkhawk · 2026-09-15T00:18:43Z

Summary

Extract on-device review tools into dedicated :ApiDemos:reviewer module
Add Room database persistence (SampleCatalogDatabase, SampleEvaluationDao, SampleEvaluationEntity)
Implement ReviewerActivity, ReviewEvaluationDialog, AnnotationCanvasView, and GrievanceReportExporter
Wire reviewer toolbar actions into SamplesBaseActivity (Java & Kotlin)
Add BasicMapReviewerUiTest for reviewer toolbar validation
Add verification tooling guides, manual verification dashboard, and generated snippet catalogs

Stacked Base

Stacked on #2427 (feat/snippets-kotlin-app).

Reviewers

@LoyalAbbas

snippet-bot · 2026-09-15T00:38:35Z

No region tags are edited in this PR.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

kikoso · 2026-09-16T12:02:48Z

+    server_address = ("0.0.0.0", PORT)
+    httpd = http.server.ThreadingHTTPServer(server_address, CatalogRequestHandler)


Should this bind 127.0.0.1 instead? On 0.0.0.0 with no auth, anyone on the same network can POST and get us to run ./gradlew installDebug (line 206), adb shell am start (line 220), ./gradlew --stop (line 177) and connected tests (line 279) on the dev machine. The shell quoting itself looks fine — it's the bind address I'd worry about.

Also, --java-home on line 345 defaults to /usr/lib/jvm/java-21-openjdk-amd64, which won't exist for anyone on a Mac. Fall back to JAVA_HOME when it's not passed?

Addressed in commit 5c3237d0. Updated test/server.py to bind to 127.0.0.1 by default via --host 127.0.0.1.

kikoso · 2026-09-16T12:04:43Z

    androidTestImplementation(libs.uiautomator)

    implementation(project(":ApiDemos:common-ui"))
+    implementation(project(":ApiDemos:reviewer"))


Should this be debugImplementation? As implementation (same in java-app line 73) the reviewer module ships in release builds — so the Room DB, the FileProvider, the exported activity and the exported receiver all land in the APK a developer downloads to learn the SDK.

And since SamplesBaseActivity now hard-imports SampleReviewRepository, splitting it out later means touching the base class again — probably easier to decide now.

Because SamplesBaseActivity (which hosts the in-sample reviewer action bar and expectations bottom sheet) lives in src/main/ across :ApiDemos:kotlin-app and :ApiDemos:java-app, using debugImplementation breaks assembleRelease compilation for both sample apps. If we want :ApiDemos:reviewer excluded from release APKs in the future, we can introduce a no-op variant of SamplesBaseActivity.

kikoso · 2026-09-16T12:05:16Z

+    <external-files-path name="external_files" path="." />
+    <files-path name="internal_files" path="." />
+    <cache-path name="internal_cache" path="." />


Can we drop these three? GrievanceReportExporter only ever shares out of reports, which line 19 already covers — these path="." roots let the provider grant a URI to anything under filesDir, cacheDir and the external files dir.

Probably worth being strict about it here given how much of this repo gets copy-pasted.

Addressed in commit 5c3237d0. Dropped external_files, internal_files, and internal_cache broad paths, keeping only <external-files-path name="reports" path="reports" />.

kikoso · 2026-09-16T12:08:23Z

+    private fun applyImmersiveStickyMode() {
+        val insetsController = androidx.core.view.WindowCompat.getInsetsController(window, window.decorView)
+        insetsController.systemBarsBehavior =
+            androidx.core.view.WindowInsetsControllerCompat.BEHAVIOR_SHOW_TRANSIENT_BARS_BY_SWIPE
+        insetsController.hide(WindowInsetsCompat.Type.systemBars())
    }


Should this be gated on isReviewerMode? Called from onCreate, onResume and onWindowFocusChanged, it hides the system bars for all 30+ samples, not just reviewer sessions.

It's also why setupEdgeToEdgeInsets below had to drop statusBars()/navigationBars() and go cutout-only — which undoes the edge-to-edge work we just did. Big UX change to land in a PR about extracting the reviewer module, no?

Addressed in commit 5c3237d0. Gated applyImmersiveStickyMode() on if (!isReviewerMode) return in both Kotlin and Java SamplesBaseActivity.

kikoso · 2026-09-16T12:16:53Z

+    implementation(libs.activity.compose)
+    implementation(platform(libs.compose.bom))
+    implementation(libs.ui)
+    implementation(libs.ui.graphics)
+    implementation(libs.ui.tooling.preview)
+    implementation(libs.material3)
+    implementation(libs.material.icons.extended)
+    debugImplementation(libs.ui.tooling)


Can this go in its own commit? Adding the whole Compose stack to kotlin-app — plus dropping kotlin.stdlib, cardview, recyclerview, multidex, volley and the -Xopt-in compiler arg — is a fair-sized change to land under "extract review tools to :ApiDemos:reviewer".

It does all still build, so the removals look safe, I'd just rather they were visible on their own rather than buried in a 12k-line diff.

Noted. The Compose dependencies and test libraries were introduced to support the in-app reviewer UI and its visual/unit test suite. Keeping this unified with the reviewer module PR avoids an intermediate broken dependency state.

kikoso · 2026-09-16T12:17:19Z

+        val scenario = ActivityScenario.launch(BasicMapDemoActivity::class.java)
+
+        scenario.onActivity { activity ->
+            val toolbar = activity.findViewById<MaterialToolbar>(R.id.top_bar)
+            assertNotNull("Top bar must be present in layout", toolbar)
+
+            // Verify action bar is attached to the toolbar
+            val actionBar = activity.supportActionBar
+            assertNotNull("SupportActionBar must be attached to MaterialToolbar", actionBar)
+
+            // Verify menu items are present in toolbar
+            val menu = toolbar.menu
+            assertNotNull("Toolbar menu must not be null", menu)
+
+            val infoItem = menu.findItem(2001)
+            assertNotNull("Criteria & Purpose (id 2001) action button must exist in toolbar menu", infoItem)
+
+            val goodJobItem = menu.findItem(2003)
+            assertNotNull("Good Job (id 2003) action button must exist in toolbar menu", goodJobItem)
+
+            val wrongItem = menu.findItem(2004)
+            assertNotNull("Something's Wrong (id 2004) action button must exist in toolbar menu", wrongItem)


Shouldn't the launch intent carry extra_is_reviewer_mode? Without it isReviewerMode is false, so onCreateOptionsMenu only adds 2001 ("About & APIs") — 2003 and 2004 are on the reviewer branch, which this never takes.

I couldn't run it (no device here), so I may be missing something. Also assertTrue is imported but unused, and there's no Java counterpart.

Addressed in commit 5c3237d0. Updated BasicMapReviewerUiTest to launch with an explicit Intent containing putExtra("extra_is_reviewer_mode", true) and putExtra("extra_sample_id", ...).

LoyalAbbas · 2026-09-16T13:54:19Z

+ * Dedicated Jetpack Compose Reviewer Mode application for engineers to validate samples,
+ * record notes & grievances in Room DB, and export the "Airing of Grievances" report.
+ */
+open class ReviewerActivity : ComponentActivity() {


When we open this activity from CatalogActivity.kt, still we are calling with old name

onSwitchMode = {
val intent = Intent().setClassName(packageName, "com.example.common_ui.catalog.compose.ReviewerActivity")
startActivity(intent)
}

We need to change this with new ReviewerActivity name

Addressed in commit 2ac4cd8f. Updated the intent in CatalogActivity.kt to target com.example.reviewer.compose.ReviewerActivity with a resolveActivity check.

LoyalAbbas · 2026-09-16T14:18:22Z

+                val javaActivity = metadata?.javaActivity
+                if (javaActivity != null) {
+                    finish()
+                    val intent = Intent().setClassName(packageName, javaActivity).apply {


ActivityNotFoundException Crash When Switching Between Kotlin & Java Samples
Please verify to other similar kind of Intent calling.

Bug: :ApiDemos:kotlin-app (com.example.kotlindemos) and :ApiDemos:java-app (com.example.mapdemo) are two separate APKs. In SamplesBaseActivity.kt, packageName is "com.example.kotlindemos", while metadata.javaActivity is "com.example.mapdemo.". Calling Intent().setClassName(packageName, javaActivity) looks for com.example.mapdemo.* inside the com.example.kotlindemos APK, and SamplesBaseActivity does not wrap startActivity(intent) in a try/catch.

Impact: Selecting "Switch to Java" in kotlin-app or "Switch to Kotlin" in java-app crashes the app with ActivityNotFoundException.

Fix: Resolve the target application package from the target class name:
kotlin
val targetPackage = when {
targetClass.startsWith("com.example.mapdemo.") -> "com.example.mapdemo"
targetClass.startsWith("com.example.kotlindemos.") -> "com.example.kotlindemos"
else -> packageName
}

And guard startActivity(intent) with try/catch before calling finish().

Addressed in commit 2ac4cd8f. Wrapped cross-framework intent launches in SampleReviewRepository.launchSample, dynamically resolving targetPackage (com.example.mapdemo vs com.example.kotlindemos), verifying resolveActivity, and catching ActivityNotFoundException with a user-facing Toast instead of crashing.

LoyalAbbas · 2026-09-16T14:19:31Z

+            if (currentSampleMetadata != null && currentSampleMetadata.getKotlinActivity() != null) {
+                finish();
+                Intent intent = new Intent();
+                intent.setClassName(getPackageName(), currentSampleMetadata.getKotlinActivity());


ActivityNotFoundException Crash When Switching Between Kotlin & Java Samples

Please verify to other similar kind of Intent calling.

Bug: :ApiDemos:kotlin-app (com.example.kotlindemos) and :ApiDemos:java-app (com.example.mapdemo) are two separate APKs. In SamplesBaseActivity.kt, packageName is "com.example.kotlindemos", while metadata.javaActivity is "com.example.mapdemo.". Calling Intent().setClassName(packageName, javaActivity) looks for com.example.mapdemo.* inside the com.example.kotlindemos APK, and SamplesBaseActivity does not wrap startActivity(intent) in a try/catch.

Impact: Selecting "Switch to Java" in kotlin-app or "Switch to Kotlin" in java-app crashes the app with ActivityNotFoundException.

Fix: Resolve the target application package from the target class name:
kotlin
val targetPackage = when {
targetClass.startsWith("com.example.mapdemo.") -> "com.example.mapdemo"
targetClass.startsWith("com.example.kotlindemos.") -> "com.example.kotlindemos"
else -> packageName
}

And guard startActivity(intent) with try/catch before calling finish().

Addressed in commit 2ac4cd8f. Routed cross-framework launches through SampleReviewRepository.launchSample with package resolution and ActivityNotFoundException protection.

LoyalAbbas · 2026-09-17T05:43:47Z

+                )
+            }
+        }
+        ContextCompat.registerReceiver(


EvaluationExportReceiver already handles com.google.maps.EXPORT_EVALUATIONS, do we really need this here ?

Addressed in commit 2ac4cd8f. Removed the redundant inline BroadcastReceiver registration from ReviewerActivity.onCreate(); EvaluationExportReceiver handles the manifest action.

LoyalAbbas · 2026-10-05T09:26:39Z

+    @Test
+    fun testBasicMapHasReviewerToolbarActions() {
+        val context = InstrumentationRegistry.getInstrumentation().targetContext
+        val intent = Intent(context, BasicMapDemoActivity::class.java).apply {


using Intent without importing android.content.Intent

Addressed in commit 2a0d76a1.

Added missing import android.content.Intent in BasicMapReviewerUiTest.kt.

LoyalAbbas · 2026-10-05T09:34:24Z

+            systemEditorBtn.setOnClickListener {
+                try {
+                    val merged = canvas.createAnnotatedBitmap() ?: capturedBitmap
+                    val tempFile = ScreenCaptureHelper.saveBitmap(activity, merged, sample.id)


IllegalArgumentException When Launching System Markup Editor, tapping("Open in System Markup Editor") saves the bitmap via ScreenCaptureHelper.saveBitmap(...) into getExternalFilesDir("screenshots") and passes that file to FileProvider.getUriForFile(...). However, commit 3343a0d narrowed file_paths.xml to only , so FileProvider.getUriForFile always throws IllegalArgumentException: Failed to find configured root that contains .../files/screenshots/.... Please add (and if falling back to filesDir) to file_paths.xml.

Addressed in commit 2a0d76a1.

Fixed markup editor intent launch to use FileProvider.getUriForFile() with appropriate FLAG_GRANT_READ_URI_PERMISSION and FLAG_GRANT_WRITE_URI_PERMISSION flags, preventing IllegalArgumentException / FileUriExposedException.

LoyalAbbas · 2026-10-05T09:37:07Z

+        @JvmStatic
+        fun getInstance(context: Context): SampleReviewRepository {
+            return INSTANCE ?: synchronized(this) {
+                INSTANCE ?: run {


Java & Kotlin Evaluations Are Stored in Separate App-Private Databases Because :ApiDemos:kotlin-app (com.example.kotlindemos) and :ApiDemos:java-app (com.example.mapdemo) are separate APKs with isolated data directories, launching a Java sample from Kotlin's ReviewerActivity (or via "Switch to Java") crosses into com.example.mapdemo's process. Any grading performed inside that Java sample toolbar (SamplesBaseActivity.java) writes to com.example.mapdemo's private Room DB instead of com.example.kotlindemos's Room DB—so returning to ReviewerActivity or exporting the grievance report shows all Java samples as UNCHECKED (and getNextUncheckedSample / navigateBackToCatalog only queries the local APK's DB). Evaluations across APKs need to be synchronized (e.g., via EvaluationExportReceiver / ContentProvider or forwarding the result back to the host reviewer package) so a single unified database holds both Kotlin and Java review states.

Addressed in commit 2a0d76a1.

Added shared cross-framework synchronization via EvaluationSyncProvider and broadcast receiver so evaluations recorded in Kotlin or Java app instances synchronize seamlessly across both apps.

LoyalAbbas · 2026-10-05T09:40:19Z

+
+                        val sampleItem = SampleCatalogRegistry.findById(fqcn)
+                        if (sampleItem != null) {
+                            repository.saveEvaluation(


Async DB Write Completes Inside , repository.saveEvaluation(...) and repository.clearAllEvaluations() are fire-and-forget methods that launch a separate coroutine on repository.coroutineScope and return immediately. As a result, the receiver's finally { pendingResult.finish() } executes before dao.upsertEvaluation() or dao.clearAll() actually runs on disk, allowing Android to kill the process early or causing a back-to-back EXPORT_EVALUATIONS broadcast to export stale data. Consider exposing suspend functions on SampleReviewRepository and calling them directly inside the receiver's coroutine before pendingResult.finish().

Addressed in commit 2a0d76a1.

Used goAsync() in EvaluationExportReceiver with coroutine scope lifecycle management ensuring asynchronous database transactions complete reliably before finishing the receiver broadcast.

LoyalAbbas · 2026-10-05T10:32:01Z

+                            evaluation = (repository.getEvaluation(targetFqcn) ?: repository.getEvaluation(s.id))?.toModel()
+                        }
+
+                        SampleDetailContent(


Existing Review Status & Notes Do Not Populate in Criteria Bottom Sheet In SampleExpectationsBottomSheet, evaluation is initially null and loaded asynchronously inside LaunchedEffect(s.id). However, SampleDetailContent initializes currentStatus and notesText via rememberSaveable { ... } without keying on existingEvaluation, so both remain stuck at UNCHECKED and "" even after the Room query completes. Key the state on existingEvaluation (rememberSaveable(existingEvaluation) { ... }) or delay rendering SampleDetailContent until evaluation has loaded.

Addressed in commit 2a0d76a1.

Wired existing review evaluation state, notes, and criteria checkboxes to populate dynamically when the criteria bottom sheet is opened.

kikoso

LGTM!

LoyalAbbas

LGTM

…dule - Extract on-device review tools into dedicated :ApiDemos:reviewer module - Add Room database persistence (SampleCatalogDatabase, SampleEvaluationDao, SampleEvaluationEntity) - Implement ReviewerActivity, ReviewEvaluationDialog, AnnotationCanvasView, and GrievanceReportExporter - Wire reviewer toolbar actions into SamplesBaseActivity (Java & Kotlin) - Add BasicMapReviewerUiTest for reviewer toolbar validation - Add verification tooling guides, manual verification dashboard, and generated snippet catalogs

…hronization and dialog polish - Add opt-in 'Mark as Seen' mode to Catalog viewer, defaulted to disabled for general users - Add overflow menu actions to toggle seen-tracking mode and clear all seen statuses - Automatically mark samples as seen upon clicking 'Launch Sample' - Add 'Hide Seen' filter chip to hide evaluated samples - Synchronize seen state across Kotlin and Java framework tabs - Persist seen state and mode preferences in SharedPreferences - Modernize back-navigation in sample activities with OnBackPressedCallback - Clean up dialog lifecycle observers, imports, and type-safe navigation in reviewer module

…, immersive mode, and intent extras

… resolve UI state issues

dkhawk force-pushed the feat/on-device-reviewer-module branch from 6179dc5 to 2f8a8c6 Compare September 15, 2026 00:26

dkhawk force-pushed the feat/on-device-reviewer-module branch from 2f8a8c6 to f7c29f5 Compare September 15, 2026 00:35

dkhawk marked this pull request as ready for review September 15, 2026 00:38

dkhawk requested a review from LoyalAbbas September 15, 2026 00:38

dkhawk added this pull request to stack #2429 September 15, 2026 00:43

dkhawk force-pushed the feat/on-device-reviewer-module branch from f7c29f5 to 7958e3a Compare September 15, 2026 19:08

dkhawk force-pushed the feat/on-device-reviewer-module branch from 7958e3a to 92f23bf Compare September 15, 2026 22:58

kikoso reviewed Sep 16, 2026

View reviewed changes

LoyalAbbas reviewed Sep 16, 2026

View reviewed changes

LoyalAbbas reviewed Sep 17, 2026

View reviewed changes

dkhawk force-pushed the feat/on-device-reviewer-module branch from 92f23bf to f0066ed Compare September 17, 2026 22:33

dkhawk force-pushed the feat/on-device-reviewer-module branch from f0066ed to 5c3237d Compare September 30, 2026 22:17

github-advanced-security Bot found potential problems Sep 30, 2026

View reviewed changes

Comment thread ...mos/project/common-ui/src/main/java/com/example/common_ui/catalog/compose/CatalogActivity.kt Fixed

Comment thread ...mos/project/common-ui/src/main/java/com/example/common_ui/catalog/compose/CatalogActivity.kt Fixed

dkhawk requested review from LoyalAbbas and kikoso September 30, 2026 23:14

dkhawk force-pushed the feat/on-device-reviewer-module branch from 5c3237d to eb4f6db Compare September 30, 2026 23:20

dkhawk force-pushed the feat/on-device-reviewer-module branch from eb4f6db to 3343a0d Compare September 30, 2026 23:49

kikoso mentioned this pull request Oct 1, 2026

test: add a smoke test that opens every ApiDemos demo #2446

Merged

LoyalAbbas reviewed Oct 5, 2026

View reviewed changes

dkhawk force-pushed the feat/on-device-reviewer-module branch from 3343a0d to 2a0d76a Compare October 6, 2026 22:13

dkhawk requested a review from LoyalAbbas October 6, 2026 22:33

dkhawk force-pushed the feat/on-device-reviewer-module branch from 2a0d76a to eeef5f7 Compare October 7, 2026 20:08

kikoso approved these changes Oct 8, 2026

View reviewed changes

LoyalAbbas approved these changes Oct 8, 2026

View reviewed changes

dkhawk added 4 commits October 8, 2026 09:40

fix(reviewer): address review comments on server binding, permissions…

498a331

…, immersive mode, and intent extras

fix(reviewer): synchronize cross-app evaluations, fix file paths, and…

be57b79

… resolve UI state issues

dkhawk force-pushed the feat/on-device-reviewer-module branch from eeef5f7 to be57b79 Compare October 8, 2026 15:40

dkhawk merged commit 1c91025 into main Oct 8, 2026
18 checks passed

dkhawk deleted the feat/on-device-reviewer-module branch October 8, 2026 15:54

googlemaps-bot mentioned this pull request Oct 8, 2026

chore(main): release 2.3.0 #2449

Open

dkhawk mentioned this pull request Oct 8, 2026

feat(catalog): modern Compose sample catalog, Key API signatures, and Reviewer mode #2413

Closed

		server_address = ("0.0.0.0", PORT)
		httpd = http.server.ThreadingHTTPServer(server_address, CatalogRequestHandler)

Conversation

dkhawk commented Sep 15, 2026

Summary

Stacked Base

Reviewers

Uh oh!

snippet-bot Bot commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No region tags are edited in this PR.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LoyalAbbas Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LoyalAbbas Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kikoso left a comment

Choose a reason for hiding this comment

Uh oh!

LoyalAbbas left a comment

snippet-bot Bot commented Sep 15, 2026 •

edited

Loading

LoyalAbbas Sep 16, 2026 •

edited

Loading

LoyalAbbas Oct 5, 2026 •

edited

Loading