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.

…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

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 added 3 commits September 30, 2026 17:47

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

3343a0d

…, immersive mode, and intent extras

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

		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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

snippet-bot Bot commented Sep 15, 2026 •

edited

Loading

LoyalAbbas Sep 16, 2026 •

edited

Loading