Closed
Conversation
- Add core, domain, and data modules for better separation of concerns - Migrate UI screens to use ViewModels and StateFlow for robust state management - Integrate Hilt for dependency injection across the application - Add comprehensive unit tests for Use Cases and ViewModels - Refactor SurveillanceDetector for DI and clean architecture - Fix duplicate module inclusions in settings.gradle.kts - Add .claude/ to .gitignore and include project documentation
- Add core, domain, and data modules for better separation of concerns - Migrate UI screens to use ViewModels and StateFlow for robust state management - Integrate Hilt for dependency injection across the application - Add comprehensive unit tests for Use Cases and ViewModels - Refactor SurveillanceDetector for DI and clean architecture - Fix duplicate module inclusions in settings.gradle.kts - Add .claude/ to .gitignore and include project documentation
- Add core, domain, and data modules for better separation of concerns - Migrate UI screens to use ViewModels and StateFlow for robust state management - Integrate Hilt for dependency injection across the application - Add comprehensive unit tests for Use Cases and ViewModels - Refactor SurveillanceDetector for DI and clean architecture - Fix duplicate module inclusions in settings.gradle.kts - Add .claude/ to .gitignore and include project documentation
- Remove duplicate app-layer domain models - Point all active consumers to core.model and domain.repository - Update test utilities to match
- Route wifi/bluetooth/cellular nav entries to rebuilt list screens - Migrate NetworkFinderScreen to modular WifiNetwork model - Migrate CompleteScannerService WiFi and classic Bluetooth paths to shared domain repositories - NOTE: BLE scan path and CellularTower write path in CompleteScannerService still use legacy DAO (tracked for next session)
- Scale scan cadence by queue pressure and flush duration - Respect battery saver state when not charging - Surface throttle and power state in scanner notification
- Persist location samples and scan sessions in the rebuilt architecture - Tag WiFi/BLE/Bluetooth/Cellular sightings with session IDs - Render trajectory breadcrumbs and mission-scoped playback in the rebuilt map - Surface diagnostics for collection pressure and playback context
- Add aggregate-plus-history Bluetooth and Cellular detail screens - Add detail ViewModels and history queries by MAC address / cell ID - Wire rebuilt list screens and nav routes to the new detail screens
- Delete legacy screens and components with no remaining rebuilt call sites - Keep shared legacy detail helper composables in a standalone file - Preserve compileability while leaving ChannelAnalysisScreen in place
|
Important Review skippedToo many files! This PR contains 197 files, which is 47 over the limit of 150. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (197)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+41
to
+51
| startScanning() | ||
| } | ||
|
|
||
| fun toggleScanning() { | ||
| _uiState.update { it.copy(isScanning = !it.isScanning) } | ||
| if (_uiState.value.isScanning) { | ||
| startScanning() | ||
| } | ||
| } | ||
|
|
||
| private fun startScanning() { |
There was a problem hiding this comment.
Bug: The ViewModel's init block starts a scanning coroutine without setting isScanning to true, causing state inconsistency and subsequent coroutine leaks when scanning is toggled by the user.
Severity: HIGH
Suggested Fix
Store the Job from the launched coroutine in a variable. In startScanning(), cancel any existing Job before launching a new one. Ensure _uiState is updated to set isScanning to true when scanning starts and false when it stops. Consider creating a separate stopScanning() function to handle cancellation and state updates, and call it when the scan is toggled off.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
app/src/main/kotlin/com/shadowcheck/mobile/presentation/viewmodel/ThreatDetectionViewModel.kt#L41-L51
Potential issue: The `ThreatDetectionViewModel`'s `init` block calls `startScanning()`,
which launches a coroutine to collect data, but the UI state `isScanning` remains
`false`. This creates an initial state where the app is actively scanning but the UI
indicates it is not. When the user taps the start button, `toggleScanning()` calls
`startScanning()` again, launching a second coroutine without cancelling the first. Each
subsequent toggle from off to on launches another coroutine, leading to an accumulation
of concurrent jobs that consume CPU and memory and can cause erratic UI updates.
Did we get this right? 👍 / 👎 to inform future reviews.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 197 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/kotlin/com/shadowcheck/mobile/data/database/model/WifiNetworkEntity.kt">
<violation number="1" location="app/src/main/kotlin/com/shadowcheck/mobile/data/database/model/WifiNetworkEntity.kt:15">
P1: Add a new Room migration (and bump the DB version) to add the new wifi_networks columns; otherwise existing users will hit a schema mismatch at startup.</violation>
</file>
<file name="app/src/main/kotlin/com/shadowcheck/mobile/presentation/viewmodel/ChannelDistributionViewModel.kt">
<violation number="1" location="app/src/main/kotlin/com/shadowcheck/mobile/presentation/viewmodel/ChannelDistributionViewModel.kt:35">
P2: Handle 2484 MHz as channel 14 explicitly; the current formula maps it to 15.</violation>
</file>
<file name="app/src/debug/kotlin/com/shadowcheck/mobile/service/MockScannerService.kt">
<violation number="1" location="app/src/debug/kotlin/com/shadowcheck/mobile/service/MockScannerService.kt:35">
P2: startMockScanning() is invoked on every onStartCommand call without checking if a scan job is already running, so multiple service starts will spawn overlapping loops and duplicate inserts. Track a Job/flag and skip if already active.</violation>
</file>
<file name="app/src/main/kotlin/com/shadowcheck/mobile/domain/model/SurveillanceDetector.kt">
<violation number="1" location="app/src/main/kotlin/com/shadowcheck/mobile/domain/model/SurveillanceDetector.kt:16">
P2: The 2-argument detectThreats no longer sorts by severity, so callers now receive detections in collection order rather than priority order.</violation>
</file>
<file name="MVVM_REFACTORING.md">
<violation number="1" location="MVVM_REFACTORING.md:1">
P3: The doc marks the refactor as completed/100% compliant but later lists multiple screens still using Room.databaseBuilder. Update the completion claim to reflect the remaining work so the status isn’t misleading.</violation>
</file>
<file name="app/src/main/kotlin/com/shadowcheck/mobile/data/database/model/LocationSampleEntity.kt">
<violation number="1" location="app/src/main/kotlin/com/shadowcheck/mobile/data/database/model/LocationSampleEntity.kt:10">
P2: Add an index for sessionId since queries filter on it; otherwise getSamplesForSession will scan the whole table as it grows.</violation>
</file>
<file name="app/src/main/kotlin/com/shadowcheck/mobile/data/database/model/HardwareMetadataEntity.kt">
<violation number="1" location="app/src/main/kotlin/com/shadowcheck/mobile/data/database/model/HardwareMetadataEntity.kt:7">
P2: `upsert` uses REPLACE but macAddress isn’t unique, so inserting the same device will create duplicate rows instead of updating the existing one. Add a unique index (or make macAddress the primary key) so REPLACE works as intended.</violation>
</file>
<file name="app/src/main/kotlin/com/shadowcheck/mobile/data/database/model/BluetoothDeviceEntity.kt">
<violation number="1" location="app/src/main/kotlin/com/shadowcheck/mobile/data/database/model/BluetoothDeviceEntity.kt:14">
P1: These new bluetooth_devices columns require a Room schema version bump and a migration to add them (with defaults). Without it, existing installs will fail schema validation or crash on open.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| val level: Int, | ||
| val timestamp: Long | ||
| val timestamp: Long, | ||
| val latitude: Double = 0.0, |
There was a problem hiding this comment.
P1: Add a new Room migration (and bump the DB version) to add the new wifi_networks columns; otherwise existing users will hit a schema mismatch at startup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/kotlin/com/shadowcheck/mobile/data/database/model/WifiNetworkEntity.kt, line 15:
<comment>Add a new Room migration (and bump the DB version) to add the new wifi_networks columns; otherwise existing users will hit a schema mismatch at startup.</comment>
<file context>
@@ -11,9 +11,94 @@ data class WifiNetworkEntity(
val level: Int,
- val timestamp: Long
+ val timestamp: Long,
+ val latitude: Double = 0.0,
+ val longitude: Double = 0.0,
+ val channel: Int = 0,
</file context>
| val rssi: Int, | ||
| val timestamp: Long | ||
| val timestamp: Long, | ||
| val sessionId: String = "", |
There was a problem hiding this comment.
P1: These new bluetooth_devices columns require a Room schema version bump and a migration to add them (with defaults). Without it, existing installs will fail schema validation or crash on open.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/kotlin/com/shadowcheck/mobile/data/database/model/BluetoothDeviceEntity.kt, line 14:
<comment>These new bluetooth_devices columns require a Room schema version bump and a migration to add them (with defaults). Without it, existing installs will fail schema validation or crash on open.</comment>
<file context>
@@ -2,17 +2,59 @@ package com.shadowcheck.mobile.data.database.model
val rssi: Int,
- val timestamp: Long
+ val timestamp: Long,
+ val sessionId: String = "",
+ val latitude: Double = 0.0,
+ val longitude: Double = 0.0,
</file context>
| .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5000), ChannelDistributionUiState()) | ||
|
|
||
| private fun getChannelFromFreq(freq: Int): Int? = when (freq) { | ||
| in 2412..2484 -> (freq - 2412) / 5 + 1 |
There was a problem hiding this comment.
P2: Handle 2484 MHz as channel 14 explicitly; the current formula maps it to 15.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/kotlin/com/shadowcheck/mobile/presentation/viewmodel/ChannelDistributionViewModel.kt, line 35:
<comment>Handle 2484 MHz as channel 14 explicitly; the current formula maps it to 15.</comment>
<file context>
@@ -0,0 +1,39 @@
+ .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5000), ChannelDistributionUiState())
+
+ private fun getChannelFromFreq(freq: Int): Int? = when (freq) {
+ in 2412..2484 -> (freq - 2412) / 5 + 1
+ in 5170..5825 -> (freq - 5170) / 5 + 34
+ else -> null
</file context>
|
|
||
| override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int { | ||
| Log.d(TAG, "MockScannerService started") | ||
| startMockScanning() |
There was a problem hiding this comment.
P2: startMockScanning() is invoked on every onStartCommand call without checking if a scan job is already running, so multiple service starts will spawn overlapping loops and duplicate inserts. Track a Job/flag and skip if already active.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/debug/kotlin/com/shadowcheck/mobile/service/MockScannerService.kt, line 35:
<comment>startMockScanning() is invoked on every onStartCommand call without checking if a scan job is already running, so multiple service starts will spawn overlapping loops and duplicate inserts. Track a Job/flag and skip if already active.</comment>
<file context>
@@ -0,0 +1,192 @@
+
+ override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int {
+ Log.d(TAG, "MockScannerService started")
+ startMockScanning()
+ return START_STICKY
+ }
</file context>
Comment on lines
+16
to
19
| return buildList { | ||
| addAll(detectRogueAccessPoints(wifiNetworks)) | ||
| addAll(detectSuspiciousBluetoothDevices(btDevices)) | ||
| } |
There was a problem hiding this comment.
P2: The 2-argument detectThreats no longer sorts by severity, so callers now receive detections in collection order rather than priority order.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/kotlin/com/shadowcheck/mobile/domain/model/SurveillanceDetector.kt, line 16:
<comment>The 2-argument detectThreats no longer sorts by severity, so callers now receive detections in collection order rather than priority order.</comment>
<file context>
@@ -1,228 +1,120 @@
- )
- )
- }
+ return buildList {
+ addAll(detectRogueAccessPoints(wifiNetworks))
+ addAll(detectSuspiciousBluetoothDevices(btDevices))
</file context>
Suggested change
| return buildList { | |
| addAll(detectRogueAccessPoints(wifiNetworks)) | |
| addAll(detectSuspiciousBluetoothDevices(btDevices)) | |
| } | |
| return buildList { | |
| addAll(detectRogueAccessPoints(wifiNetworks)) | |
| addAll(detectSuspiciousBluetoothDevices(btDevices)) | |
| }.sortedByDescending { it.severity.ordinal } |
|
|
||
| @Entity( | ||
| tableName = "location_samples", | ||
| indices = [Index(value = ["timestamp"])] |
There was a problem hiding this comment.
P2: Add an index for sessionId since queries filter on it; otherwise getSamplesForSession will scan the whole table as it grows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/kotlin/com/shadowcheck/mobile/data/database/model/LocationSampleEntity.kt, line 10:
<comment>Add an index for sessionId since queries filter on it; otherwise getSamplesForSession will scan the whole table as it grows.</comment>
<file context>
@@ -0,0 +1,53 @@
+
+@Entity(
+ tableName = "location_samples",
+ indices = [Index(value = ["timestamp"])]
+)
+data class LocationSampleEntity(
</file context>
| import androidx.room.PrimaryKey | ||
| import com.shadowcheck.mobile.core.model.HardwareMetadata | ||
|
|
||
| @Entity(tableName = "hardware_metadata") |
There was a problem hiding this comment.
P2: upsert uses REPLACE but macAddress isn’t unique, so inserting the same device will create duplicate rows instead of updating the existing one. Add a unique index (or make macAddress the primary key) so REPLACE works as intended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/src/main/kotlin/com/shadowcheck/mobile/data/database/model/HardwareMetadataEntity.kt, line 7:
<comment>`upsert` uses REPLACE but macAddress isn’t unique, so inserting the same device will create duplicate rows instead of updating the existing one. Add a unique index (or make macAddress the primary key) so REPLACE works as intended.</comment>
<file context>
@@ -0,0 +1,37 @@
+import androidx.room.PrimaryKey
+import com.shadowcheck.mobile.core.model.HardwareMetadata
+
+@Entity(tableName = "hardware_metadata")
+data class HardwareMetadataEntity(
+ @PrimaryKey(autoGenerate = true) val id: Long = 0,
</file context>
| @@ -0,0 +1,276 @@ | |||
| # MVVM Refactoring - COMPLETED ✅ | |||
There was a problem hiding this comment.
P3: The doc marks the refactor as completed/100% compliant but later lists multiple screens still using Room.databaseBuilder. Update the completion claim to reflect the remaining work so the status isn’t misleading.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At MVVM_REFACTORING.md, line 1:
<comment>The doc marks the refactor as completed/100% compliant but later lists multiple screens still using Room.databaseBuilder. Update the completion claim to reflect the remaining work so the status isn’t misleading.</comment>
<file context>
@@ -0,0 +1,276 @@
+# MVVM Refactoring - COMPLETED ✅
+
+## Summary
</file context>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 284aee5103
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private val MIGRATION_5_6 = object : Migration(5, 6) { | ||
| override fun migrate(database: SupportSQLiteDatabase) { | ||
| database.execSQL("ALTER TABLE wifi_networks ADD COLUMN sessionId TEXT NOT NULL DEFAULT ''") | ||
| database.execSQL("ALTER TABLE ble_devices ADD COLUMN sessionId TEXT NOT NULL DEFAULT ''") |
There was a problem hiding this comment.
MIGRATION_5_6 alters ble_devices, but the migration chain in this file never creates that table (and similarly MIGRATION_2_3 alters sensor_readings without a prior create), so upgrading an existing shadowcheck.db can fail with no such table during app startup. This is a blocking upgrade regression for users coming from older schemas, so the new entities need explicit create-table steps (or a different migration strategy) before these ALTER statements run.
Useful? React with 👍 / 👎.
Comment on lines
+332
to
+336
| sensorService = SensorCollectionService( | ||
| context = this, | ||
| sensorReadingRepository = sensorReadingRepository, | ||
| hardwareMetadataRepository = hardwareMetadataRepository, | ||
| highPerformanceMode = highPerformanceMode |
There was a problem hiding this comment.
onStartCommand always replaces sensorService with a new instance before handling the action, which breaks lifecycle when a second START (or STOP) intent arrives while scanning: startScanning() short-circuits on isScanning, so the new instance is never started, and later stopScanning() stops this new instance instead of the original running one. That leaves the original sensor listeners orphaned and still collecting in the background.
Useful? React with 👍 / 👎.
Comment on lines
+518
to
+520
| runBlocking { | ||
| flushPendingWrites() | ||
| endCurrentSession() |
There was a problem hiding this comment.
stopScanning() uses runBlocking to flush queued writes and end the session synchronously on the service thread. Because these calls perform multiple repository/database writes, shutdown can block long enough to trigger ANR under heavy scan load. The flush/end operations should run asynchronously instead of blocking the main service lifecycle path.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Related Issues
Closes #
Changes Made
Testing Performed
Screenshots (if applicable)
Checklist
Code Quality
Additional Notes
Summary by cubic
Rearchitected the app into a modular MVVM stack with Hilt DI, shared core models, and a rebuilt scanner pipeline. This improves boundaries, testability, and adds session-based telemetry with better performance.
New Features
:core,:domain,:data, and modular WiFi (com.shadowcheck.mobile.wifi).CompleteScannerServicewith session tracking, location samples, BLE/classic BT/cell capture, and power‑aware throttling.MainActivity.EmulatorHelperandMockScannerServicefor quick data seeding on emulators..github/workflows/ci.yml),.editorconfig, and contributor docs.Refactors
com.shadowcheck.mobile.core.modeland removed duplicate app-layer domain models and repositories.BleModule,SensorDataModule,SessionDataModule, etc.).SurveillanceDetectorfor DI and modular WiFi models; removed legacy screens likeARNetworkView.compileSdkto 35, addedproguard-rules.pro, and updated.gitignore.WiGLEApiService) withWifiRepositoryModule.Written for commit 284aee5. Summary will update on new commits.