[{"id":4447786291,"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-14 06:19 UTC · commit `c2446c0`_\n\u003c!-- Sticky Pull Request Commentbaseline-reminder-bot --\u003e","html_url":"https://github.com/Mesya82/Service-Book/pull/86#issuecomment-4447786291","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-14T05:17:39Z","updated_at":"2026-05-14T06:19:52Z"},{"id":4447806083,"body":"## Code Review\n\n### Overview\nConverts contact-email validation in the bug-report screen from an imperative submit-time snackbar into reactive `TextField` error state with a disabled submit button. Adds Sentry `environment` + `release` tags so events can be filtered per build, plus a debug-only \"Force test crash\" button to verify the Sentry pipeline end-to-end.\n\n### Strengths\n- **Better UX**: invalid email now surfaces inline as the user types, instead of failing only at submit. Submit button correctly disables via the new `isEmailInvalid` derived state — clean and idiomatic.\n- **Sentry tagging is right**: `environment = debug|release` and `release = applicationId@versionName+versionCode` follows Sentry's recommended release format.\n- **Test coverage tracks the refactor**: removed the now-impossible \"submit with invalid email\" path and replaced it with `isEmailInvalid` assertions in VM tests, a screen test asserting the disabled submit + error string, and a new screenshot baseline. `SentryWrapperTest` now verifies environment/release passthrough; `ServiceBookApplicationTest` covers `configureSentryOptions` directly.\n- **Debug-only crash button correctly gated** under the existing `if (BuildConfig.DEBUG)` block at `SettingsScreen.kt:515`.\n\n### Issues / Suggestions\n\n**1. Duplicated environment/release computation (medium)**\n`ReportBugViewModel.kt:156-157` recomputes the exact strings that the new `sentryEnvironment` / `sentryRelease` properties on `ServiceBookApplication` already expose:\n```kotlin\nif (BuildConfig.DEBUG) \"debug\" else \"release\",\n\"${BuildConfig.APPLICATION_ID}@${BuildConfig.VERSION_NAME}+${BuildConfig.VERSION_CODE}\",\n```\nIf someone changes the format in `ServiceBookApplication` (e.g. to add a flavor suffix), the lazy-init path will silently diverge. Since `app` is already injected into the VM, prefer reading `app.sentryEnvironment` / `app.sentryRelease` — or, even cleaner, push those two strings (and the DSN) into `SentryWrapperImpl` so the wrapper owns Sentry config end-to-end and the VM doesn't carry build metadata.\n\n**2. `internal` visibility purely for tests (minor)**\n`configureSentryOptions` was extracted as `internal` so `ServiceBookApplicationTest` can call it. The existing `doSentryInit` is already `protected open` and overridden by `SentryEnabledApp` in tests — the same configuration assertions could be made by capturing the options inside that override, keeping `configureSentryOptions` `private`. Not a blocker, but the test seam already existed.\n\n**3. \"Force test crash\" has no confirmation (minor)**\nA `TextButton` that throws `RuntimeException` on tap is one fat-finger away from killing the dev session mid-edit. Consider an `AlertDialog` confirmation, or at least styling it as `OutlinedButton` to distinguish from the adjacent benign actions. Debug-only so low stakes, but easy to add.\n\n**4. `Patterns.EMAIL_ADDRESS` on the UI thread (very minor)**\nRecomputing the regex match on every keystroke via the `get()` getter is fine for short emails, but if you ever paste a multi-KB string it'll churn. Not worth changing now.\n\n**5. Test naming nit**\n`ReportBugViewModelTest.kt` — the renamed test is now `canSubmit should be false when email is invalid`, but it asserts both the invalid *and* valid transitions. Either rename to `canSubmit reflects email validity` or split into two tests for clarity.\n\n### Risks\n- **Behavior change for users who paste an invalid email then keep typing**: previously they'd see a snackbar only on submit; now the field shows red as they type. This is strictly better but worth a note in release notes.\n- **Sentry release-tag format change**: if you already have rules/alerts filtering by release in Sentry, those filters will start missing events from this version forward until updated. Check existing Sentry project config.\n- **`SentryWrapper` interface is now 7 args**: any other callers (currently only `ReportBugViewModel`) need updating — confirmed only one call site exists, so OK.\n\n### Verdict\nApprove with the duplication fix (#1) addressed before merge — the other items are stylistic. Test coverage looks solid; no security concerns (validation is client-side UX, server-side Sentry handling unchanged; debug-only crash button is gated).","html_url":"https://github.com/Mesya82/Service-Book/pull/86#issuecomment-4447806083","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-14T05:20:10Z","updated_at":"2026-05-14T05:20:10Z"},{"id":4448087830,"body":"## Full PR Pass (HEAD `6ae944f`)\n\n+137 / −44 · 11 files · 2 commits\n\n### CI\n- `ci` — **SKIPPED** (not green; verify the skip is intentional, e.g. a `paths-ignore` filter, before merge)\n- `Coverage Baseline Reminder` — SUCCESS\n- `Semgrep Scan` — SUCCESS\n\n### What this PR does\n1. **Reactive email validation.** New derived `ReportBugState.isEmailInvalid`; `canSubmit` ANDs it in. `TextField` gets `isError` + `supportingText`. Imperative snackbar/return in `onEvent(Submit)` removed.\n2. **Sentry options polish.** `configureSentryOptions(options, dsn)` extracted; adds `environment` (`\"debug\"|\"release\"`) and `release` (`applicationId@versionName+versionCode`). Same values exposed as `internal open val sentryEnvironment` / `sentryRelease` and consumed from `ReportBugViewModel.performSubmission()` via `app.sentryEnvironment` / `app.sentryRelease`, piped through the extended `lazyInitAndSendFeedback(env, release, …)`.\n3. **Debug-only \"Force test crash\" button** in `SettingsScreen`, under the existing `if (BuildConfig.DEBUG)` block. `OutlinedButton` whose `onClick` throws `RuntimeException`.\n\n### Strengths\n- Clean MVVM fit. Reactive `isEmailInvalid` matches the project's StateFlow conventions; `canSubmit` stays one declarative expression.\n- Single source of truth for env/release — `ReportBugViewModel` defers to `app.sentryEnvironment`/`app.sentryRelease`.\n- The new `configureSentryOptions sets dsn, environment and release with privacy defaults` test pins all the privacy-critical defaults (PII off, attach-screenshot off, attach-view-hierarchy off, user-interaction tracing off, auto session tracking off, traces sample 0.0). High-value regression net.\n- Test coverage well-distributed across unit, screen, screenshot, wrapper, and application layers.\n- No orphan strings; `report_bug_contact_email_invalid` is still used (now by `supportingText` instead of by the snackbar).\n- `lazyInitAndSendFeedback` has exactly one production caller, so the signature change is contained.\n\n### Issues / Suggestions\n\n1. **(LOW) Whitespace-padded emails are flagged invalid.** `isEmailInvalid = contactEmail.isNotBlank() \u0026\u0026 !Patterns.EMAIL_ADDRESS.matcher(contactEmail).matches()` rejects `\"foo@bar.com \"` (trailing space). Consider `.trim()` in both `isEmailInvalid` and the value forwarded to Sentry.\n2. **(LOW) `ReportBugScreenTest` mocks the ViewModel.** Asserts the screen wiring (`isError`, `supportingText`) but not the VM↔screen contract end-to-end. VM has its own unit test, so acceptable — just noting the boundary isn't covered.\n3. **(LOW) Screenshot is light-only.** `report_bug_invalid_email_light` exists; no `_dark` counterpart. Other states in the file appear in `_light`/`_dark` pairs (e.g. `disclosure_light`/`_dark`). One-line addition; prevents theming regressions in error colors.\n4. **(LOW) No screenshot for the new Settings button.** `SettingsScreenshotTest` doesn't baseline the new \"Force test crash\" section, and the impl also switched `TextButton` → `OutlinedButton`. Add e.g. `SettingsScreen_debug_crash_section_light` so a future tweak can't regress silently.\n5. **(LOW) Two Sentry init paths with different option sets.** `doSentryInit` configures the full privacy stack; `lazyInitAndSendFeedback` only sets `dsn`/`environment`/`release`/`shutdownTimeoutMillis`. Mutually exclusive today via `!sentryWrapper.isEnabled()`, but the lazy path fires even when the user has **disabled crash reporting** (main init is skipped → `isEnabled()` false → lazy branch runs). Pressing Submit is reasonable consent for that one event, but worth a one-line `// NOTE: bypasses crash-reporting opt-out; user-initiated.` so a future maintainer doesn't generalize this path.\n6. **(LOW) Detekt suppressions are a smell.** `@Suppress(\"LongParameterList\")` on the now-7-param `lazyInitAndSendFeedback` — consider extracting a `SentryInitParams(dsn, environment, release)` data class next time you touch this (don't widen scope in this PR). `@Suppress(\"TooGenericExceptionThrown\")` on the crash button is intrinsic; fine.\n7. **(LOW) Crash button has no confirmation and contains hardcoded English.** `\"ServiceBook dev test crash\"` is a literal in `onClick`; debug-only so non-blocking. A confirmation prevents accidental loss of unsaved state elsewhere.\n8. **(NIT) `Patterns.EMAIL_ADDRESS` rejects internationalized domains/local parts.** Standard Android limitation; flag only if non-ASCII email markets become a target.\n\n### Risks\n- **Release-tag format change.** First time `options.release` is explicitly set; existing Sentry issues may regroup under the new tag.\n- **UX shift snackbar → inline.** Less attention-grabbing for users who don't notice a disabled button; matches modern Material patterns.\n- **`ci` check is SKIPPED**, not passed. Confirm intentional skip before merge.\n\n### Verdict\n**Approve.** All round-1 blockers addressed; the consolidation in `performSubmission` is correct. Remaining items are polish.\n\nSuggested follow-ups (not this PR): trim emails before validation (#1), add dark-mode screenshot (#3), add Settings-debug-section screenshot (#4), extract `SentryInitParams` data class (#6).","html_url":"https://github.com/Mesya82/Service-Book/pull/86#issuecomment-4448087830","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-14T06:05:11Z","updated_at":"2026-05-14T06:05:11Z"},{"id":4448222441,"body":"## Round 2 review — `c2446c0` \"fix: address Round 2 PR review comments and refactor SentryInitParams\"\n\n### CI\n- `ci` — **SKIPPED**\n- `Coverage Baseline Reminder` — **SUCCESS**\n- `Semgrep Scan` — **SUCCESS**\n\nThe `ci` skip persists — worth confirming the path filters aren't masking a real failure on the new screenshot baselines / refactor.\n\n### What this round changes\n1. **`SentryInitParams` data class** introduced in `SentryWrapper.kt`; `lazyInitAndSendFeedback` now takes one params object instead of three positional strings. `@Suppress(\"LongParameterList\")` is gone — clean.\n2. **Email trimming** — both `ReportBugState.isEmailInvalid` and `performSubmission()`'s `email` value now call `.trim()`, so whitespace-padded addresses are validated/normalized correctly.\n3. **Confirmation dialog before \"Force test crash\"** — `OutlinedButton` now sets `showCrashConfirmation = true`; an `AlertDialog` then gates the actual `throw`.\n4. **New screenshots**: `ReportBugScreen_invalid_email_dark`, `SettingsScreen_debug_crash_section_light`.\n5. **Tests updated**: `ReportBugViewModelTest` `verify { ... match { ... } }` block pins `dsn`/`environment`/`release` on the new `SentryInitParams`. `SentryWrapperTest` switches to `SentryInitParams` construction.\n\n### Strengths\n- The `SentryInitParams` extraction is exactly the right shape — it folds the three-string positional parameter explosion into one cohesive value and removes the detekt suppression. Future additions (`sampleRate`, `tracesSampleRate`) can grow this type without churning every call site.\n- The `verify { ... match { ... } }` in `ReportBugViewModelTest` now asserts on the actual init parameters (`dsn`, `environment`, `release`) rather than `any(), any(), any(), any()` — much stronger contract.\n- Round-1 follow-ups (email trim, dark screenshot, debug-section screenshot, crash confirmation) all landed.\n\n### Issues / Suggestions\n\n**1. (Medium) The new `SettingsScreen_debug_crash_section_light` test almost certainly does not show the debug section.**\n`SettingsScreenshotTest.kt:104-108`:\n```kotlin\nsetupScreen(isDark = false)\n// Scroll to the bottom to see the debug section\n// We use the string resource for \"Crash Reporting (Dev Only)\" indirectly via its content\ncomposeTestRule.onRoot().captureScreen(\"SettingsScreen_debug_crash_section_light\")\n```\nThe comment says \"Scroll to the bottom\" but there is no `performScrollTo(...)` or `captureFull(...)` call — the debug section lives below the screen fold inside a vertically-scrolling `Column`, so `captureScreen` (viewport-only) will record the same top-of-screen content as `settings_light`. Other off-screen tests in this same file (`settings_conflict_light`, `settings_validating_light`, `settings_existing_vault_light`) already use `captureFull(...)` for exactly this reason. Either:\n- switch to `captureFull(\"SettingsScreen_debug_crash_section_light\")`, or\n- `composeTestRule.onNodeWithText(...settings_force_test_crash...).performScrollTo()` before capturing.\nThe recorded baseline PNG won't catch any regression in the new section as written.\n\n**2. (Low) AlertDialog uses hardcoded English strings.**\n`SettingsScreen.kt`:\n```kotlin\ntitle = { Text(\"Force Test Crash\") },\ntext = { Text(\"This will immediately crash the app ... may be lost.\") },\nconfirmButton = { TextButton(...) { Text(\"Crash Now\", ...) } },\ndismissButton = { TextButton(...) { Text(\"Cancel\") } },\n```\nThe button just above already uses `stringResource(R.string.settings_force_test_crash)`. Even though this is debug-only, the inconsistency is jarring and `ktlint`/translators may flag it. Suggest:\n- `R.string.settings_force_test_crash_dialog_title` / `_message` / `_confirm` and reuse `android.R.string.cancel` for the dismiss action.\n- Alternatively, leave a `// debug-only: not localized` comment if intentional.\n\n**3. (Low) Dialog state declared mid-composable, dialog placed before the button.**\n```kotlin\nvar showCrashConfirmation by remember { mutableStateOf(false) }\nif (showCrashConfirmation) { AlertDialog(...) }\nOutlinedButton(onClick = { showCrashConfirmation = true }, ...)\n```\nWorks fine, but convention is to hoist state to the top of the composable and put the dialog block at the end (or in a sibling `Box`) so the reader scans the linear UI tree first. Not blocking — purely a readability nit.\n\n**4. (Nit) `SentryInitParams` is colocated with `SentryWrapper.kt`.**\nReasonable as a private-ish helper, but it's a public top-level data class consumed across packages. Either move to `com.servicebook.util.SentryInitParams.kt` (one type per file is the codebase convention in `data/model/`) or annotate the rationale.\n\n**5. (Nit) `crashReportingEnabled = false` default in `SettingsScreenshotTest.setupScreen` is unchanged.**\nFor the debug-section test you don't strictly care, since the section is gated on `BuildConfig.DEBUG`, but for clarity consider passing it explicitly when adding new tests so the test reads as a self-contained spec.\n\n### Risks\n- **Screenshot baseline accuracy** — see issue #1; if the new Settings baseline is just the top-of-screen content, a future change to the debug crash UI would not trigger a screenshot diff. Worth fixing before merge.\n- **No instrumented coverage of the AlertDialog flow** — the dialog only exists in debug builds and isn't exercised by tests. Low risk given it's dev-only, but a `ReportBugScreenTest`-style mocked test would be cheap insurance.\n- **CI still SKIPPED** for the third commit in a row. Confirm path filters aren't excluding `app/src/main/**` from this branch's diff.\n\n### Verdict\n**Approve with one nit-to-fix.** The refactor and round-1 follow-ups are clean — the only thing I'd push back on before merge is the Settings debug-section screenshot test (issue #1), which currently records a stale viewport and gives false confidence. Either switch to `captureFull` or add a `performScrollTo` before capture, and re-record the baseline. Everything else is polish.\n\nSuggested follow-ups for a future PR:\n- Localize the dialog strings (issue #2).\n- Move `SentryInitParams` to its own file if the type grows.\n","html_url":"https://github.com/Mesya82/Service-Book/pull/86#issuecomment-4448222441","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-14T06:30:55Z","updated_at":"2026-05-14T06:30:55Z"},{"id":4448235296,"body":"## Full PR sweep — at head `c2446c0`\n\nReviewing the entire PR (`master…c2446c0`) as a single unit rather than incremental commits. +212 / −53 across 15 files (3 binary PNGs).\n\n### CI\n- `ci` — **SKIPPED** (third commit in a row; please confirm path filters aren't masking real signal — this PR touches `app/src/main/**`, `app/src/test/**`, and `res/values/strings.xml`)\n- `Coverage Baseline Reminder` — **SUCCESS**\n- `Semgrep Scan` — **SUCCESS**\n\n### What this PR does\n\nThree intertwined changes, all production-ready in their final form:\n\n1. **Reactive email validation on the bug-report form.** `ReportBugState.isEmailInvalid` derives validity from `contactEmail.trim()`; the form's email `TextField` flips `isError` + shows `supportingText` inline; submit is gated via `canSubmit`. Replaces the prior submit-time snackbar.\n2. **Improved Sentry initialization.** Options config is extracted to `internal fun configureSentryOptions(options, dsn)` so it can be asserted unit-style without touching `SentryAndroid.init`. `environment` and `release` are now set during both the normal init path *and* the one-shot `lazyInitAndSendFeedback` path. `SentryInitParams` data class collapses the three-string positional explosion.\n3. **Debug-only \"Force test crash\" affordance** in Settings, behind `BuildConfig.DEBUG`, gated by a confirmation `AlertDialog`.\n\n### Strengths\n\n- **State-derived validation done right.** `isEmailInvalid` / `canSubmit` are pure derived getters on `ReportBugState`, with no public setter on the StateFlow — matches the codebase's documented MVVM convention.\n- **Privacy posture is intact and explicit.** `ServiceBookApplicationTest.configureSentryOptions sets dsn, environment and release with privacy defaults` pins the lineup: `isSendDefaultPii=false`, `isAttachScreenshot=false`, `isAttachViewHierarchy=false`, `isEnableAutoSessionTracking=false`, `isEnableUserInteractionTracing=false`, `tracesSampleRate=0.0`. Future regressions to this surface will fail loudly.\n- **Test signature now pins parameters by value, not by `any()`.** `ReportBugViewModelTest` uses `match { it.dsn == ... \u0026\u0026 it.environment == \"debug\" \u0026\u0026 it.release == \"...\" }` — meaningful contract.\n- **`SentryInitParams` is the right shape.** Folds three positional strings into one cohesive value object, removes the `@Suppress(\"LongParameterList\")`, and gives a natural home for future fields (`sampleRate`, `tracesSampleRate`).\n- **Both the disabled-submit + visible-error behavior and the visual look are covered.** `ReportBugScreenTest` for behavior; `ReportBugScreenshotTest` for light + dark visual baselines.\n- **The one-shot opt-out bypass is explicitly documented** in the updated `lazyInitAndSendFeedback` NOTE — the intent (user-initiated → ephemeral Sentry → immediate close) is clear and defensible.\n\n### Issues / Suggestions\n\n**1. (Medium — fix before merge) `SettingsScreenshotTest.settingsScreen_debug_crash_section_light` records the wrong viewport.**\n`SettingsScreenshotTest.kt:101-108`:\n```kotlin\nsetupScreen(isDark = false)\n// Scroll to the bottom to see the debug section\n// We use the string resource for \"Crash Reporting (Dev Only)\" indirectly via its content\ncomposeTestRule.onRoot().captureScreen(\"SettingsScreen_debug_crash_section_light\")\n```\nNo `performScrollTo(...)` and no `captureFull(...)`. The debug section lives below the screen fold inside a vertically-scrolling Column. Sibling tests in this same file (`settings_conflict_light`, `settings_validating_light`, `settings_existing_vault_light`) use `captureFull(...)` for off-screen content for exactly this reason. The new `SettingsScreen_debug_crash_section_light.png` baseline most likely captures the top-of-screen viewport (the same content as `settings_light`), giving false confidence that the new section is regression-protected. **Switch to `captureFull(...)`** (and re-record the baseline), or `onNodeWithText(...settings_force_test_crash...).performScrollTo()` before capture.\n\n**2. (Low) AlertDialog uses hardcoded English strings.**\n`SettingsScreen.kt:568-595`:\n```kotlin\ntitle = { Text(\"Force Test Crash\") },\ntext = { Text(\"This will immediately crash the app to test Sentry integration. ...\") },\nconfirmButton = { TextButton(...) { Text(\"Crash Now\", ...) } },\ndismissButton = { TextButton(...) { Text(\"Cancel\") } },\n```\nThe trigger button just above uses `stringResource(R.string.settings_force_test_crash)`, so this is inconsistent within the same composable. Even for a debug build, this will trip ktlint/CI translation tooling. Either extract `R.string.settings_force_test_crash_dialog_{title,message,confirm}` and use `android.R.string.cancel`, or annotate `// debug-only: not localized` if intentional.\n\n**3. (Low) `ReportBugScreenTest.submit button is disabled and error is shown when email is invalid` uses a fully mocked ViewModel.**\nThe existing happy-path test in the same file (`submit button starts disabled and enables...`) drives state through real `onEvent` calls. The new test side-steps that and pushes a hand-crafted `ReportBugState` straight into a `MutableStateFlow + mockk\u003cReportBugViewModel\u003e(relaxed = true)`. That conflicts with the project memory item *\"Use natural event flow to set up ViewModel state in tests\"* and skips the real `isEmailInvalid` derivation path. Stronger version: construct a real VM, call `SummaryChanged(\"S\")`, `DescriptionChanged(\"D\")`, `ContactEmailChanged(\"invalid\")` and assert the same UI affordances. The behavior under test is *VM → UI*, not *UI render given an arbitrary state*.\n\n**4. (Low) `lazyInitAndSendFeedback` is still the only caller-facing way Sentry can run for an opted-out user.**\nThe NOTE comment captures the rationale, but consider whether the calling site in `ReportBugViewModel.performSubmission()` should also log/breadcrumb the one-shot bypass (e.g., `Log.i(\"ReportBug\", \"Submitting feedback via lazy Sentry init while crash reporting disabled\")`) — purely for forensics if a user later asks \"why did Sentry fire on my device when I had it off?\". Optional.\n\n**5. (Low) `internal open val sentryEnvironment`/`sentryRelease` widen the Application API surface to share two strings with one cross-package caller.**\nThis is the trade-off you had to make to keep `ReportBugViewModel` package-independent — the alternative would be to inject these via `ViewModelDependencies` and keep `ServiceBookApplication`'s sentry knobs `private`/`protected`. Not a blocker; just worth a comment that `sentryEnvironment`/`sentryRelease` are \"Sentry tag values, internal-only because `ReportBugViewModel.performSubmission` needs them for the lazy-init path\".\n\n**6. (Nit) `SentryInitParams` is a top-level public data class colocated with `SentryWrapper.kt`.**\nReasonable for a small helper, but the rest of the codebase puts model types in `data/model/` or its own file. If this class grows (sample rates, tracing options) it should move to `com.servicebook.util.SentryInitParams.kt` for grep-ability.\n\n**7. (Nit) `Patterns.EMAIL_ADDRESS` is intentionally lenient.**\nIt accepts `a@b` (no TLD) and rejects some valid IDN/quoted-local addresses. Same behavior as before, so not a regression — but if you ever get a \"your validator rejected my real address\" report, the deeper fix is RFC-5322-leaning (e.g., Apache Commons `EmailValidator`). Worth a one-line comment noting the lenient choice.\n\n**8. (Nit) Dialog state is declared mid-composable.**\n```kotlin\nvar showCrashConfirmation by remember { mutableStateOf(false) }\nif (showCrashConfirmation) { AlertDialog(...) }\nOutlinedButton(onClick = { showCrashConfirmation = true }, ...)\n```\nConvention is to hoist `remember` state to the top of the composable and either render the dialog block at the end or inside a sibling `Box`. Pure readability.\n\n**9. (Nit) `strings.xml` adds `settings_crash_reporting_dev_title=\"Crash reporting\"` which is *not* qualified as \"Dev only\"** despite the inline comment in the test saying so. If the label is meant to look obviously debug-only in the UI, use something like `\"Crash reporting (Dev only)\"` to match the comment. Otherwise update the comment.\n\n### Risks\n\n- **Release tag format change.** Prior to this PR Sentry events had no explicit `release` tag (or relied on Sentry's default). Now they're stamped `com.servicebook@1.0.0+1`. If you have existing Sentry dashboards filtered by inferred release, they'll see a discontinuity. Low risk but worth a heads-up to whoever monitors the project on Sentry.\n- **UX change for invalid email.** Snackbar → inline error. Documented disclosure block at `ReportBugScreen.kt:309-317` is unaffected. Worth confirming with any QA spec.\n- **CI skipped, third commit running.** If `ci` was intentionally filtered for docs-only PRs, this PR has real source changes and should run. Worth investigating the workflow path filters before merge.\n- **Screenshot baselines (3 new PNGs).** Two of them (`ReportBugScreen_invalid_email_{light,dark}`) are legitimate and useful. The third (`SettingsScreen_debug_crash_section_light`) is suspect — see issue #1.\n\n### Verdict\n\n**Approve, with issue #1 (Settings debug-section screenshot) recommended as a fix-before-merge** because it bakes in a stale baseline that future contributors will inherit and trust. Everything else — including issues #2 (hardcoded dialog strings) and #3 (mocked VM in the new screen test) — is comfortable as follow-up.\n\nSuggested follow-up PR scope:\n- Localize the dev-crash AlertDialog strings (#2).\n- Convert the invalid-email screen test to a real-VM, event-driven setup matching the rest of the file (#3).\n- Add a comment noting the `Patterns.EMAIL_ADDRESS` leniency (#7).\n- Move `SentryInitParams` to its own file if it grows (#6).\n\nNice work overall — the refactor lands cleanly and the privacy-defaults test is a strong addition.\n","html_url":"https://github.com/Mesya82/Service-Book/pull/86#issuecomment-4448235296","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-14T06:33:41Z","updated_at":"2026-05-14T06:33:41Z"}]