# Implementation Plan — Address PR Review Comments (Issue #111)

We will address the issues highlighted in the review comments on PR #112.

## Proposed Changes

### Build Configuration

#### [MODIFY] [build.gradle.kts](file:///home/Messier82/projects/service-book/app/build.gradle.kts)
- Revert `additionalTestOutputDir` back to `/sdcard/test-outputs/smoke` in `defaultConfig`. This keeps the default/smoke auto-pull root isolated and unchanged, preventing any pollution.
- Remove variables `isStoreScreenshotInvocation`, `isPixel2SmokeInvocation`, `storeScreenshotBucket`, and `requestedTaskNames` from the top of the build file.
- Configure `testInstrumentationRunnerArguments` dynamically for each `ManagedDeviceInstrumentationTestTask` in a `configureEach` block (using reflection to access the internal `getInstrumentationRunnerArguments()` `MapProperty` on `TestData`):
  - For `phonePixel6api34`, `tablet7Nexus7api34`, and `tablet10PixelTabletApi34`: set `package` to `"com.yorvana.screenshots"`, `storeScreenshotBucket` to the corresponding bucket name, and `additionalTestOutputDir` to `"/sdcard/test-outputs/screenshots"`.
  - For `pixel2api33`: set `notPackage` to `"com.yorvana.screenshots"`, and `additionalTestOutputDir` to `"/sdcard/test-outputs/smoke"`.
- Refactor `registerStoreScreenshotCopy` to remove the nested Gradle `Exec` runner task, and instead establish a direct dependency chain:
  - Make each GMD test task depend on its corresponding `clean` task.
  - Make each `copy` task depend on the GMD test task.
  - Delete the temporary reflection helper task `printTestTaskProps`.

### Instrumentation Screenshots Base Class

#### [MODIFY] [StoreScreenshotBase.kt](file:///home/Messier82/projects/service-book/app/src/androidTest/java/com/yorvana/screenshots/StoreScreenshotBase.kt)
- Remove `configureDisplay` function and its call in `setUpStoreScreenshot`. This avoids hardcoded `wm size` overrides, relying on the native virtual device defaults instead.
- Modify `outputBucket()` to throw an error immediately if the `storeScreenshotBucket` argument is missing, removing the dead weight device-hint/size fallback.

### Issue Update

- Post a comment on issue #111 on GitHub using GitHub CLI (`gh`) to document that `screencap` was used instead of `captureRoboImage()` for full platform/system-UI fidelity.

## Verification Plan

### Automated Tests
- Run `./gradlew generateStoreScreenshots` to verify that the three GMD tasks run sequentially, cleanly capture screenshots, and copy them successfully to `distribution/store-assets/screenshots/{phone,tablet-7,tablet-10}/`.
- Run `./gradlew pixel2api33DebugAndroidTest` (or `./gradlew pixel2api33Check`) to verify that the smoke tests execute correctly, pull outputs from `/sdcard/test-outputs/smoke`, and do not run the screenshot tests.
- Run lint and formatting checks: `./gradlew ktlintCheck detekt lintDebug`.