[{"id":4453266713,"body":"🔁 **Coverage baseline may need a refresh.**\n\nThis PR touches `app/src/main/**`. If the changes affect what smoke covers, please refresh the baseline before merging:\n\n```bash\n./gradlew generateGmdCoverage\ngit add app/coverage-baselines/gmd_smoke.ec\ngit commit -m \"chore: refresh smoke coverage baseline\"\n```\n\n_Last checked: 2026-05-15 04:06 UTC · commit `fe1702e`_\n\u003c!-- Sticky Pull Request Commentbaseline-reminder-bot --\u003e","html_url":"https://github.com/Mesya82/Service-Book/pull/91#issuecomment-4453266713","user":{"login":"github-actions[bot]","id":41898282,"profile_url":"https://github.com/apps/github-actions","avatar_url":"https://avatars.githubusercontent.com/in/15368?v=4"},"author_association":"NONE","reactions":{"total_count":0,"+1":0,"-1":0,"laugh":0,"confused":0,"heart":0,"hooray":0,"rocket":0,"eyes":0},"created_at":"2026-05-14T17:50:30Z","updated_at":"2026-05-15T04:06:11Z"},{"id":4453323475,"body":"## Code review\n\n### Overview\nExtracts privacy-disclosure UI into reusable components. Settings + Setup get a `PrivacyPolicyDialog` (popup). Consent dialog and Report Bug stay inline-expand but adopt synced `expandVertically + fadeIn` specs (`tween(250)`, `Alignment.Top`) to fix the visual glitch. Also flips Setup's crash-reporting default to `true`.\n\n---\n\n### Deviations from the approved plan\n\n- **Report Bug not converted to popup.** Plan was to extract `DeviceInfoDialog.kt`, drop `ReportBugState.showDeviceInfo` / `ToggleDeviceInfo`, and open a dialog. Instead the PR keeps the inline expand and only fixes the animation specs. That does resolve the visible bug, but it leaves Report Bug inconsistent with Settings/Setup (both popup now) and skips the VM cleanup. Either accept the simplification deliberately or finish the plan — currently it's half-way.\n- **`DeviceInfoDialog.kt` not created**, `TestTags.REPORT_BUG_DEVICE_INFO_DIALOG` not added — consistent with the above.\n\n### Concerns\n\n- **Privacy-sensitive default change is undisclosed scope creep.** `SetupScreen` flips `crashReportingEnabled` from `false` to `true`. This is a separate UX/privacy decision (opt-out vs opt-in for Sentry), not part of the issue #83 fix, and the PR body's \"as requested\" claim has no corresponding instruction in the plan or the issue. Suggest splitting into its own PR with explicit rationale — opt-out crash reporting is the kind of change reviewers and users will want to be explicit about.\n- **`VehicleListConsentDialogTest` asserts different strings in expand vs. collapse tests.**\n  - `clicking Learn more expands disclosure` asserts `settings_privacy_description` is displayed.\n  - `clicking Show less collapses disclosure` asserts `settings_privacy_captured_title` does not exist.\n  \n  Use the same anchor string in both tests so the pair actually proves the same content appears/disappears.\n- **Test names are now misleading.** `settings_privacy_expanded` and `setup_step2_sentry_privacy_expanded` capture a dialog, not an expansion. Plan called for renaming to `settings_privacy_dialog` (and same for setup) — please rename so the screenshot file/test names match the UX.\n- **`Column { ExpandablePrivacyDisclosure() }`** in `VehicleListScreen.kt` wraps a single child in a `Column` — leftover from the deleted `AnimatedVisibility` block. Drop the wrapper.\n- **`heightIn(max = 400.dp)`** in `PrivacyPolicyDialog` is a magic number. `AlertDialog`'s `text` slot already scrolls; consider letting it size naturally or use a density-aware value. Minor.\n\n### Smaller notes\n\n- `ExpandablePrivacyDisclosure` looks good: matched `tween(250)` for size + fade, `expandFrom` / `shrinkTowards = Alignment.Top` — this is the right fix for the inline-collapse glitch.\n- Imports cleaned in `SettingsScreen.kt`, `AppNavGraph.kt`, `VehicleListScreen.kt`.\n- Test tag `PRIVACY_LEARN_MORE` added and used consistently — good for stable selection.\n- New screenshot baseline added for the Setup expanded state; the Settings baseline reuses the existing filename (will need re-recording since the UI changed from inline to dialog — confirm `verifyRoborazziDebug` is green).\n\n### Suggested actions before merge\n\n1. Either complete the Report Bug → dialog conversion or update the PR description to declare the simplification (and remove the unused `state.showDeviceInfo` / `ToggleDeviceInfo` if keeping it inline — currently still in the VM).\n2. Split out the `crashReportingEnabled = true` default change, or justify it in the PR body with a link to the decision.\n3. Rename `*_privacy_expanded` tests/snapshots to `*_privacy_dialog`.\n4. Align the two `VehicleListConsentDialogTest` assertions on the same string.\n5. Drop the unnecessary `Column` wrapper in `VehicleListScreen`.","html_url":"https://github.com/Mesya82/Service-Book/pull/91#issuecomment-4453323475","user":{"login":"Mesya82","id":32867735,"profile_url":"https://github.com/Mesya82","avatar_url":"https://avatars.githubusercontent.com/u/32867735?v=4"},"author_association":"OWNER","reactions":{"total_count":0,"+1":0,"-1":0,"laugh":0,"confused":0,"heart":0,"hooray":0,"rocket":0,"eyes":0},"created_at":"2026-05-14T17:57:52Z","updated_at":"2026-05-14T17:57:52Z"},{"id":4453679882,"body":"Follow-up review on commit `241c837`.\n\n## Resolved from prior review\n- Report Bug device-info card converted to dialog (`DeviceInfoDialog.kt`) — plan deviation closed\n- `Column` wrapper removed in `VehicleListScreen`\n- `*_expanded` screenshot tests renamed to `*_dialog` across Settings, Setup, ReportBug\n- `VehicleListConsentDialogTest` expand/collapse assertions now consistent (`settings_privacy_description`)\n- `ToggleDeviceInfo` split into explicit `ShowDeviceInfo` / `DismissDeviceInfo`\n- Crash-reporting default flip is disclosed in the PR body (item 5) — retracting my earlier objection\n\n## Remaining\n- **Magic dimension:** `PrivacyPolicyDialog.kt:24` still uses a hardcoded `heightIn(max = 400.dp)`. Worth extracting to a named constant or removing in favor of dialog default sizing.\n\n## Minor / nits\n- `DeviceInfoDialog`'s switch no longer respects `isSubmitting` (the inline version had `enabled = !state.isSubmitting`). The trigger row's `clickable` is gated, so the dialog can't be opened mid-submit — but if submission begins while the dialog is open, the toggle stays enabled. Likely fine; flagging for awareness.\n- Confirm button uses `R.string.action_ok` rather than the plan's `R.string.action_dismiss` (\"Got it\"). Minor consistency call.\n- Plan called for removing `showDeviceInfo` from `ReportBugState` in favor of a local `rememberSaveable`. The author kept it in state and renamed the events instead. Reasonable choice (config-change survival via the VM); just noting the deviation.\n\nOverall: the substantive concerns from the first pass are addressed. Only the magic 400.dp is worth changing before merge.","html_url":"https://github.com/Mesya82/Service-Book/pull/91#issuecomment-4453679882","user":{"login":"Mesya82","id":32867735,"profile_url":"https://github.com/Mesya82","avatar_url":"https://avatars.githubusercontent.com/u/32867735?v=4"},"author_association":"OWNER","reactions":{"total_count":0,"+1":0,"-1":0,"laugh":0,"confused":0,"heart":0,"hooray":0,"rocket":0,"eyes":0},"created_at":"2026-05-14T18:42:20Z","updated_at":"2026-05-14T18:42:20Z"},{"id":4456855100,"body":"## Full-pass review (all 3 commits)\n\n### What it does\nResolves #83. Replaces the inline \"Learn more\" expand in Settings + Setup with `PrivacyPolicyDialog`; converts the Report Bug device-info card into `DeviceInfoDialog`; extracts `ExpandablePrivacyDisclosure` (used only by the consent dialog, where nested popups would be bad UX). Flips the crash-reporting default to `true` on Setup (disclosed in PR body item 5).\n\n### Correctness\n- `SetupStepSentry` renders the dialog before `Column` — fine for `AlertDialog` (popup), matches `ReportBugScreen` ordering.\n- `DeviceInfoDialog` switch correctly respects `isSubmitting`; confirm button uses `action_dismiss` for consistency with `PrivacyPolicyDialog`.\n- `PrivacyPolicyDialog` no longer wraps in `Box(heightIn(max = 400.dp))` — `AlertDialog` manages its own max height.\n- `ExpandablePrivacyDisclosure` extraction is clean; consent dialog drops the redundant `Column` wrapper.\n\n### Worth changing\n- **`ReportBugState.showDeviceInfo` still lives in the ViewModel** with `ShowDeviceInfo`/`DismissDeviceInfo` events that just flip a boolean. The plan called for a local `rememberSaveable` in the screen since this is pure UI state, unlike `includeDeviceInfo`. Functionally fine, but the VM is doing ceremony for state that doesn't need to survive process death tied to a VM and doesn't need to be unit-testable. The other dialogs added in this PR (Settings `showPrivacyDialog`, Setup `showPrivacyDialog`) already use local `rememberSaveable` — this is the inconsistent one.\n\n### Minor / nits\n- `SetupScreenshotTest.setup_step2_sentry_privacy_dialog` uses `useUnmergedTree = true` to find `PRIVACY_LEARN_MORE`. The tag is on the `TextButton`'s modifier, so the merged tree should find it. Inconsistent with sibling tests in the same file.\n- `PRIVACY_LEARN_MORE` is shared across two different control types (dialog-opener in Settings/Setup; inline-toggle in `ExpandablePrivacyDisclosure`). Safe today since they don't co-exist, but the shared semantic is \"click for privacy details\" rather than a specific control identity.\n- `TestTags.kt` — extra blank line above `PRIVACY_LEARN_MORE` breaks the tight-grouping convention used elsewhere in the file. Trivial.\n\n### Tests\n- `VehicleListConsentDialogTest` split into separate expand/collapse tests; assertion target switched to `settings_privacy_description` (what `PrivacyPolicyContent` actually displays) — more correct than the previous `settings_privacy_captured_title`.\n- `ReportBugScreenTest` dropped `device info header has correct semantics`. That a11y check no longer applies since the card now opens a dialog, not an inline expand — not replaced. Minor a11y coverage loss, but matches reality.\n- `SmokeTest` step 3 updated coherently (open dialog → assert → dismiss with `action_dismiss`).\n- Screenshot baselines renamed to `*_privacy_dialog` / `*_device_info_dialog_light`; dialogs correctly use `captureFull` to capture the overlay.\n\n### Risks\nNone substantive. The crash-reporting default flip is the only user-visible behavior change outside the issue scope, and it's explicitly disclosed.\n\n### Bottom line\nLGTM. The one thing worth touching before merge is moving `showDeviceInfo` from `ReportBugState` to a local `rememberSaveable` to match the plan and the precedent set elsewhere in this same PR. Everything else is polish.","html_url":"https://github.com/Mesya82/Service-Book/pull/91#issuecomment-4456855100","user":{"login":"Mesya82","id":32867735,"profile_url":"https://github.com/Mesya82","avatar_url":"https://avatars.githubusercontent.com/u/32867735?v=4"},"author_association":"OWNER","reactions":{"total_count":0,"+1":0,"-1":0,"laugh":0,"confused":0,"heart":0,"hooray":0,"rocket":0,"eyes":0},"created_at":"2026-05-15T04:16:45Z","updated_at":"2026-05-15T04:16:45Z"}]