[{"id":4430680494,"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-13 05:59 UTC · commit `8de3920`_\n\u003c!-- Sticky Pull Request Commentbaseline-reminder-bot --\u003e","html_url":"https://github.com/Mesya82/Service-Book/pull/73#issuecomment-4430680494","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-12T12:54:57Z","updated_at":"2026-05-13T05:59:54Z"},{"id":4430740204,"body":"## Code Review\n\n### Overview\nAdds a user-facing \"Report a bug or suggest a feature\" screen wired to Sentry's User Feedback API, with a mailto: fallback, settings entry point, and unit/screenshot/smoke test coverage. ~960 LOC across 11 files.\n\n### Correctness / Bugs\n\n- **`retryCount` is never reset** (`ReportBugViewModel.kt:448`). It's a class-level field that increments forever. After one failed submission burns through 2 retries, every subsequent submission in the same VM instance skips retrying and goes straight to email fallback. Reset to 0 inside `submitReport()` before the try block, or on success.\n- **`SentryWrapperImpl.lazyInitAndSendFeedback` violates opt-in semantics** (`SentryWrapper.kt:785-796`). If the user has crash reporting disabled, `Sentry.isEnabled()` returns false and the code calls `SentryAndroid.init(...)` to send feedback anyway — effectively initializing Sentry without the consent the ADR-009 flow gates on. Either (a) require crash-reporting opt-in to submit, or (b) make the policy explicit (one-shot, scoped init for the report only) and document it.\n- **`UserFeedback(SentryId.EMPTY_ID)`** (`ReportBugViewModel.kt:485`). The Sentry API expects feedback tied to an event ID. Submitting with `EMPTY_ID` may be silently dropped or stripped server-side; verify against a real Sentry project before relying on this in production.\n- **`state.error` is set but never rendered**. The \"Retrying (1/2)…\" copy and final error string are stored in state but no Composable consumes them; only the snackbar success path is wired. Either render `state.error` or remove the dead field plus `DismissError` event.\n- **Double-submit window**. During the retry path, `isSubmitting` is flipped to `false` (`ReportBugViewModel.kt:518`) before the recursive call. The submit button briefly becomes enabled and a tap could trigger a parallel submission. Keep `isSubmitting = true` for the duration of the retry chain.\n\n### Convention / Style\n\n- **Hardcoded user-facing strings** in `ReportBugScreen.kt`:\n  - `text = \"Type\"` (line 242) — section header not localized.\n  - `\"No email app found\"` (line 184) — fallback toast not localized.\n  - `\"--- Device Info ---\"` and field labels in `fallbackToEmail()` (`ReportBugViewModel.kt:543-549`).\n- **PR description ↔ code mismatch**: description claims a Material 3 `ButtonGroup` is used for type selection; the code uses two regular `Button`s with conditional colors and fully-qualified `androidx.compose.material3.ButtonDefaults.buttonColors()` calls (lines 253, 255, 265, 267).\n- **Deprecated `Uri.parse(\"mailto:\")`** (line 176). Use `\"mailto:\".toUri()`.\n- **Unused test tag** `REPORT_BUG_LOADING` declared in `TestTags.kt` but never applied to a node.\n- **Card + Switch UX conflict** (`ReportBugScreen.kt:311-347`). The `Switch` controls `includeDeviceInfo` but the `Card` below has its own `onClick` toggling a separate local `showDeviceInfo`. Two independent device-info toggles on the same screen is confusing — clarify which gates submission vs. which previews.\n- **`docs/plan-smoke-tests.md` S21 heading regression**: the change demotes `### S21:` to `- **S21: ...**`, breaking the heading hierarchy. Looks accidental.\n\n### Tests\n\n- Smoke S22 only checks rendering and disabled-submit state — appropriate, no Sentry side effects.\n- `ReportBugViewModelTest` does not cover **`retryCount` reset across submissions** — would have caught the bug above. Add a \"submit succeeds after a previous failed submission\" test.\n- Tests do not cover the **`fallbackToEmail` body contents** (device-info conditional inclusion).\n- Screenshot test for `isSubmitting=true` is added but the screen does not visually reflect submitting state (no progress indicator, no disabled overlay) — the screenshot will look identical to the empty state. Either render a loading indicator (the `REPORT_BUG_LOADING` tag suggests intent) or drop the screenshot.\n\n### Security / Privacy\n\n- Same concern as the opt-in note above: initializing Sentry on demand when the user has disabled crash reporting should be explicit policy, not an implementation detail.\n- `contactEmail` is submitted as `feedback.email`; verify the Sentry project's PII scrubbing matches the privacy policy.\n\n### Performance\n\n- Negligible. Init/close of Sentry on every fallback report is wasteful but rare.\n\n### Suggested follow-ups before merge\n1. Reset `retryCount` on each `submitReport()` entry.\n2. Resolve the opt-in semantics for `lazyInitAndSendFeedback` (block submission when crash reporting is off, or document the exception).\n3. Localize the hardcoded strings; remove or wire up `state.error`.\n4. Replace `Uri.parse` with `toUri()`, drop the unused `REPORT_BUG_LOADING` tag, fix `_state.update` ordering to avoid the double-submit window.\n5. Revert the S21 heading change in `docs/plan-smoke-tests.md`.\n6. Either render a real submitting state or drop the `submitting` screenshot.","html_url":"https://github.com/Mesya82/Service-Book/pull/73#issuecomment-4430740204","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-12T13:02:33Z","updated_at":"2026-05-12T13:02:33Z"},{"id":4437838607,"body":"## Re-review (fixup `8de3920`)\n\n### Previously raised — resolved\n\n- **`retryCount` reset** ✅ Reset to 0 on each `Submit` event (`ReportBugViewModel.kt:277`). Regression test added.\n- **`UserFeedback(SentryId.EMPTY_ID)`** ✅ Now captures a `\"User Feedback\"` event and uses its returned event ID (`SentryWrapper.kt:432-437`). Note this emits a real event into the Sentry project on every submission — acceptable but worth being aware of for project hygiene/billing.\n- **`state.error` unused** ✅ Field and `DismissError` event removed; retry/error messages now flow through snackbar effects.\n- **Double-submit window** ✅ `performSubmission()` recurses without flipping `isSubmitting`; submit/cancel/fields all gated on `!state.isSubmitting`. Solid.\n- **Hardcoded strings** ✅ `report_bug_type_section` and `report_bug_no_email_app` localized. (Device-info labels in `fallbackToEmail()` still inline English — see below.)\n- **`Uri.parse` → `toUri()`** ✅ Replaced.\n- **Unused `REPORT_BUG_LOADING` tag** ✅ Now applied to the in-button `CircularProgressIndicator`; submitting screenshot is now meaningful.\n- **Card + Switch UX** ✅ Refactored — switch lives inside the expanded card, one expand IconButton, one switch controlling `includeDeviceInfo`. Clearer.\n- **S21 heading** ✅ Reverted to `### S21:`.\n- **Test coverage** ✅ New tests: retryCount reset, fallback body with/without device info.\n\n### Opt-in policy — partially addressed\n\n- `SentryWrapperImpl.lazyInitAndSendFeedback` now carries an explicit docstring (`SentryWrapper.kt:39-44`) and `Sentry.close()` is called after submission. The intent (\"scoped, one-shot, no background telemetry\") is now documented and the SDK is torn down — a meaningful improvement.\n- **Remaining concern:** the user who opted *out* of crash reporting still has the SDK initialized in-process for the duration of the submit, with whatever startup auto-instrumentation `SentryAndroid.init` does (lifecycle, ANR watchdog hooks, etc.) until `close()`. If this is acceptable, consider an in-screen disclosure when crash reporting is off (\"Submitting will briefly connect to our error reporting service for this report only\") so the consent is explicit at the moment of action. Otherwise, gating submission on the crash-reporting opt-in remains the cleanest path. Either way, capture the decision in ADR-009 so the policy is discoverable.\n\n### Still open / new observations\n\n- **PR description ↔ code**: still says Material 3 `ButtonGroup` — code uses two `Button`s. Either update the description or switch to `ButtonGroup`.\n- **`androidx.compose.material3.ButtonDefaults.buttonColors()`** still fully qualified (`ReportBugScreen.kt:138, 146`). Add an import for consistency with the rest of the file.\n- **Device-info payload still has inline English** in `fallbackToEmail()` (`\"--- Device Info ---\"`, `\"App Version\"`, etc.). The body is what the user sees in their email client, so this is user-facing copy.\n- **Snackbar storm on failure**: full failure now produces 3 snackbar effects in quick succession (retry 1, retry 2, failed) before the email intent fires. `SnackbarHostState` shows them one at a time and the email Activity launch may interrupt before any are seen. Consider rolling these into a single status snackbar or suppressing the retry toasts.\n- **Race on `isSubmitting`**: `submitReport()` now flips `isSubmitting = true` inside `viewModelScope.launch` rather than before launching (`ReportBugViewModel.kt:283-287`). In practice main-dispatcher serialization makes this race-free; just noting in case the dispatcher ever changes.\n- **`ReportBugScreenTest` shifted from real input to mocked state** for the submit-button test. The behavior coverage is still there via the VM tests, but you lost the \"typing into the real field enables the button\" round-trip — acceptable trade-off, just flagging.\n- **Smoke baseline reminder bot** flagged `app/src/main/**` changes; if S22 changes what the smoke covers, refresh `gmd_smoke.ec` before merging.\n\n### Verdict\n\nThe fixup addresses every critical bug from the previous review. The remaining items are docs/polish/policy clarifications — none are merge-blockers, but the opt-in disclosure and ADR-009 update are worth doing before this lands.","html_url":"https://github.com/Mesya82/Service-Book/pull/73#issuecomment-4437838607","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-13T06:02:42Z","updated_at":"2026-05-13T06:02:42Z"}]