{"review_threads":[{"is_resolved":true,"is_outdated":true,"is_collapsed":true,"comments":[{"body":"`observeVehicle(...).collect { ... }` uses the `collect` terminal operator, but this file does not import `kotlinx.coroutines.flow.collect` (and doesn't have a wildcard flow import). As-is this will not compile; add the missing import (or use an already-imported terminal operator).","path":"app/src/main/java/com/servicebook/ui/records/VehicleDetailViewModel.kt","line":78,"author":"copilot-pull-request-reviewer","created_at":"2026-04-24T13:08:57Z","updated_at":"2026-04-24T13:08:59Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3137888365"}],"total_count":1},{"is_resolved":true,"is_outdated":true,"is_collapsed":true,"comments":[{"body":"`observeVehicle` can emit `null` (e.g., after `deleteVehicle` + refresh). Currently the collector ignores nulls, leaving the last non-null vehicle in state and preventing the UI from reacting to deletions. Consider explicitly clearing `state.vehicle` and/or emitting a `NavigateBack`/error effect when `v == null`.\n```suggestion\n                _state.update { it.copy(vehicle = v) }\n```","path":"app/src/main/java/com/servicebook/ui/records/VehicleDetailViewModel.kt","author":"copilot-pull-request-reviewer","created_at":"2026-04-24T13:08:57Z","updated_at":"2026-04-24T13:08:59Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3137888395"}],"total_count":1},{"is_resolved":true,"is_outdated":false,"is_collapsed":true,"comments":[{"body":"`observeRecord(...).collect { ... }` uses the `collect` terminal operator, but this file does not import `kotlinx.coroutines.flow.collect` (and doesn't have a wildcard flow import). This will not compile; add the missing import (or switch to an already-imported terminal operator).","path":"app/src/main/java/com/servicebook/ui/records/RecordDetailViewModel.kt","line":85,"author":"copilot-pull-request-reviewer","created_at":"2026-04-24T13:08:58Z","updated_at":"2026-04-24T13:08:59Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3137888423"}],"total_count":1},{"is_resolved":true,"is_outdated":true,"is_collapsed":true,"comments":[{"body":"`observeRecord` can emit `null` (e.g., if the record is deleted from another screen and the repository refreshes). The current `return@collect` leaves the last record rendered indefinitely. Consider clearing `state.record` and/or sending a `NavigateBack` effect when `record == null` so the UI reacts correctly to deletions.\n```suggestion\n                if (record == null) {\n                    _state.update {\n                        it.copy(\n                            record = null,\n                            categoryLabel = \"\",\n                            attachments = emptyList(),\n                            confirmDelete = false,\n                            odometerDisplay = \"\",\n                            costDisplay = null,\n                        )\n                    }\n                    _effects.send(RecordDetailEffect.NavigateBack)\n                    return@collect\n                }\n```","path":"app/src/main/java/com/servicebook/ui/records/RecordDetailViewModel.kt","author":"copilot-pull-request-reviewer","created_at":"2026-04-24T13:08:58Z","updated_at":"2026-04-24T13:08:59Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3137888446"}],"total_count":1},{"is_resolved":true,"is_outdated":false,"is_collapsed":true,"comments":[{"body":"This file still calls `performTextInput(...)` (e.g., in `selectMake`), but the import was replaced with `performTextReplacement` and `performTextInput` is no longer imported. Re-add the `performTextInput` import or update the remaining call sites to avoid a compile error.\n```suggestion\nimport androidx.compose.ui.test.performScrollTo\nimport androidx.compose.ui.test.performTextInput\n```","path":"app/src/androidTest/java/com/servicebook/SmokeTest.kt","line":23,"author":"copilot-pull-request-reviewer","created_at":"2026-04-24T13:08:58Z","updated_at":"2026-04-24T13:08:59Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3137888469"}],"total_count":1},{"is_resolved":true,"is_outdated":false,"is_collapsed":true,"comments":[{"body":"Adding `observeVehicle(id)` to this interface requires updating any existing `VehicleRepository` mocks/fakes to implement/stub it. For example, unit tests that create `VehicleDetailViewModel` now call `observeVehicle` during init; relaxed mocks will return `null` for this non-null `Flow` unless explicitly stubbed.","path":"app/src/main/java/com/servicebook/data/repository/VehicleRepository.kt","line":11,"author":"copilot-pull-request-reviewer","created_at":"2026-04-24T13:08:59Z","updated_at":"2026-04-24T13:08:59Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3137888490"}],"total_count":1},{"is_resolved":true,"is_outdated":false,"is_collapsed":true,"comments":[{"body":"Adding `observeRecord(vehicleId, recordId)` to this interface requires updating any existing `RecordRepository` mocks/fakes to implement/stub it. `RecordDetailViewModel` now calls `observeRecord` during init; relaxed mocks will return `null` for this non-null `Flow` unless explicitly stubbed, causing unit tests to fail.","path":"app/src/main/java/com/servicebook/data/repository/RecordRepository.kt","line":17,"author":"copilot-pull-request-reviewer","created_at":"2026-04-24T13:08:59Z","updated_at":"2026-04-24T13:08:59Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3137888514"}],"total_count":1},{"is_resolved":true,"is_outdated":false,"is_collapsed":true,"comments":[{"body":"New observeVehicle(id) behavior isn’t covered by unit tests. VehicleRepositoryImplTest currently exercises observeVehicles() in many scenarios, so adding at least one test for observeVehicle() (initial emit + emit after save/delete/refresh) would prevent regressions in the new reactive detail flow.","path":"app/src/main/java/com/servicebook/data/repository/VehicleRepositoryImpl.kt","line":30,"author":"copilot-pull-request-reviewer","created_at":"2026-04-24T13:37:59Z","updated_at":"2026-04-24T13:38:01Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3138051843"}],"total_count":1},{"is_resolved":true,"is_outdated":false,"is_collapsed":true,"comments":[{"body":"New observeRecord(vehicleId, recordId) behavior isn’t covered by unit tests. RecordRepositoryImplTest already has good coverage for observeRecords(); adding tests for observeRecord() (record exists, record deleted -\u003e emits null, and refresh-triggered re-emit) would better validate the new Flow-based record detail observation used by the UI.","path":"app/src/main/java/com/servicebook/data/repository/RecordRepositoryImpl.kt","line":46,"author":"copilot-pull-request-reviewer","created_at":"2026-04-24T13:38:00Z","updated_at":"2026-04-24T13:38:01Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3138051899"}],"total_count":1},{"is_resolved":true,"is_outdated":false,"is_collapsed":true,"comments":[{"body":"The init collector does multiple suspend calls per emission (category lookup, vehicle read, attachment resolution). Using collectLatest here would avoid doing work for stale emissions if the observed record refreshes rapidly (e.g., during edits), and also makes it easier to cancel in-flight processing on newer values.","path":"app/src/main/java/com/servicebook/ui/records/RecordDetailViewModel.kt","line":90,"author":"copilot-pull-request-reviewer","created_at":"2026-04-24T13:38:00Z","updated_at":"2026-04-24T13:38:01Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3138051953"}],"total_count":1},{"is_resolved":true,"is_outdated":true,"is_collapsed":true,"comments":[{"body":"After saving the edited vehicle, the test immediately asserts the updated title. Since the detail screen title update depends on async repository refresh + Flow collection, this can be flaky. Prefer waiting until the title contains the new nickname (similar to other waitUntil patterns used earlier in this file).\n```suggestion\n        Log.d(TAG, \"S13: waiting for updated name on detail screen\")\n        composeTestRule.waitUntil(10_000) {\n            composeTestRule\n                .onAllNodesWithTag(\"ScreenTitle\")\n                .fetchSemanticsNodes()\n                .any { it.config.getOrNull(androidx.compose.ui.semantics.SemanticsProperties.Text)\n                    ?.any { text -\u003e text.text.contains(\"Updated Car\") } == true }\n        }\n```","path":"app/src/androidTest/java/com/servicebook/SmokeTest.kt","author":"copilot-pull-request-reviewer","created_at":"2026-04-24T13:38:00Z","updated_at":"2026-04-24T13:38:01Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3138051993"}],"total_count":1},{"is_resolved":true,"is_outdated":false,"is_collapsed":true,"comments":[{"body":"The delete confirmation clicks the last node with the generic \"Delete\" text. This is still order-dependent and can become ambiguous if the UI changes. A more robust approach is to target the dialog’s confirm button via a dedicated testTag (or a more specific matcher scoped to the dialog).","path":"app/src/androidTest/java/com/servicebook/SmokeTest.kt","line":697,"author":"copilot-pull-request-reviewer","created_at":"2026-04-24T13:38:01Z","updated_at":"2026-04-24T13:38:01Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3138052035"}],"total_count":1},{"is_resolved":true,"is_outdated":false,"is_collapsed":true,"comments":[{"body":"Similar to record deletion, vehicle deletion confirmation uses onLast() for the generic \"Delete\" label, which is brittle if other delete actions exist or ordering changes. Prefer selecting the confirm action via a dialog-specific testTag (or scoping the query to the dialog semantics).","path":"app/src/androidTest/java/com/servicebook/SmokeTest.kt","line":724,"author":"copilot-pull-request-reviewer","created_at":"2026-04-24T13:38:01Z","updated_at":"2026-04-24T13:38:01Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3138052069"}],"total_count":1},{"is_resolved":false,"is_outdated":false,"is_collapsed":false,"comments":[{"body":"`observeVehicle(...).collectLatest` will send `NavigateBack` every time the flow emits `null`. Since `VehicleDetailScreen` handles this by `backStack.removeLastOrNull()`, repeated emissions (e.g., subsequent refresh triggers while the VM is still alive) can pop multiple screens. Consider guarding so `NavigateBack` is only sent once (e.g., cancel the collection after the first null, or keep a `hasNavigatedBack` flag and skip subsequent sends).","path":"app/src/main/java/com/servicebook/ui/records/VehicleDetailViewModel.kt","line":79,"author":"copilot-pull-request-reviewer","created_at":"2026-04-24T18:25:38Z","updated_at":"2026-04-24T18:25:39Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3139591496"}],"total_count":1},{"is_resolved":false,"is_outdated":false,"is_collapsed":false,"comments":[{"body":"When `record` becomes null, this sends `NavigateBack` from inside the collector. If the upstream `observeRecord` emits `null` again (e.g., due to later refresh triggers before the screen is disposed), the UI may process multiple `NavigateBack` effects and pop too far. Consider ensuring the effect is emitted at most once (cancel the collecting coroutine after the first null, or guard with a flag).","path":"app/src/main/java/com/servicebook/ui/records/RecordDetailViewModel.kt","line":83,"author":"copilot-pull-request-reviewer","created_at":"2026-04-24T18:25:38Z","updated_at":"2026-04-24T18:25:39Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3139591533"}],"total_count":1},{"is_resolved":false,"is_outdated":false,"is_collapsed":false,"comments":[{"body":"`runCatching { recordRepo.deleteRecord(...) }` ignores failures, so a delete error will be silently swallowed (no navigation and no user-visible feedback). Consider handling `.onFailure { ... }` (e.g., add an `error` field to `RecordDetailState` and show a snackbar, or send an effect) so the user isn’t left with a no-op.\n```suggestion\n                    recordRepo.deleteRecord(vehicleId, recordId)\n```","path":"app/src/main/java/com/servicebook/ui/records/RecordDetailViewModel.kt","line":125,"author":"copilot-pull-request-reviewer","created_at":"2026-04-24T18:25:39Z","updated_at":"2026-04-24T18:25:39Z","html_url":"https://github.com/Mesya82/Service-Book/pull/18#discussion_r3139591556"}],"total_count":1}],"totalCount":16,"pageInfo":{"hasNextPage":false,"hasPreviousPage":false,"startCursor":"Y3Vyc29yOnYyOpK0MjAyNi0wNC0yNFQxMzowODo1N1rOfWcXZQ==","endCursor":"Y3Vyc29yOnYyOpK0MjAyNi0wNC0yNFQxODoyNTozOVrOfXqFVA=="}}