Output too large. Showing first 8,000 and last 32,000 characters. For full output see: /home/Messier82/.gemini/tmp/service-book/tool-outputs/session-6a1c8d1e-8a60-4460-a64e-c185658d819d/mcp_github_pull_request_read_1778749587189_0.txt
[{"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 08:50 UTC · commit `7ac00c8`_\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-14T08:50:01Z"},{"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

... [35,273 characters omitted] ...

"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-14T07:53:42Z","updated_at":"2026-05-14T07:53:42Z"},{"id":4448919211,"body":"All Round 4 review comments addressed:\n1. **Dynamic versioning in tests**: Replaced the hardcoded version string in `ReportBugViewModelTest` with `BuildConfig` references.\n2. **Full screenshot coverage**: Added `SettingsScreen_debug_crash_section_dark` screenshot for dark mode parity.\n3. **Improved screenshot framing**: Updated the scroll target in `SettingsScreenshotTest` to anchor on the \"Force test crash\" button.\n4. **Performance optimization**: Refactored `ReportBugState.isEmailInvalid` to perform `.trim()` only once per read.\n5. **Enhanced Sentry telemetry**: Added `addBreadcrumb` to `SentryWrapper` and automatically log a breadcrumb during the lazy-init feedback path to document the intentional opt-out bypass.\n\nVerified with 29 passing unit/screenshot tests and `ktlintFormat`.","html_url":"https://github.com/Mesya82/Service-Book/pull/86#issuecomment-4448919211","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-14T08:13:48Z","updated_at":"2026-05-14T08:13:48Z"},{"id":4448977110,"body":"## Round 4 review — `5c9dcfa`\n\nTargeted review of the new commit only (changes since `3afbcc8`).\n\n### CI\n- ✅ **Coverage Baseline Reminder** — pass\n- ✅ **Semgrep Scan** — pass\n- ⚠️ **ci** — `skipping` for the 5th commit in a row. Every commit in this PR has touched production Kotlin, so this is almost certainly a path-filter misconfiguration rather than intentional behavior. One-time check on the workflow's `paths`/`paths-ignore` before merge.\n\n### Issues addressed from prior rounds\n\nFive of the six remaining findings from the Round 3 / full-sweep are resolved:\n\n1. **Medium #1 (full sweep) — hardcoded `\"com.servicebook@1.0.0+1\"` in `ReportBugViewModelTest`.** Now reads `\"${BuildConfig.APPLICATION_ID}@${BuildConfig.VERSION_NAME}+${BuildConfig.VERSION_CODE}\"` (`ReportBugViewModelTest.kt:158`), matching `ServiceBookApplicationTest`. Stealth failure on next version bump averted. ✔\n2. **Low #2 (full sweep) — missing dark variant for debug crash screenshot.** New `settingsScreen_debug_crash_section_dark` test + baseline PNG (19973 bytes). ✔\n3. **Low #3 (full sweep) — lazy-init bypass not logged at runtime.** New `SentryWrapper.addBreadcrumb(message)` interface method + impl, invoked from `lazyInitAndSendFeedback` after init and before `captureFeedback` with the static message `\"User-initiated feedback submission (crash reporting disabled)\"`. The breadcrumb will travel on the feedback event itself, making it visible in Sentry triage. ✔\n4. **Nit #5 (full sweep) — `isEmailInvalid` calls `.trim()` twice.** Refactored to `contactEmail.trim().let { trimmed -\u003e ... }`. Single allocation per read. ✔\n5. **Nit #8 (full sweep) — screenshot anchored on section title, not button.** Both `settingsScreen_debug_crash_section_light` and `_dark` now scroll to `R.string.settings_force_test_crash`. More durable framing if controls are added below. ✔\n\nStill open (acceptably):\n- **Low #4 (full sweep) — `Patterns.EMAIL_ADDRESS` leniency.** No behavior change here; the KDoc continues to call out the leniency and the rationale (\"matches the default Android behavior for broad compatibility\"). Reasonable defer — would only matter if Sentry's server-side validation diverges from Android's regex.\n\n### New observations on this commit\n\n#### Low\n\n1. **`addBreadcrumb` has no test coverage on the lazy-init path.** `SentryWrapperTest.lazyInitAndSendFeedback should init Sentry, send feedback, flush and close`'s `verifyOrder` block at lines 82–86 still lists only `SentryAndroid.init` → `Sentry.flush` → `Sentry.close`. A future refactor that drops or reorders the new `addBreadcrumb` call would pass this test. Recommend adding it to the `verifyOrder` block so the ordering and the call itself are pinned:\n\n   ```kotlin\n   verifyOrder {\n       SentryAndroid.init(any\u003cContext\u003e(), any\u003cSentry.OptionsConfiguration\u003cSentryAndroidOptions\u003e\u003e())\n       Sentry.addBreadcrumb(match\u003cio.sentry.Breadcrumb\u003e { it.message?.contains(\"User-initiated feedback submission\") == true })\n       Sentry.flush(5_000L)\n       Sentry.close()\n   }\n   ```\n\n   (Or verify the wrapper's own `addBreadcrumb(String)` was invoked with the static message — whichever level you prefer to pin.)\n\n#### Nit\n\n2. **Breadcrumb message is static and PII-free.** Worth noting positively — no user email, no feedback body, no device tags in the breadcrumb itself. Good privacy posture for a string that ships with every bypass event.\n3. **Breadcrumb placement is correct.** Fires after `SentryAndroid.init` (so it has a hub to attach to) and before `captureFeedback` (so it precedes the event chronologically in the Sentry UI). Order in the impl is exactly right.\n\n### Risks\n\n- **New breadcrumb travels on the feedback event.** This is the intended behavior, but it does mean that anyone with access to your Sentry workspace can see that a given event came from a user who had crash reporting *disabled*. Privacy-wise that's strictly less revealing than the feedback body itself, so no concern; just be aware that the bypass is no longer \"silent\" from a Sentry-admin perspective. Worth a note in `privacy.md` or whatever doc describes the lazy-init path.\n- **Two updated PNG baselines + one new one.** `SettingsScreen_debug_crash_section_light.png` (19152 → 19851 bytes) and the new `_dark.png` (19973 bytes). The light delta is from the `performScrollTo` target change (section title → button), so the captured viewport shifts slightly. Spot-check.\n\n### Verdict\n\n**Approve. No fix-before-merge blockers.** This is a clean Round 4: every concrete fix-before-merge candidate from the prior reviews has been addressed, including the Medium hardcoded-version test fix. The single new finding (#1 above — `addBreadcrumb` not in `verifyOrder`) is a Low — it's a real test-coverage gap, but the production behavior is correct and the gap fails *silent* not *loud*. Ship as-is and follow up with the test pin, or fold it in before merge — either is reasonable.\n\nThe PR has now had four rounds of incremental review and every finding has either been resolved or has a documented defer rationale. End-to-end it's in good shape: contained scope, real coverage at unit/screen/screenshot levels, privacy defaults pinned by test, internal API surface kept narrow, and now runtime visibility into the bypass path. CI `ci=skipping` remains the only nagging signal worth resolving — likely a workflow path-filter issue independent of this PR.\n","html_url":"https://github.com/Mesya82/Service-Book/pull/86#issuecomment-4448977110","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-14T08:22:05Z","updated_at":"2026-05-14T08:22:05Z"},{"id":4448988478,"body":"## Full PR sweep — `5c9dcfa` (current HEAD)\n\nEnd-to-end review of all changes in PR #86 vs. `master`, not just the latest commit.\n\n### What this PR does\n\nThree logical changes packaged together (5 commits):\n\n1. **Reactive contact-email validation** in the Report Bug screen — validity is derived state (`ReportBugState.isEmailInvalid`), rendered inline via Material3 `TextField`'s `isError` + `supportingText`, and gates `canSubmit`. Replaces the previous snackbar-after-submit flow.\n2. **Sentry init refactor** — extracts `configureSentryOptions(options, dsn)` so behavior is testable; introduces `environment` (`debug`/`release`) and `release` (`\u003cappId\u003e@\u003cversionName\u003e+\u003cversionCode\u003e`) tags on both the normal and lazy-init paths; adds a Sentry breadcrumb (`\"User-initiated feedback submission (crash reporting disabled)\"`) on the lazy path for triage visibility. New `SentryInitParams(dsn, environment, release)` data class formalizes the three values.\n3. **Debug-only Force Test Crash button** in Settings → Developer, with a confirmation `AlertDialog` and full string-resource set. Throws `RuntimeException` on confirm to exercise Sentry crash reporting. Verified debug-gated by `if (BuildConfig.DEBUG)` at `SettingsScreen.kt:517`.\n\n### CI\n\n- ✅ **Coverage Baseline Reminder** — pass\n- ✅ **Semgrep Scan** — pass\n- ⚠️ **ci** — `skipping` on all 5 commits. Every commit in this PR touches production Kotlin (`ServiceBookApplication`, `ReportBugViewModel`, `SettingsScreen`, `SentryWrapper`), so the workflow path filter looks misconfigured for this PR — worth a one-time check, or a forced run before merge.\n\n### Strengths\n\n- **Privacy defaults pinned by test.** `configureSentryOptions sets dsn, environment and release with privacy defaults` asserts every one of `isSendDefaultPii=false`, `isEnableUserInteractionTracing=false`, `isEnableAutoSessionTracking=false`, `isAttachScreenshot=false`, `isAttachViewHierarchy=false`. Any future regression that flips one of these will fail loudly. Excellent guardrail for a privacy-sensitive integration.\n- **Version-bump-safe verifier in the VM test.** `verify { sentryWrapper.lazyInitAndSendFeedback(any(), match { it.release == \"${BuildConfig.APPLICATION_ID}@${BuildConfig.VERSION_NAME}+${BuildConfig.VERSION_CODE}\" }, ...) }` references `BuildConfig` constants instead of hardcoding the version string. Survives version bumps.\n- **Breadcrumb on lazy-init bypass.** `addBreadcrumb(\"User-initiated feedback submission (crash reporting disabled)\")` fires after init and before `captureFeedback`, so it travels on the same event session. Triage from the Sentry UI can now distinguish bypass-feedback from normal-feedback. The message is static and PII-free.\n- **`SentryInitParams` shape.** Three correlated values that always travel together — making them a data class is the right call. Colocated in its own file (`util/SentryInitParams.kt`) rather than at the top of `SentryWrapper.kt`.\n- **Consistent `.trim()` semantics.** `s.contactEmail.trim().ifBlank { null }` in `performSubmission` lines up with `isEmailInvalid`'s `.trim()`. No `\" foo@bar.com \"` mismatch between validation and submission. `isEmailInvalid` itself now trims once via `.let { trimmed -\u003e ... }`.\n- **Reactive validation matches the codebase pattern.** Using `isEmailInvalid` as a derived `val get()` on the data class keeps the rule in one place and feeds both the submit-gate and the `supportingText`.\n- **Dialog strings fully externalized.** Title/message/confirm in `strings.xml`; cancel uses `android.R.string.cancel` so the user's system locale picks it up automatically.\n- **Screenshot tests scroll to the button.** Both `_light` and `_dark` variants do `performScrollTo()` on `R.string.settings_force_test_crash` (the button itself), so the captured region won't drift if controls are added below.\n- **ReportBugScreen test follows the event-flow convention.** Real `ReportBugViewModel` constructed with `ViewModelDependencies`, state driven via `onEvent(SummaryChanged/DescriptionChanged/ContactEmailChanged)` — matches the project's \"StateFlow has no public setter\" rule.\n- **Force-crash is debug-gated.** Lives inside the existing `if (BuildConfig.DEBUG)` block at `SettingsScreen.kt:517`. Confirmed no path for release users to reach this button.\n- **`internal open val sentryEnvironment/sentryRelease` kept narrow.** `internal` keeps the new Application API surface within the module; `open` allows test override; KDoc justifies the cross-package visibility (`ReportBugViewModel.performSubmission`'s lazy-init path needs them).\n\n### Issues\n\n#### Low\n\n1. **`addBreadcrumb` is not pinned by `SentryWrapperTest.lazyInitAndSendFeedback`.** The `verifyOrder` block at `SentryWrapperTest.kt:82-86` lists `SentryAndroid.init` → `Sentry.flush` → `Sentry.close`, but not the new `Sentry.addBreadcrumb` call that fires between init and flush. A future refactor that drops the breadcrumb (e.g., during a Sentry SDK migration) would pass this test. Recommend adding it to the `verifyOrder` block to pin both the call and its position:\n\n   ```kotlin\n   verifyOrder {\n       SentryAndroid.init(any\u003cContext\u003e(), any\u003cSentry.OptionsConfiguration\u003cSentryAndroidOptions\u003e\u003e())\n       Sentry.addBreadcrumb(match\u003cio.sentry.Breadcrumb\u003e {\n           it.message?.contains(\"User-initiated feedback submission\") == true\n       })\n       Sentry.flush(5_000L)\n       Sentry.close()\n   }\n   ```\n\n2. **`Patterns.EMAIL_ADDRESS` accepts inputs Sentry might reject.** The KDoc on `isEmailInvalid` calls out the leniency (\"e.g., accepts a@b\"), but `email` is passed verbatim to `Sentry.captureUserFeedback`. If Sentry's server-side normalization drops malformed addresses, the user can submit thinking their email is attached and never get a reply. Low-impact — Sentry usually stores the value as-is — but worth tracking against actual feedback data once this ships.\n\n3. **`SentryWrapperTest` still uses the literal `\"com.servicebook@1.0.0+1\"`** at line 65 when constructing `SentryInitParams`. This is the same kind of stealth-failure-on-version-bump that was fixed in `ReportBugViewModelTest`. The test only asserts that `options.release == params.release`, so it would technically pass even if the literal drifted — but it's misleading to readers and inconsistent with the sibling tests. Worth replacing with `\"${BuildConfig.APPLICATION_ID}@${BuildConfig.VERSION_NAME}+${BuildConfig.VERSION_CODE}\"` for consistency.\n\n#### Nit\n\n4. **Invalid-email screenshots use `captureScreen` rather than `captureFull`.** The email field is mid-screen so the viewport is fine in practice, but `report_bug_disclosure_light` and friends in the same file use `captureScreen` too — so this is actually consistent. Mentioning for completeness, no change needed.\n\n5. **`internal open val sentryEnvironment/sentryRelease` widens the Application API for one cross-package caller.** The KDoc justifies it well. Pragmatic; `internal` keeps the blast radius to the module. An alternative would be to pass an `(Application) -\u003e SentryInitParams` factory through `ViewModelDependencies`, but that's only worth the churn if you grow more cross-package Sentry callers.\n\n6. **Breadcrumb travels on the feedback event** — anyone with access to the Sentry workspace can now infer that a given submission came from a user with crash reporting *disabled*. Strictly less revealing than the feedback body itself, so no privacy concern, but the bypass is no longer \"silent\" from a Sentry-admin perspective. Consider noting in `privacy.md` or wherever the lazy-init path is described.\n\n### Risks\n\n- **First Sentry `release` tag.** Pre-PR, the SDK auto-detected releases (or didn't set them); post-PR, every event will carry `com.servicebook@\u003cversion\u003e+\u003ccode\u003e`. Existing Sentry dashboards filtering by release will see a clean break — old events on the auto-detected value, new events on the explicit value. Heads-up for whoever maintains the Sentry workspace.\n- **UX change: snackbar → inline error.** Users who learned to fix email *after* tapping Submit (snackbar prompt) now find Submit greyed out without a triggering action. Generally a UX improvement, but worth being intentional about — the supporting-text wording (`report_bug_contact_email_invalid`) is the only signal.\n- **`tracesSampleRate = 0.0` plus explicit `environment`/`release` is fine** — performance traces are off, so the new tags only flow on the error/feedback paths.\n- **4 new screenshot baselines** (`ReportBugScreen_invalid_email_light/dark.png`, `SettingsScreen_debug_crash_section_light/dark.png`). All re-recorded to current geometry. Spot-check before merge.\n\n### Verdict\n\n**Approve.** No fix-before-merge blockers. All 9 numbered issues from the Round 2 sweep, plus the Round 3 follow-ups, plus the full-sweep Medium #1 (hardcoded version string) and Lows #2/#3 and Nits #5/#8, are resolved. The three remaining items (Low #1 breadcrumb verifyOrder, Low #2 EMAIL_ADDRESS leniency, Low #3 SentryWrapperTest literal) are all \"follow-up scope\" — none silently break behavior or fail at runtime.\n\nEnd-to-end the PR is in good shape: contained scope, real test coverage at unit / screen / screenshot levels, privacy defaults pinned by test, internal API surface kept narrow, runtime breadcrumb on the bypass path, and screenshots anchored on durable targets. CI `ci=skipping` is now on its 5th commit and worth resolving as a one-off — likely a workflow path-filter issue independent of this PR.\n","html_url":"https://github.com/Mesya82/Service-Book/pull/86#issuecomment-4448988478","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-14T08:23:53Z","updated_at":"2026-05-14T08:23:53Z"},{"id":4449152010,"body":"All Round 5 review comments addressed:\n1. **Pinned `addBreadcrumb` in `SentryWrapperTest`**: Added `Sentry.addBreadcrumb` to the `verifyOrder` block in `lazyInitAndSendFeedback` test to ensure correct sequencing.\n2. **Dynamic versioning in `SentryWrapperTest`**: Replaced the hardcoded release string with `BuildConfig` references for consistency with production code and to avoid failures on version bumps.\n3. **CI Skipping**: Investigated `.github/workflows/ci-label.yml`. CI is explicitly gated behind the `ci` label. I recommend adding the `ci` label to this PR to trigger the full CI suite.\n\nVerified with 434 passing unit tests (including fixed `SentryWrapperTest`) and `ktlintFormat`.","html_url":"https://github.com/Mesya82/Service-Book/pull/86#issuecomment-4449152010","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-14T08:49:48Z","updated_at":"2026-05-14T08:49:48Z"},{"id":4449206981,"body":"## Round 5 review — `7ac00c8`\n\nTargeted review of the new commit only (changes since `5c9dcfa`).\n\n### CI\n- ✅ **Coverage Baseline Reminder** — pass\n- ✅ **Semgrep Scan** — pass\n- ⚠️ **ci** — `skipping` for the 6th commit in a row. The only file changed this commit is a test file, so this commit not running CI is at least *defensible* (path filter could legitimately exclude test-only changes) — but earlier production-code commits in this PR also skipped, so the underlying misconfiguration is unchanged. Worth a one-time check on the workflow's `paths`/`paths-ignore` once this PR ships.\n\n### Issues addressed from prior rounds\n\nBoth Lows from the Round 4 / full-sweep that were still open are resolved:\n\n1. **Low #1 (Round 4) — `addBreadcrumb` not pinned in `verifyOrder`.** Now pinned at `SentryWrapperTest.kt:90-94`:\n   ```kotlin\n   Sentry.addBreadcrumb(\n       match\u003cString\u003e {\n           it.contains(\"User-initiated feedback submission\")\n       },\n   )\n   ```\n   Correct overload (`Sentry.addBreadcrumb(String)` — matches `SentryWrapperImpl.addBreadcrumb` at line 40), placed between `SentryAndroid.init` and `Sentry.flush` so position is pinned too. A refactor that drops or reorders the breadcrumb now fails this test loud. ✔\n2. **Low #3 (full sweep) — `SentryWrapperTest` literal `\"com.servicebook@1.0.0+1\"`.** Now uses `\"${BuildConfig.APPLICATION_ID}@${BuildConfig.VERSION_NAME}+${BuildConfig.VERSION_CODE}\"` (`SentryWrapperTest.kt:69`), consistent with `ReportBugViewModelTest` and `ServiceBookApplicationTest`. Version bumps won't silently invalidate the fixture. ✔\n\nStill open (acceptably):\n- **Low #2 (full sweep) — `Patterns.EMAIL_ADDRESS` leniency.** No behavior change here; documented in KDoc on `ReportBugState.isEmailInvalid`. Reasonable defer.\n\n### New observations on this commit\n\nNone. This is a tightly-scoped test-only commit (1 file, +12/−1) that addresses exactly the two findings called out in the prior reviews. No new code paths, no new fixture shapes, no test-helper drift. Imports cleanly add `com.servicebook.BuildConfig` next to the existing imports; the `match\u003cString\u003e` block is consistent with the wrapper's `addBreadcrumb(String)` signature.\n\n### Risks\n\nNone new. No production code touched.\n\n### Verdict\n\n**Approve. No fix-before-merge blockers.** Round 5 closes the last two open Lows from the Round 4 / full sweep. The PR has now had 5 rounds of incremental review plus 2 full sweeps; every Medium and Low with a concrete remediation has been addressed, and the only remaining items are KDoc'd defers (`Patterns.EMAIL_ADDRESS` leniency) or out-of-PR scope (`ci=skipping` workflow path filter).\n\nReady to merge.\n","html_url":"https://github.com/Mesya82/Service-Book/pull/86#issuecomment-4449206981","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-14T08:58:10Z","updated_at":"2026-05-14T08:58:10Z"},{"id":4449216930,"body":"## Full PR sweep — `7ac00c8` (current HEAD)\n\nEnd-to-end review of all changes in PR #86 vs. `master`, not just the latest commit.\n\n### What this PR does\n\nThree logical changes packaged together (6 commits, 17 files, +278/−53):\n\n1. **Reactive contact-email validation** in the Report Bug screen — validity is derived state (`ReportBugState.isEmailInvalid`), rendered inline via Material3 `TextField`'s `isError` + `supportingText`, and gates `canSubmit`. Replaces the previous snackbar-after-submit flow. `performSubmission` and the validator both `.trim()` so leading/trailing whitespace doesn't cause a validation/submission mismatch.\n2. **Sentry init refactor** — extracts `configureSentryOptions(options, dsn)` so behavior is testable; introduces `environment` (`debug`/`release`) and `release` (`\u003cappId\u003e@\u003cversionName\u003e+\u003cversionCode\u003e`) tags on both the normal-init and lazy-init paths; adds a Sentry breadcrumb (`\"User-initiated feedback submission (crash reporting disabled)\"`) on the lazy path for triage visibility. New `SentryInitParams(dsn, environment, release)` data class formalizes the three values.\n3. **Debug-only Force Test Crash button** in Settings → Developer, with a confirmation `AlertDialog` and full string-resource set. Throws `RuntimeException` on confirm to exercise Sentry crash reporting. Gated by `if (BuildConfig.DEBUG)` at `SettingsScreen.kt:517`.\n\n### CI\n\n- ✅ **Coverage Baseline Reminder** — pass\n- ✅ **Semgrep Scan** — pass\n- ⚠️ **ci** — `skipping` on all 6 commits. Production-Kotlin commits 1–5 of this PR were clearly miscategorized by the workflow's path filter; the test-only commit 6 is defensible. Recommend a one-time check on the workflow's `paths`/`paths-ignore` once this PR ships — independent of this PR's content.\n\n### Strengths\n\n- **Privacy defaults pinned by test.** `configureSentryOptions sets dsn, environment and release with privacy defaults` (`ServiceBookApplicationTest.kt:126-145`) asserts every one of `isSendDefaultPii=false`, `isEnableUserInteractionTracing=false`, `isEnableAutoSessionTracking=false`, `isAttachScreenshot=false`, `isAttachViewHierarchy=false`, plus `sampleRate=1.0` and `tracesSampleRate=0.0`. Any future regression that flips one of these fails loud. Excellent guardrail for a privacy-sensitive integration.\n- **Version-bump-safe verifiers across all three Sentry tests.** `ReportBugViewModelTest.kt:158`, `ServiceBookApplicationTest.kt:138-140`, and `SentryWrapperTest.kt:69` all use `\"${BuildConfig.APPLICATION_ID}@${BuildConfig.VERSION_NAME}+${BuildConfig.VERSION_CODE}\"`. Version bumps won't silently invalidate any fixture.\n- **Breadcrumb pinned in `verifyOrder`.** `SentryWrapperTest.kt:87-94` places `Sentry.addBreadcrumb` between `SentryAndroid.init` and `Sentry.flush` — so both the call itself *and its position* are pinned. A refactor that drops or reorders the bypass breadcrumb will fail loud.\n- **Environment/release setters verified.** `verify { options.environment = params.environment }` and `verify { options.release = params.release }` at `SentryWrapperTest.kt:97-98` pin the values that flow from `SentryInitParams` into the SDK options. Mock-friendly and behaviorally precise.\n- **`SentryInitParams` shape.** Three correlated values that always travel together — making them a data class is the right call. Colocated in its own file (`util/SentryInitParams.kt`) rather than at the top of `SentryWrapper.kt`.\n- **Consistent `.trim()` semantics.** `s.contactEmail.trim().ifBlank { null }` in `performSubmission` (`ReportBugViewModel.kt:143`) lines up with `isEmailInvalid`'s `.trim()` (line 48). No `\" foo@bar.com \"` mismatch between validation and submission. `isEmailInvalid` trims once via `.let { trimmed -\u003e ... }`.\n- **Reactive validation matches the codebase pattern.** Using `isEmailInvalid` as a derived `val get()` on the data class keeps the rule in one place and feeds both the submit-gate and the `supportingText`.\n- **Dialog strings fully externalized.** Title / message / confirm in `strings.xml`; cancel uses `android.R.string.cancel` so the user's system locale picks it up automatically. The destructive-action message (\"Any unsaved changes in other screens may be lost\") is appropriate.\n- **Screenshot tests scroll to the button.** Both `_light` and `_dark` variants `performScrollTo()` `R.string.settings_force_test_crash` — the button itself, not the section heading — so the captured region won't drift if controls are added below.\n- **ReportBugScreen test follows the event-flow convention.** Real `ReportBugViewModel` constructed with `ViewModelDependencies`, state driven via `onEvent(SummaryChanged / DescriptionChanged / ContactEmailChanged)` — matches the project's \"StateFlow has no public setter\" rule (CLAUDE.md).\n- **Force-crash is debug-gated.** Lives inside the existing `if (BuildConfig.DEBUG)` block at `SettingsScreen.kt:517`. No release-build code path reaches the button or the `RuntimeException` throw.\n- **`internal open val sentryEnvironment/sentryRelease` kept narrow.** `internal` keeps the new Application API surface within the module; `open` allows test override; KDoc explicitly justifies the cross-package visibility (`ReportBugViewModel.performSubmission`'s lazy-init path needs them).\n- **Breadcrumb is static and PII-free.** No user email, no feedback body, no device tags in the breadcrumb string itself. Good privacy posture for a string that ships with every bypass event.\n\n### Issues\n\n#### Low\n\n1. **`Patterns.EMAIL_ADDRESS` accepts inputs Sentry might reject.** The KDoc on `isEmailInvalid` calls out the leniency (\"e.g., accepts a@b\"), but `email` is passed verbatim to `Sentry.captureUserFeedback`. If Sentry's server-side normalization drops malformed addresses, the user can submit thinking their email is attached and never get a reply. Low-impact — Sentry typically stores the value as-is — but worth tracking against actual feedback data once this ships. Documented defer is acceptable.\n\n#### Nit\n\n2. **`verify { options.environment = params.environment }` is slightly indirect.** Reads \"verify that whatever I happened to pass in got assigned\" — true but tautological at first glance. `verify { options.environment = \"debug\" }` would be more explicit, at a small cost in coupling. Stylistic; the current form is fine and consistent with the per-field pattern.\n3. **New `ReportBugScreenTest` constructs `ViewModelDependencies` with all 9 named parameters.** A new dependency in `ViewModelDependencies` would force a fix here — but every other VM test in the file pays the same cost, and using named parameters makes the failure mode obvious. Consider a `ViewModelDependencies.testInstance(app)` factory only if the named-arg list grows; today it's fine.\n4. **Invalid-email screenshots use `captureScreen`, not `captureFull`.** The field is mid-screen so the viewport contains the error supporting text in practice. Consistent with the rest of `ReportBugScreenshotTest.kt`. No change.\n5. **Breadcrumb travels on the feedback event** — anyone with Sentry workspace access can now infer that a given submission came from a user with crash reporting disabled. Strictly less revealing than the feedback body itself, so no privacy concern, but the bypass is no longer \"silent\" from a Sentry-admin perspective. Consider noting in `privacy.md` or wherever the lazy-init path is described.\n\n### Risks\n\n- **First Sentry `release` tag.** Pre-PR, the SDK auto-detected releases (or didn't set them); post-PR, every event will carry `com.servicebook@\u003cversion\u003e+\u003ccode\u003e`. Existing Sentry dashboards filtering by release will see a clean break — old events on the auto-detected value, new events on the explicit value. Heads-up for whoever maintains the Sentry workspace.\n- **UX change: snackbar → inline error.** Users who learned to fix email *after* tapping Submit (snackbar prompt) now find Submit greyed out without a triggering action. Generally a UX improvement, but worth being intentional about — the supporting-text wording (`report_bug_contact_email_invalid`) is the only signal.\n- **`tracesSampleRate = 0.0` plus explicit `environment`/`release` is fine** — performance traces are off, so the new tags only flow on the error/feedback paths.\n- **4 new screenshot baselines** (`ReportBugScreen_invalid_email_light/dark.png`, `SettingsScreen_debug_crash_section_light/dark.png`). All re-recorded to current geometry. Spot-check before merge.\n\n### Verdict\n\n**Approve. No fix-before-merge blockers.** The PR has now had 5 rounds of incremental review and 3 full sweeps. Every Medium and concretely-actionable Low across all rounds is resolved:\n\n- Hardcoded version strings in tests → all three test files now reference `BuildConfig` constants.\n- Missing dark screenshot for debug crash → added.\n- Lazy-init bypass with no runtime trace → `addBreadcrumb` added on the bypass path, pinned by test ordering.\n- Double `.trim()` in `isEmailInvalid` → refactored to a single `.let { trimmed -\u003e ... }`.\n- Screenshot anchored on section title → re-anchored on the button itself.\n- `dsn` parameter sprawl → consolidated into `SentryInitParams`.\n- `addBreadcrumb` not pinned in `verifyOrder` → now pinned (Round 5).\n- `SentryWrapperTest` literal version string → now dynamic (Round 5).\n\nThe single remaining item (`Patterns.EMAIL_ADDRESS` leniency) is KDoc'd as intentional and only matters if Sentry's server-side validation diverges from Android's regex — reasonable defer. `ci=skipping` is an out-of-PR workflow path-filter concern.\n\nReady to merge.\n","html_url":"https://github.com/Mesya82/Service-Book/pull/86#issuecomment-4449216930","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-14T08:59:42Z","updated_at":"2026-05-14T08:59:42Z"}]