Conversation
Summary of ChangesHello @tejpratap46, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's data management capabilities by introducing a robust offline-first synchronization framework. It integrates Supabase as a backend service, providing a structured approach to synchronize local data with a remote server. The new framework ensures data consistency, handles conflict resolution, and allows the application to function reliably even in offline scenarios, thereby improving overall user experience and data integrity. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a bidirectional sync system to motion-store: sync-aware DAOs, per-table cursor tracking, backend adapters (Firebase/Supabase/REST), SyncManager orchestration, WorkManager worker/factory, device ID initializer, and dependency updates for Firebase/Supabase and WorkManager. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App/Activity
participant SM as SyncManager
participant Adapter as Backend Adapter<br/>(Firebase/Supabase/REST)
participant DAO as SyncableDao
participant LocalDB as Local SQLite DB
participant Backend as Remote Backend<br/>(Firestore/PostgREST/HTTP)
rect rgba(100, 149, 237, 0.5)
Note over App,Backend: Sync Flow (syncTable)
App->>SM: syncTable(dao)
SM->>DAO: getDownloadedTill(table)
DAO->>LocalDB: SELECT downloaded_till
LocalDB-->>DAO: value
DAO-->>SM: lastDownloadedTill
SM->>Adapter: fetchSince(table, since, deviceId)
Adapter->>Backend: query rows (uploadedAt > since, updatedBy ≠ deviceId)
Backend-->>Adapter: rows
Adapter-->>SM: server rows
SM->>DAO: fromServerRow/map & upsert
DAO->>LocalDB: INSERT/UPDATE rows
LocalDB-->>DAO: results
DAO-->>SM: DownloadStats
SM->>DAO: findDirty()
DAO->>LocalDB: SELECT is_dirty=1
LocalDB-->>DAO: dirtyRows
DAO-->>SM: dirtyRows
loop per dirtyRow
SM->>Adapter: create/update(table, data)
Adapter->>Backend: POST/PUT
Backend-->>Adapter: server row (serverId, uploadedAt)
Adapter-->>SM: server response
SM->>DAO: markUploaded/markSynced(localId, serverId, uploadedAt)
DAO->>LocalDB: UPDATE sync columns
end
SM->>DAO: setDownloadedTill(table, maxUploadedAt)
DAO->>LocalDB: UPSERT downloaded_tracker
SM-->>App: SyncResult
end
sequenceDiagram
participant WM as WorkManager
participant SW as SyncWorker
participant SM as SyncManager
participant DAOs as List<SyncableDao>
rect rgba(144, 238, 144, 0.5)
Note over WM,DAOs: Periodic Sync
WM->>SW: doWork()
SW->>SM: sync(daos)
loop per DAO
SM->>DAOs: syncTable(dao)
DAOs-->>SM: SyncResult
end
SM-->>SW: List<SyncResult>
alt NetworkError with retries left
SW-->>WM: Result.retry()
else Success
SW-->>WM: Result.success(metrics)
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR introduces an offline-first sync system with bidirectional synchronization between local SQLite and a remote backend (Supabase). The architecture is well-structured with clear separation of concerns.
Critical Issues Found: 4
The implementation has several crash risks and a data loss scenario that must be fixed before merge:
- Crash risks in SupabaseAdapter:
.first()calls will crash if server returns empty responses - Data loss risk in SyncManager: Download cursor advances even when entity operations fail, causing permanent data loss
- Crash risk in SyncableDao: NULL serverId values will cause type mismatch crashes
These issues prevent correct functionality under error conditions and must be resolved.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| for (row in serverRows) { | ||
| val serverId = row[SyncTracker.COL_SERVER_ID] as? String ?: continue | ||
| val uploadedAt = (row[SyncTracker.COL_UPLOADED_AT] as? Number)?.toLong() ?: 0L | ||
|
|
||
| val serverUpdatedOn = (row[SyncTracker.COL_UPDATED_ON] as? Number)?.toLong() ?: 0L | ||
|
|
||
| val localEntity = dao.findByServerId(serverId) | ||
|
|
||
| when { | ||
| // ── New row from server — never seen locally ────────────────── | ||
| localEntity == null -> { | ||
| val entity = | ||
| try { | ||
| dao.fromServerMap(row) | ||
| } catch (e: Exception) { | ||
| throw SyncException.ParseError(dao.tableName, row, e) | ||
| } | ||
| dao.insert(entity) | ||
| saved++ | ||
| } | ||
|
|
||
| // ── Conflict: server wins (server is newer or equal) ────────── | ||
| serverUpdatedOn >= localEntity.syncTracker.updatedOn -> { | ||
| val entity = | ||
| try { | ||
| dao.fromServerMap(row, localId = localEntity.id) | ||
| } catch (e: Exception) { | ||
| throw SyncException.ParseError(dao.tableName, row, e) | ||
| } | ||
| dao.update(localEntity.id, entity) | ||
| conflicts++ | ||
| saved++ | ||
| } | ||
|
|
||
| // ── Conflict: local wins (local is newer) — skip ────────────── | ||
| else -> { | ||
| skipped++ | ||
| } | ||
| } | ||
|
|
||
| if (uploadedAt > highWaterMark) highWaterMark = uploadedAt | ||
| } | ||
|
|
||
| // Advance cursor only after all rows are safely written | ||
| if (highWaterMark > since) { | ||
| downloadedTracker.setDownloadedTill(dao.tableName, highWaterMark) | ||
| } |
There was a problem hiding this comment.
🛑 Logic Error: Download cursor is advanced even if entity inserts/updates fail. If dao.insert() or dao.update() throws an exception (e.g., database constraint violation), the cursor advances to highWaterMark anyway at line 175, causing those server rows to be skipped permanently on future syncs. Wrap the download loop in a database transaction or only advance the cursor after verifying all operations succeeded.
| updatedBy = getString(idx(SyncTracker.COL_UPDATED_BY)), | ||
| createdOn = getLong(idx(SyncTracker.COL_CREATED_ON)), | ||
| updatedOn = getLong(idx(SyncTracker.COL_UPDATED_ON)), | ||
| serverId = getString(idx(SyncTracker.COL_SERVER_ID)), |
There was a problem hiding this comment.
🛑 Crash Risk: getString() returns null when the column value is NULL, but the result is assigned directly to serverId: String in the SyncTracker constructor without null handling. This will cause a type mismatch crash when reading rows where server_id is NULL. The SyncTracker field is nullable (serverId: String?), so this should handle the null case.
| serverId = getString(idx(SyncTracker.COL_SERVER_ID)), | |
| serverId = if (isNull(idx(SyncTracker.COL_SERVER_ID))) null else getString(idx(SyncTracker.COL_SERVER_ID)), |
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt
Outdated
Show resolved
Hide resolved
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt
Outdated
Show resolved
Hide resolved
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| package com.tejpratapsingh.motionstore.domain | ||
|
|
||
| import android.content.ContentValues | ||
| import com.tejpratapsingh.motionstore.tables.SyncTracker |
There was a problem hiding this comment.
WARNING: Unused import
android.content.ContentValues is imported but never used in this interface. The SyncableEntity interface only references SyncTracker and Long. This import should be removed.
| import com.tejpratapsingh.motionstore.tables.SyncTracker |
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt
Show resolved
Hide resolved
| } | ||
|
|
||
| // ── Conflict: server wins (server is newer or equal) ────────── | ||
| serverUpdatedOn >= localEntity.syncTracker.updatedOn -> { |
There was a problem hiding this comment.
WARNING: Dirty local changes silently discarded when server row is newer or equal
The download phase runs before the upload phase. If a local row is dirty (has pending local changes, isDirty = true) and the server has a row with updatedOn >= localEntity.syncTracker.updatedOn, the server data overwrites the local dirty row at line 159 (dao.update(localEntity.id, entity)). The local changes are then lost — they will never be uploaded because the row is no longer dirty after the overwrite.
Consider checking localEntity.syncTracker.isDirty before overwriting: if the local row is dirty, either skip the server update (let the upload phase win) or merge the changes, depending on your conflict resolution policy.
Code Review SummaryStatus: 6 Issues Found | Recommendation: Address before merge Fix these issues in Kilo Cloud Overview
Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)Issues found in unchanged code or requiring manifest changes:
Files Reviewed (12 files)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive offline-first synchronization mechanism. A critical security concern is a potential PII leak in the error handling logic, where the ParseError exception could expose sensitive user information in logs or the UI. Furthermore, there's a potential for data inconsistency due to incorrect timestamp handling during the upload phase, a risk of crashes in the Supabase adapter when handling server responses, and some minor documentation inconsistencies.
| val uploadedAt = | ||
| (response[SyncTracker.COL_UPLOADED_AT] as? Number)?.toLong() | ||
| ?: System.currentTimeMillis() |
There was a problem hiding this comment.
Falling back to System.currentTimeMillis() for uploadedAt is a critical flaw that can break sync consistency. The server-generated timestamp is essential for the download cursor. Using a client-side timestamp can cause this device to miss future updates from other devices. If the server does not return uploaded_at, it must be treated as an error.
| val uploadedAt = | |
| (response[SyncTracker.COL_UPLOADED_AT] as? Number)?.toLong() | |
| ?: System.currentTimeMillis() | |
| val uploadedAt = | |
| (response[SyncTracker.COL_UPLOADED_AT] as? Number)?.toLong() | |
| ?: throw SyncException.NetworkError("Server did not return '${SyncTracker.COL_UPLOADED_AT}' after create on '${dao.tableName}'") |
| val uploadedAt = | ||
| (response[SyncTracker.COL_UPLOADED_AT] as? Number)?.toLong() | ||
| ?: System.currentTimeMillis() |
There was a problem hiding this comment.
Falling back to System.currentTimeMillis() for uploadedAt is a critical flaw that can break sync consistency. The server-generated timestamp is essential for the download cursor. Using a client-side timestamp can cause this device to miss future updates from other devices. If the server does not return uploaded_at, it must be treated as an error.
| val uploadedAt = | |
| (response[SyncTracker.COL_UPLOADED_AT] as? Number)?.toLong() | |
| ?: System.currentTimeMillis() | |
| val uploadedAt = | |
| (response[SyncTracker.COL_UPLOADED_AT] as? Number)?.toLong() | |
| ?: throw SyncException.NetworkError("Server did not return '${SyncTracker.COL_UPLOADED_AT}' after update on '${dao.tableName}'") |
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt
Outdated
Show resolved
Hide resolved
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt
Outdated
Show resolved
Hide resolved
| class ParseError( | ||
| tableName: String, | ||
| row: Map<String, Any?>, | ||
| cause: Throwable? = null, | ||
| ) : SyncException("Failed to parse row from '$tableName': $row", cause) |
There was a problem hiding this comment.
The ParseError exception message includes the entire row map, which contains raw data fetched from the server. This data often includes Personally Identifiable Information (PII) such as names, emails, or other sensitive user data. If this exception is logged (e.g., via SyncResult.toString() or by a global error handler) or displayed in the UI, it leads to a PII leak. Avoid including the entire row map in the exception message. Instead, log only non-sensitive identifiers such as the table name and the server-side ID of the row that failed to parse.
| class ParseError( | |
| tableName: String, | |
| row: Map<String, Any?>, | |
| cause: Throwable? = null, | |
| ) : SyncException("Failed to parse row from '$tableName': $row", cause) | |
| class ParseError( | |
| tableName: String, | |
| row: Map<String, Any?>, | |
| cause: Throwable? = null, | |
| ) : SyncException("Failed to parse row from '$tableName' (serverId: ${row["server_id"]})", cause) |
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.kt
Outdated
Show resolved
Hide resolved
| * ── Type parameter ─────────────────────────────────────────────────────────── | ||
| * [T] is the DTO / map type that the backend speaks. Each concrete adapter | ||
| * converts between [T] and [Map<String, Any?>] internally, so [SyncManager] | ||
| * always works with plain maps. | ||
| */ |
There was a problem hiding this comment.
The documentation comment mentions a type parameter [T] that is not present in the BackendAdapter interface definition. This comment appears to be a leftover from a previous design and should be removed to avoid confusion.
| override fun toString(): String = | ||
| "[$tableName] ↓$downloaded conflicts=$conflicts skipped=$skipped ↑$uploaded failed=$uploadFailed" + | ||
| (if (hasError) " ERROR=${error!!.message}" else "") |
There was a problem hiding this comment.
The toString() implementation uses the non-null asserted call !! on error.message. This will cause a crash if error is not null but its message is. It's safer to use the safe call operator ?..
| override fun toString(): String = | |
| "[$tableName] ↓$downloaded conflicts=$conflicts skipped=$skipped ↑$uploaded failed=$uploadFailed" + | |
| (if (hasError) " ERROR=${error!!.message}" else "") | |
| override fun toString(): String = | |
| "[$tableName] ↓$downloaded conflicts=$conflicts skipped=$skipped ↑$uploaded failed=$uploadFailed" + | |
| (if (hasError) " ERROR=${error?.message}" else "") |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt (1)
164-170: Consider handling non-numeric primitive edge cases.In
toAny(), if aJsonPrimitiveis neither a string, boolean, long, nor valid double, thedoubleaccessor will throw. While uncommon, malformed server responses could trigger this.🛡️ Defensive fix
is JsonPrimitive -> { when { isString -> content booleanOrNull != null -> boolean longOrNull != null -> long - else -> double + else -> doubleOrNull ?: content // fallback to raw string if not parseable } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt` around lines 164 - 170, The JsonPrimitive branch in toAny() can throw when calling the unsafe double accessor; change the final branch to use doubleOrNull (e.g., doubleOrNull ?: content) so malformed/non-numeric primitives don't throw—update the JsonPrimitive handling in toAny() (the checks using isString, booleanOrNull, longOrNull) to use doubleOrNull with a safe fallback to content (or another safe sentinel) instead of calling double directly.modules/motion-store/build.gradle (1)
46-50: Remove unusedauth-ktandrealtime-ktdependencies.The
SupabaseAdapterimplementation uses onlypostgrestandktor-client-cio(as the HTTP client transport). Theauth-ktandrealtime-ktdependencies are not imported or referenced anywhere in the module. Remove them to avoid unnecessary dependency bloat, or defer their addition until they are actually needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/build.gradle` around lines 46 - 50, Remove the unused Supabase dependencies from the module: delete the implementation entries for libs.auth.kt and libs.realtime.kt in build.gradle because SupabaseAdapter only references postgrest and the ktor client transport (libs.postgrest.kt and libs.ktor.client.cio); keep platform(libs.supabase.bom) and the required libs.postgrest.kt and libs.ktor.client.cio, and only add auth-kt or realtime-kt later if SupabaseAdapter (or other classes) starts using their APIs.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt (1)
263-293: Consider makingfromServerRowan abstract method for compile-time safety.Extension functions cannot be overridden in Kotlin—they can only be shadowed by member functions with the same signature. If a concrete DAO forgets to implement
fromServerRow, sync will fail at runtime withNotImplementedError.Moving this to an abstract method in
SyncableDaowould catch missing implementations at compile time.Proposed refactor
In
SyncableDao.kt, add an abstract method:abstract class SyncableDao<T : SyncableEntity>(dbManager: DatabaseManager) : BaseDao<T>(dbManager) { // ... existing code ... /** * Deserialize a server map into an entity. * `@param` row The raw field map received from the server. * `@param` localId The local SQLite ID to assign (use -1 for new rows). */ abstract fun fromServerRow(row: Map<String, Any?>, localId: Long = -1L): T }Then in
SyncManager.kt, simplify to a direct call:-fun <T : SyncableEntity> SyncableDao<T>.fromServerMap( - row: Map<String, Any?>, - localId: Long = -1L, -): T = fromServerRow(row, localId) - -fun <T : SyncableEntity> SyncableDao<T>.fromServerRow( - row: Map<String, Any?>, - localId: Long, -): T = - throw NotImplementedError( - "Override fromServerRow() in ${this::class.simpleName} to deserialize server maps.", - )And update usages in
download()to calldao.fromServerRow(row, localId)directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt` around lines 263 - 293, Replace the runtime-thrown extension fromServerRow/fromServerMap with a compile-time enforced abstract method on SyncableDao: add abstract fun fromServerRow(row: Map<String, Any?>, localId: Long = -1L): T to the SyncableDao class, remove the extension implementations in SyncManager.kt, and update callers (e.g., SyncManager.download) to invoke dao.fromServerRow(row, localId) directly so DAOs must implement it at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.kt`:
- Around line 16-21: Update the KDoc to match the actual class declaration:
change the comment on DownloadedTrackerDao so it no longer claims it "operates
directly on the shared [SQLiteDatabase] rather than extending [BaseDao]" and
instead states that DownloadedTrackerDao extends BaseDao<DownloadedTracker> (and
clarify any remaining behavior such as being framework-internal or having no
sync metadata if still true). Locate the comment above the class
DownloadedTrackerDao and edit the text to accurately describe the class
inheritance and responsibilities so the KDoc and the class signature
(BaseDao<DownloadedTracker>) are consistent.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/BackendAdapter.kt`:
- Around line 12-16: The KDoc for BackendAdapter references a non-existent
generic type parameter [T]; update the comment on the BackendAdapter interface
to remove or correct that section so it no longer mentions [T] and instead
describes that the adapter works directly with Map<String, Any?> (or add a real
type parameter if you intended one). Edit the KDoc around the block that
currently reads "Type parameter ... [T] is the DTO / map type..." to either
remove those lines or replace them with a short note that BackendAdapter
converts to/from Map<String, Any?> and that SyncManager operates on plain maps.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.kt`:
- Line 3: The import of android.content.ContentValues is unused in the
SyncableEntity interface; remove the unused import statement
(android.content.ContentValues) from
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.kt
so the file no longer includes that import and the code compiles without unused
imports.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncException.kt`:
- Around line 17-22: The ParseError constructor currently embeds the full row
Map in the exception message (class ParseError, extending SyncException) which
may leak PII; change the message to omit raw row contents and instead include
non-sensitive metadata—e.g. tableName and row size or a sanitized identifier if
present (check a known key like "id" or "uid" but avoid including values that
could be PII), and keep the original cause passed to SyncException; update
ParseError to build a safe message (e.g. "Failed to parse row from
'<tableName>': <metadata>") and, if you need the raw row for local debugging,
log it separately via a debug/sanitizing utility rather than embedding it in the
exception text.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt`:
- Around line 79-92: The update method currently calls .first() on the parsed
result which will throw if the server returns an empty data set; in the update
function (client.postgrest[tableName].update(...)) replace the unsafe .first()
with a safe check (e.g., parseJsonToListOfMaps().firstOrNull() or check
result.data/null/empty) and handle the empty case explicitly — either return an
empty Map<String, Any?>, null, or throw a descriptive exception — so the method
(update) no longer crashes when no rows are returned; ensure you reference the
same symbols (update, client.postgrest[tableName], SyncTracker.COL_SERVER_ID,
parseJsonToListOfMaps) when making the change.
- Around line 68-77: The create method in SupabaseAdapter.kt currently calls
result.data.parseJsonToListOfMaps().first() which can throw
NoSuchElementException on an empty response; change this to use firstOrNull()
and handle the null case explicitly: after calling
client.postgrest[tableName].insert(...){ select() } and parsing with
parseJsonToListOfMaps(), check the result of firstOrNull and either throw a
clear IllegalStateException (or a custom exception) that includes tableName and
the input data for debugging, or return a defined fallback value (e.g., empty
map) depending on the intended behavior; update create(...) accordingly so it no
longer propagates NoSuchElementException from .first().
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt`:
- Around line 218-223: The catch blocks in SyncManager (catch (e: SyncException)
and catch (e: Exception)) are silently swallowing errors; update both handlers
to log the failure before incrementing failed so issues can be diagnosed—use the
class logger in SyncManager (or the existing logging utility) to record the
exception, the exception.message and relevant context (e.g., the current item/id
or batch info) and keep the existing failed++ behavior so the loop continues.
- Around line 244-248: The cancel() method currently cancels the internal
CoroutineScope (scope) making the SyncManager unusable for subsequent sync() or
syncTable() calls; update cancel() to recreate a fresh scope after cancellation
(e.g., cancel existing scope, reset scope = CoroutineScope(SupervisorJob() +
appropriate dispatcher) and clear/reset any state if needed) so future calls to
sync()/syncTable() succeed, and keep _status.value set to SyncStatus.Idle;
alternatively, if you choose not to recreate the scope, update the class
documentation and public API to state that SyncManager instances are single-use
after cancel() and must be discarded.
---
Nitpick comments:
In `@modules/motion-store/build.gradle`:
- Around line 46-50: Remove the unused Supabase dependencies from the module:
delete the implementation entries for libs.auth.kt and libs.realtime.kt in
build.gradle because SupabaseAdapter only references postgrest and the ktor
client transport (libs.postgrest.kt and libs.ktor.client.cio); keep
platform(libs.supabase.bom) and the required libs.postgrest.kt and
libs.ktor.client.cio, and only add auth-kt or realtime-kt later if
SupabaseAdapter (or other classes) starts using their APIs.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt`:
- Around line 164-170: The JsonPrimitive branch in toAny() can throw when
calling the unsafe double accessor; change the final branch to use doubleOrNull
(e.g., doubleOrNull ?: content) so malformed/non-numeric primitives don't
throw—update the JsonPrimitive handling in toAny() (the checks using isString,
booleanOrNull, longOrNull) to use doubleOrNull with a safe fallback to content
(or another safe sentinel) instead of calling double directly.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt`:
- Around line 263-293: Replace the runtime-thrown extension
fromServerRow/fromServerMap with a compile-time enforced abstract method on
SyncableDao: add abstract fun fromServerRow(row: Map<String, Any?>, localId:
Long = -1L): T to the SyncableDao class, remove the extension implementations in
SyncManager.kt, and update callers (e.g., SyncManager.download) to invoke
dao.fromServerRow(row, localId) directly so DAOs must implement it at compile
time.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
gradle/libs.versions.tomlmodules/motion-store/build.gradlemodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/SyncableDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/BackendAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncException.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncResult.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/DownloadedTracker.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/SyncTracker.kt
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.kt
Outdated
Show resolved
Hide resolved
| * ── Type parameter ─────────────────────────────────────────────────────────── | ||
| * [T] is the DTO / map type that the backend speaks. Each concrete adapter | ||
| * converts between [T] and [Map<String, Any?>] internally, so [SyncManager] | ||
| * always works with plain maps. | ||
| */ |
There was a problem hiding this comment.
Documentation references non-existent type parameter [T].
The KDoc mentions a type parameter [T] but the interface doesn't define one. The interface works directly with Map<String, Any?>. Consider removing or correcting this documentation section.
📝 Proposed fix
* [SyncManager] only ever calls these four methods — swapping backends
* means swapping the [BackendAdapter] implementation, nothing else.
*
* All methods are suspend functions; call them from a coroutine scope.
- *
- * ── Type parameter ───────────────────────────────────────────────────────────
- * [T] is the DTO / map type that the backend speaks. Each concrete adapter
- * converts between [T] and [Map<String, Any?>] internally, so [SyncManager]
- * always works with plain maps.
*/
interface BackendAdapter {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * ── Type parameter ─────────────────────────────────────────────────────────── | |
| * [T] is the DTO / map type that the backend speaks. Each concrete adapter | |
| * converts between [T] and [Map<String, Any?>] internally, so [SyncManager] | |
| * always works with plain maps. | |
| */ | |
| * [SyncManager] only ever calls these four methods — swapping backends | |
| * means swapping the [BackendAdapter] implementation, nothing else. | |
| * | |
| * All methods are suspend functions; call them from a coroutine scope. | |
| */ | |
| interface BackendAdapter { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/BackendAdapter.kt`
around lines 12 - 16, The KDoc for BackendAdapter references a non-existent
generic type parameter [T]; update the comment on the BackendAdapter interface
to remove or correct that section so it no longer mentions [T] and instead
describes that the adapter works directly with Map<String, Any?> (or add a real
type parameter if you intended one). Edit the KDoc around the block that
currently reads "Type parameter ... [T] is the DTO / map type..." to either
remove those lines or replace them with a short note that BackendAdapter
converts to/from Map<String, Any?> and that SyncManager operates on plain maps.
| @@ -0,0 +1,36 @@ | |||
| package com.tejpratapsingh.motionstore.domain | |||
|
|
|||
| import android.content.ContentValues | |||
There was a problem hiding this comment.
Remove unused import.
ContentValues is imported but not used in this interface.
Proposed fix
package com.tejpratapsingh.motionstore.domain
-import android.content.ContentValues
import com.tejpratapsingh.motionstore.tables.SyncTracker📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import android.content.ContentValues | |
| package com.tejpratapsingh.motionstore.domain | |
| import com.tejpratapsingh.motionstore.tables.SyncTracker |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.kt`
at line 3, The import of android.content.ContentValues is unused in the
SyncableEntity interface; remove the unused import statement
(android.content.ContentValues) from
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.kt
so the file no longer includes that import and the code compiles without unused
imports.
| /** A row downloaded from the server could not be parsed into a local entity. */ | ||
| class ParseError( | ||
| tableName: String, | ||
| row: Map<String, Any?>, | ||
| cause: Throwable? = null, | ||
| ) : SyncException("Failed to parse row from '$tableName': $row", cause) |
There was a problem hiding this comment.
Avoid logging raw row data in exception messages.
Including the entire row map in the exception message could expose sensitive user data (PII) in crash logs, error reporting tools, or log aggregators. Consider logging only non-sensitive metadata like row count or a sanitized identifier.
🛡️ Proposed fix
/** A row downloaded from the server could not be parsed into a local entity. */
class ParseError(
tableName: String,
row: Map<String, Any?>,
cause: Throwable? = null,
- ) : SyncException("Failed to parse row from '$tableName': $row", cause)
+ ) : SyncException("Failed to parse row from '$tableName' (keys: ${row.keys})", cause)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** A row downloaded from the server could not be parsed into a local entity. */ | |
| class ParseError( | |
| tableName: String, | |
| row: Map<String, Any?>, | |
| cause: Throwable? = null, | |
| ) : SyncException("Failed to parse row from '$tableName': $row", cause) | |
| /** A row downloaded from the server could not be parsed into a local entity. */ | |
| class ParseError( | |
| tableName: String, | |
| row: Map<String, Any?>, | |
| cause: Throwable? = null, | |
| ) : SyncException("Failed to parse row from '$tableName' (keys: ${row.keys})", cause) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncException.kt`
around lines 17 - 22, The ParseError constructor currently embeds the full row
Map in the exception message (class ParseError, extending SyncException) which
may leak PII; change the message to omit raw row contents and instead include
non-sensitive metadata—e.g. tableName and row size or a sanitized identifier if
present (check a known key like "id" or "uid" but avoid including values that
could be PII), and keep the original cause passed to SyncException; update
ParseError to build a safe message (e.g. "Failed to parse row from
'<tableName>': <metadata>") and, if you need the raw row for local debugging,
log it separately via a debug/sanitizing utility rather than embedding it in the
exception text.
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt
Show resolved
Hide resolved
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt
Show resolved
Hide resolved
| } catch (e: SyncException) { | ||
| // Log and continue — a single row failure must not abort the whole batch | ||
| failed++ | ||
| } catch (e: Exception) { | ||
| failed++ | ||
| } |
There was a problem hiding this comment.
Add logging for failed uploads.
The comment states "Log and continue" but exceptions are silently swallowed. Without logging, debugging sync failures becomes difficult.
Proposed fix
} catch (e: SyncException) {
- // Log and continue — a single row failure must not abort the whole batch
+ // Log and continue — a single row failure must not abort the whole batch
+ android.util.Log.w("SyncManager", "Upload failed for ${dao.tableName}/${entity.id}", e)
failed++
} catch (e: Exception) {
+ android.util.Log.w("SyncManager", "Upload failed for ${dao.tableName}/${entity.id}", e)
failed++
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (e: SyncException) { | |
| // Log and continue — a single row failure must not abort the whole batch | |
| failed++ | |
| } catch (e: Exception) { | |
| failed++ | |
| } | |
| } catch (e: SyncException) { | |
| // Log and continue — a single row failure must not abort the whole batch | |
| android.util.Log.w("SyncManager", "Upload failed for ${dao.tableName}/${entity.id}", e) | |
| failed++ | |
| } catch (e: Exception) { | |
| android.util.Log.w("SyncManager", "Upload failed for ${dao.tableName}/${entity.id}", e) | |
| failed++ | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt`
around lines 218 - 223, The catch blocks in SyncManager (catch (e:
SyncException) and catch (e: Exception)) are silently swallowing errors; update
both handlers to log the failure before incrementing failed so issues can be
diagnosed—use the class logger in SyncManager (or the existing logging utility)
to record the exception, the exception.message and relevant context (e.g., the
current item/id or batch info) and keep the existing failed++ behavior so the
loop continues.
| /** Cancel the internal coroutine scope (call from Application.onTerminate or similar). */ | ||
| fun cancel() { | ||
| scope.cancel() | ||
| _status.value = SyncStatus.Idle | ||
| } |
There was a problem hiding this comment.
Scope cannot be reused after cancel().
Calling cancel() destroys the internal CoroutineScope. Any subsequent calls to sync() or syncTable() will fail with JobCancellationException. Consider either:
- Recreating the scope after cancellation, or
- Documenting that
SyncManagerinstance is unusable aftercancel()and should be discarded
Option 1: Recreate scope on cancel
+ private var scope: CoroutineScope = createScope()
+
+ private fun createScope() = CoroutineScope(
+ SupervisorJob() + Dispatchers.IO + CoroutineName("SyncManager"),
+ )
+
/** Cancel the internal coroutine scope (call from Application.onTerminate or similar). */
fun cancel() {
scope.cancel()
+ scope = createScope()
_status.value = SyncStatus.Idle
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Cancel the internal coroutine scope (call from Application.onTerminate or similar). */ | |
| fun cancel() { | |
| scope.cancel() | |
| _status.value = SyncStatus.Idle | |
| } | |
| private var scope: CoroutineScope = createScope() | |
| private fun createScope() = CoroutineScope( | |
| SupervisorJob() + Dispatchers.IO + CoroutineName("SyncManager"), | |
| ) | |
| /** Cancel the internal coroutine scope (call from Application.onTerminate or similar). */ | |
| fun cancel() { | |
| scope.cancel() | |
| scope = createScope() | |
| _status.value = SyncStatus.Idle | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt`
around lines 244 - 248, The cancel() method currently cancels the internal
CoroutineScope (scope) making the SyncManager unusable for subsequent sync() or
syncTable() calls; update cancel() to recreate a fresh scope after cancellation
(e.g., cancel existing scope, reset scope = CoroutineScope(SupervisorJob() +
appropriate dispatcher) and clear/reset any state if needed) so future calls to
sync()/syncTable() succeed, and keep _status.value set to SyncStatus.Idle;
alternatively, if you choose not to recreate the scope, update the class
documentation and public API to state that SyncManager instances are single-use
after cancel() and must be discarded.
tejpratap46
force-pushed
the
feat/offline-first-sync
branch
from
c842fa6 to
2154bbf
Compare
| .collection(tableName) | ||
| .whereGreaterThan(SyncTracker.COL_UPLOADED_AT, since) | ||
| .whereNotEqualTo(SyncTracker.COL_UPDATED_BY, deviceId) | ||
| .orderBy(SyncTracker.COL_UPLOADED_AT, Query.Direction.ASCENDING) |
There was a problem hiding this comment.
CRITICAL: Firestore query will throw a runtime exception due to conflicting ordering constraints.
Firestore requires that when whereNotEqualTo is used, the first orderBy clause must be on the same field as the whereNotEqualTo filter. Here, whereNotEqualTo is on COL_UPDATED_BY but orderBy is on COL_UPLOADED_AT. Firestore will implicitly add orderBy(COL_UPDATED_BY) first, which conflicts with the explicit orderBy(COL_UPLOADED_AT) and will throw FirebaseFirestoreException: Invalid query at runtime.
Fix: Either remove the whereNotEqualTo filter and handle device-ID exclusion client-side, or restructure the query to order by COL_UPDATED_BY first and then COL_UPLOADED_AT:
.whereGreaterThan(SyncTracker.COL_UPLOADED_AT, since)
.whereNotEqualTo(SyncTracker.COL_UPDATED_BY, deviceId)
.orderBy(SyncTracker.COL_UPDATED_BY) // required first when using whereNotEqualTo
.orderBy(SyncTracker.COL_UPLOADED_AT, Query.Direction.ASCENDING)Note: this also requires a composite index in Firestore.
| firestore | ||
| .collection(tableName) | ||
| .document(serverId) | ||
| .set(payload) |
There was a problem hiding this comment.
WARNING: set() replaces the entire Firestore document, causing data loss.
DocumentReference.set(payload) overwrites the entire document with payload. Any fields on the server document not present in payload (e.g., fields added by Cloud Functions or other clients) will be silently deleted.
Use update() instead to perform a partial update that only modifies the provided fields:
firestore
.collection(tableName)
.document(serverId)
.update(payload as Map<String, Any>)
.await()Or use set(payload, SetOptions.merge()) to merge rather than replace.
| } | ||
|
|
||
| val code = connection.responseCode | ||
| val stream = if (code in 200..299) connection.inputStream else connection.errorStream |
There was a problem hiding this comment.
CRITICAL: connection.errorStream can return null, causing a NullPointerException.
Per the HttpURLConnection docs, getErrorStream() returns null if the connection is not connected, or if the server did not send a useful data alongside the error response (e.g., a 404 with an empty body). Calling .bufferedReader() on null will throw an NPE.
| val stream = if (code in 200..299) connection.inputStream else connection.errorStream | |
| val stream = if (code in 200..299) connection.inputStream else (connection.errorStream ?: connection.inputStream) |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt (2)
245-247:⚠️ Potential issue | 🟠 Major
cancel()makes the manager unusable for future sync calls unless scope is recreated.After canceling a scope/job, subsequent launches fail. Either recreate scope or explicitly document this instance as single-use.
In kotlinx.coroutines, can a CoroutineScope with a cancelled Job be reused for new async/launch calls, or must a new scope/job be created?♻️ One possible fix
-class SyncManager( +class SyncManager( private val deviceId: String, private val backend: BackendAdapter, private val downloadedTracker: DownloadedTrackerDao, - private val scope: CoroutineScope = - CoroutineScope( - SupervisorJob() + Dispatchers.IO + CoroutineName("SyncManager"), - ), + scope: CoroutineScope? = null, ) { + private fun createScope(): CoroutineScope = + CoroutineScope(SupervisorJob() + Dispatchers.IO + CoroutineName("SyncManager")) + + private var scope: CoroutineScope = scope ?: createScope() + /** Cancel the internal coroutine scope (call from Application.onTerminate or similar). */ fun cancel() { scope.cancel() + scope = createScope() _status.value = SyncStatus.Idle } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt` around lines 245 - 247, cancel() currently calls scope.cancel() which permanently cancels the underlying Job making this SyncManager instance unusable for future syncs; change cancel() to either (A) cancel only active children (e.g., scope.coroutineContext[Job]?.cancelChildren()) and then set _status.value = SyncStatus.Idle, or (B) recreate a fresh scope after cancellation (assign scope = CoroutineScope(SupervisorJob() + existingDispatcher) and then set _status.value = SyncStatus.Idle) so subsequent launches work; update the cancel() implementation accordingly referencing the scope and SyncStatus.Idle symbols.
152-160:⚠️ Potential issue | 🟠 MajorDirty local rows can be overwritten by server rows before upload (data loss risk).
When local is dirty, this branch can replace local unsynced edits, then upload phase no longer sends them.
💡 Suggested guard
when { + localEntity?.syncTracker?.isDirty == true -> { + // Preserve pending local edits; let upload phase handle conflict policy. + skipped++ + } // ── New row from server — never seen locally ────────────────── localEntity == null -> { val entity = try { dao.fromServerMap(row)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt` around lines 152 - 160, The branch that applies server rows when serverUpdatedOn >= localEntity.syncTracker.updatedOn currently overwrites local unsynced edits; change it to first check localEntity.syncTracker.dirty (or however dirty is represented) and if dirty do not blindly call dao.update(localEntity.id, entity) — instead increment conflicts (or mark the row for conflict resolution/merge) and skip the overwrite so the local dirty row will still be uploaded later; keep the try/catch around dao.fromServerMap and only perform dao.update when localEntity.syncTracker.dirty is false.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt (1)
59-60:⚠️ Potential issue | 🟡 MinorInitial sync can miss legacy rows with
uploaded_at= 0 or null.Using
gt(uploaded_at, since)whensince == 0skips those rows permanently.In latest supabase-kt PostgREST DSL, is `gte(column, value)` available for numeric filters, and is it valid to switch to `gte` only when `since == 0`?🛠️ Suggested adjustment
.select(Columns.ALL) { filter { - gt(SyncTracker.COL_UPLOADED_AT, since) + if (since == 0L) { + gte(SyncTracker.COL_UPLOADED_AT, since) + } else { + gt(SyncTracker.COL_UPLOADED_AT, since) + } neq(SyncTracker.COL_UPDATED_BY, deviceId) } order(SyncTracker.COL_UPLOADED_AT, Order.ASCENDING) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt` around lines 59 - 60, The query currently uses gt(SyncTracker.COL_UPLOADED_AT, since) which when since == 0 will permanently skip legacy rows with uploaded_at == 0 or null; change the filter construction in SupabaseAdapter.kt so that when since == 0 you use a condition that includes zeros and nulls (e.g. use gte(SyncTracker.COL_UPLOADED_AT, 0) combined with an OR isNull(SyncTracker.COL_UPLOADED_AT) or an explicit equality check for 0), and otherwise keep gt(SyncTracker.COL_UPLOADED_AT, since); preserve the existing neq(SyncTracker.COL_UPDATED_BY, deviceId) condition. Ensure you reference SyncTracker.COL_UPLOADED_AT and deviceId when updating the query builder logic.
🧹 Nitpick comments (4)
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/SyncTracker.kt (1)
49-56: Consider usingtrimIndent()onCOLUMNS_SQLfor cleaner embedding.The raw string literal retains indentation, which may produce extra whitespace when embedded in
CREATE TABLEstatements. While this works, calling.trimIndent()would produce cleaner SQL.📝 Proposed fix
- const val COLUMNS_SQL = """ + val COLUMNS_SQL = """ $COL_IS_DIRTY INTEGER NOT NULL DEFAULT 1, $COL_UPDATED_BY TEXT NOT NULL, $COL_CREATED_ON INTEGER NOT NULL, $COL_UPDATED_ON INTEGER NOT NULL, $COL_SERVER_ID TEXT, $COL_UPLOADED_AT INTEGER - """ + """.trimIndent()Note: This requires changing from
const valtovalsincetrimIndent()is a runtime call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/SyncTracker.kt` around lines 49 - 56, COLUMNS_SQL currently is a const raw string literal that keeps indentation; change its declaration from const val to val and call .trimIndent() on the raw string (the COLUMNS_SQL value) so the embedded SQL has no leading whitespace—update the declaration of COLUMNS_SQL in SyncTracker.kt to use val COLUMNS_SQL = """...""".trimIndent() and keep the same column names (COL_IS_DIRTY, COL_UPDATED_BY, COL_CREATED_ON, COL_UPDATED_ON, COL_SERVER_ID, COL_UPLOADED_AT).modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.kt (1)
35-35: Consider using a self-type pattern for better type safety.
withSyncTrackerreturnsSyncableEntityinstead of the concrete type, which loses type information and requires casting at call sites.📝 Suggested improvement
-interface SyncableEntity { +interface SyncableEntity<T : SyncableEntity<T>> { val id: Long val syncTracker: SyncTracker - fun withSyncTracker(tracker: SyncTracker): SyncableEntity + fun withSyncTracker(tracker: SyncTracker): T }This allows implementations to return their concrete type:
data class User(...) : SyncableEntity<User> { override fun withSyncTracker(tracker: SyncTracker): User = copy(syncTracker = tracker) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.kt` at line 35, Change SyncableEntity to use a self-type generic so implementations can return their concrete type: make SyncableEntity generic (e.g. SyncableEntity<T : SyncableEntity<T>>) and update the interface method signature from fun withSyncTracker(tracker: SyncTracker): SyncableEntity to fun withSyncTracker(tracker: SyncTracker): T; then update all implementing classes (e.g. User) to implement SyncableEntity<User> and have their withSyncTracker return the concrete type (e.g. copy(...)). Ensure SyncTracker type usage remains unchanged.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncResult.kt (1)
17-19: Remove unnecessary parentheses.The parentheses around the
ifexpression are redundant.📝 Proposed fix
override fun toString(): String = "[$tableName] ↓$downloaded conflicts=$conflicts skipped=$skipped ↑$uploaded failed=$uploadFailed" + - (if (hasError) " ERROR=${error!!.message}" else "") + if (hasError) " ERROR=${error!!.message}" else ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncResult.kt` around lines 17 - 19, In SyncResult's toString override remove the redundant parentheses around the trailing if expression: update the toString method (symbol: toString in class SyncResult) to concatenate the conditional segment without surrounding parentheses so the expression reads ...failed=$uploadFailed + if (hasError) " ERROR=${error!!.message}" else "" and preserve existing spacing and null-safety usage of hasError/error!!.message.build.gradle.kts (1)
16-16: Consider moving the plugin version to the version catalog.For consistency with other dependencies, the Google Services plugin version could be defined in
libs.versions.tomlrather than hardcoded here. This centralizes version management.📝 Suggested approach
In
gradle/libs.versions.toml:+googleServicesVersion = "4.4.4"Then in
[plugins]section:+google-services = { id = "com.google.gms.google-services", version.ref = "googleServicesVersion" }And use it here:
- id("com.google.gms.google-services") version "4.4.4" apply false + alias(libs.plugins.google.services) apply false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.gradle.kts` at line 16, Move the hardcoded plugin version out of the build script by adding a plugin entry/version in the version catalog (libs.versions.toml) and referencing it from the build script: add a plugin definition for com.google.gms.google-services in libs.versions.toml (either under [plugins] with version.ref to a [versions] key or as a plugin entry), then update the build.gradle.kts line id("com.google.gms.google-services") version "4.4.4" apply false to use the catalog-provided version (e.g., use the catalog alias via libs.plugins.<alias>.get().version or the plugins block that references the catalog alias) so the version is managed centrally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gradle/libs.versions.toml`:
- Around line 2-3: Update the BOM versions in the versions file: change the
firebaseBomVersion symbol from "34.10.0" to the valid "34.9.0" and bump the
supabaseBomVersion symbol from "3.3.0" to "3.4.0" so the project uses the
released Firebase Android BoM and the newer Supabase SDK; locate and edit the
firebaseBomVersion and supabaseBomVersion entries in gradle/libs.versions.toml
accordingly.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.kt`:
- Around line 83-98: The current setDownloadedTill uses a non-atomic
read-then-write (getDownloadedTill -> check -> insertWithOnConflict) which can
race; replace that with a single atomic upsert SQL executed against
dbManager.getDb() (or a compiled SQLiteStatement) that inserts the row for
TABLE_NAME and on conflict on COL_TABLE_NAME updates COL_DOWNLOADED_TILL to the
greater of the existing and incoming timestamp (use MAX or excluded value logic)
so concurrent callers never regress or cause redundant read-write races; modify
setDownloadedTill to execute that single conditional upsert instead of calling
getDownloadedTill and insertWithOnConflict.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/FirebaseAdapter.kt`:
- Around line 44-47: The current code uses putIfAbsent when setting
SyncTracker.COL_SERVER_ID from doc.id, which preserves any incorrect embedded
serverId; change the assignment to always overwrite the field so doc.id is the
single source of truth. In the block that builds the map from doc.data (the
expression using doc.data.orEmpty().toMutableMap().apply { ... }), replace the
putIfAbsent(SyncTracker.COL_SERVER_ID, doc.id) call with a direct put/assignment
for SyncTracker.COL_SERVER_ID so the Firestore document ID (doc.id) always
replaces any embedded value.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/RestAdapter.kt`:
- Line 41: The URL construction currently injects raw path segments (e.g.,
tableName, serverId) via string concatenation (see the val url assignment and
other URL builders) which can break routing; replace these with
Uri.Builder-based construction so path segments are properly encoded: use
Uri.parse(baseUrl).buildUpon().appendPath("sync").appendPath(tableName).appendQueryParameter("since",
since.toString()).appendQueryParameter("deviceId",
deviceId.encode()).build().toString() (and similarly use appendPath for
serverId/tableName in the other URL constructions and appendQueryParameter for
query values) so all path segments and query parameters are encoded correctly.
Ensure you update every occurrence that builds a URL (the val url at the synced
fetch, the serverId/tableName usages referenced in the comment) to use
appendPath/appendQueryParameter instead of raw string concatenation.
- Around line 75-105: The request() function must guard against a null error
stream and ensure the HttpURLConnection is disconnected; update the logic inside
request() so that after getting connection.responseCode you check whether the
chosen stream (connection.inputStream or connection.errorStream) is null and
fall back to an empty string or read from an available source before calling
bufferedReader(), and wrap connection usage in try/finally (or use runCatching)
to call connection.disconnect() in the finally block; ensure the thrown
SyncException.NetworkError still includes the HTTP code, method, url and
response text, and reference the connection variable and
SyncException.NetworkError when making the fix.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt`:
- Around line 263-293: The current top-level extension functions fromServerMap
and fromServerRow on SyncableDao are resolved statically, so calls to
fromServerRow inside fromServerMap will always hit the file-level
NotImplementedError implementation; instead, add an abstract or open member
function fun fromServerRow(row: Map<String, Any?>, localId: Long): T to the
SyncableDao interface/class (remove or keep the extension as a convenience that
delegates to the member), update fromServerMap to call the instance member
(this.fromServerRow(...)) so concrete DAO classes can override fromServerRow and
provide the deserialization logic (preserve the descriptive NotImplementedError
message in the default member if you make it open rather than abstract).
---
Duplicate comments:
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt`:
- Around line 59-60: The query currently uses gt(SyncTracker.COL_UPLOADED_AT,
since) which when since == 0 will permanently skip legacy rows with uploaded_at
== 0 or null; change the filter construction in SupabaseAdapter.kt so that when
since == 0 you use a condition that includes zeros and nulls (e.g. use
gte(SyncTracker.COL_UPLOADED_AT, 0) combined with an OR
isNull(SyncTracker.COL_UPLOADED_AT) or an explicit equality check for 0), and
otherwise keep gt(SyncTracker.COL_UPLOADED_AT, since); preserve the existing
neq(SyncTracker.COL_UPDATED_BY, deviceId) condition. Ensure you reference
SyncTracker.COL_UPLOADED_AT and deviceId when updating the query builder logic.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt`:
- Around line 245-247: cancel() currently calls scope.cancel() which permanently
cancels the underlying Job making this SyncManager instance unusable for future
syncs; change cancel() to either (A) cancel only active children (e.g.,
scope.coroutineContext[Job]?.cancelChildren()) and then set _status.value =
SyncStatus.Idle, or (B) recreate a fresh scope after cancellation (assign scope
= CoroutineScope(SupervisorJob() + existingDispatcher) and then set
_status.value = SyncStatus.Idle) so subsequent launches work; update the
cancel() implementation accordingly referencing the scope and SyncStatus.Idle
symbols.
- Around line 152-160: The branch that applies server rows when serverUpdatedOn
>= localEntity.syncTracker.updatedOn currently overwrites local unsynced edits;
change it to first check localEntity.syncTracker.dirty (or however dirty is
represented) and if dirty do not blindly call dao.update(localEntity.id, entity)
— instead increment conflicts (or mark the row for conflict resolution/merge)
and skip the overwrite so the local dirty row will still be uploaded later; keep
the try/catch around dao.fromServerMap and only perform dao.update when
localEntity.syncTracker.dirty is false.
---
Nitpick comments:
In `@build.gradle.kts`:
- Line 16: Move the hardcoded plugin version out of the build script by adding a
plugin entry/version in the version catalog (libs.versions.toml) and referencing
it from the build script: add a plugin definition for
com.google.gms.google-services in libs.versions.toml (either under [plugins]
with version.ref to a [versions] key or as a plugin entry), then update the
build.gradle.kts line id("com.google.gms.google-services") version "4.4.4" apply
false to use the catalog-provided version (e.g., use the catalog alias via
libs.plugins.<alias>.get().version or the plugins block that references the
catalog alias) so the version is managed centrally.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.kt`:
- Line 35: Change SyncableEntity to use a self-type generic so implementations
can return their concrete type: make SyncableEntity generic (e.g.
SyncableEntity<T : SyncableEntity<T>>) and update the interface method signature
from fun withSyncTracker(tracker: SyncTracker): SyncableEntity to fun
withSyncTracker(tracker: SyncTracker): T; then update all implementing classes
(e.g. User) to implement SyncableEntity<User> and have their withSyncTracker
return the concrete type (e.g. copy(...)). Ensure SyncTracker type usage remains
unchanged.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncResult.kt`:
- Around line 17-19: In SyncResult's toString override remove the redundant
parentheses around the trailing if expression: update the toString method
(symbol: toString in class SyncResult) to concatenate the conditional segment
without surrounding parentheses so the expression reads ...failed=$uploadFailed
+ if (hasError) " ERROR=${error!!.message}" else "" and preserve existing
spacing and null-safety usage of hasError/error!!.message.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/SyncTracker.kt`:
- Around line 49-56: COLUMNS_SQL currently is a const raw string literal that
keeps indentation; change its declaration from const val to val and call
.trimIndent() on the raw string (the COLUMNS_SQL value) so the embedded SQL has
no leading whitespace—update the declaration of COLUMNS_SQL in SyncTracker.kt to
use val COLUMNS_SQL = """...""".trimIndent() and keep the same column names
(COL_IS_DIRTY, COL_UPDATED_BY, COL_CREATED_ON, COL_UPDATED_ON, COL_SERVER_ID,
COL_UPLOADED_AT).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
build.gradle.ktsgradle/libs.versions.tomlmodules/motion-store/build.gradlemodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/SyncableDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/BackendAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncException.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncResult.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/FirebaseAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/RestAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/DownloadedTracker.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/SyncTracker.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/DownloadedTracker.kt
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/SyncableDao.kt
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncException.kt
| fun setDownloadedTill( | ||
| tableName: String, | ||
| timestamp: Long, | ||
| ) { | ||
| val current = getDownloadedTill(tableName) | ||
| if (timestamp <= current) return | ||
|
|
||
| val values = | ||
| ContentValues().apply { | ||
| put(COL_TABLE_NAME, tableName) | ||
| put(COL_DOWNLOADED_TILL, timestamp) | ||
| } | ||
| dbManager | ||
| .getDb() | ||
| .insertWithOnConflict(TABLE_NAME, null, values, SQLiteDatabase.CONFLICT_REPLACE) | ||
| } |
There was a problem hiding this comment.
Potential race condition in setDownloadedTill.
The read-then-conditional-write pattern (getDownloadedTill → check → insert) is not atomic. Concurrent calls could both pass the timestamp check and perform redundant writes. While CONFLICT_REPLACE prevents crashes, this could lead to unexpected behavior if multiple sync operations run concurrently.
Consider using a single SQL statement with a conditional update:
📝 Proposed atomic upsert
fun setDownloadedTill(
tableName: String,
timestamp: Long,
) {
- val current = getDownloadedTill(tableName)
- if (timestamp <= current) return
-
- val values =
- ContentValues().apply {
- put(COL_TABLE_NAME, tableName)
- put(COL_DOWNLOADED_TILL, timestamp)
- }
- dbManager
- .getDb()
- .insertWithOnConflict(TABLE_NAME, null, values, SQLiteDatabase.CONFLICT_REPLACE)
+ dbManager.getDb().execSQL(
+ """
+ INSERT INTO $TABLE_NAME ($COL_TABLE_NAME, $COL_DOWNLOADED_TILL)
+ VALUES (?, ?)
+ ON CONFLICT($COL_TABLE_NAME) DO UPDATE SET
+ $COL_DOWNLOADED_TILL = MAX($COL_DOWNLOADED_TILL, excluded.$COL_DOWNLOADED_TILL)
+ """.trimIndent(),
+ arrayOf(tableName, timestamp)
+ )
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.kt`
around lines 83 - 98, The current setDownloadedTill uses a non-atomic
read-then-write (getDownloadedTill -> check -> insertWithOnConflict) which can
race; replace that with a single atomic upsert SQL executed against
dbManager.getDb() (or a compiled SQLiteStatement) that inserts the row for
TABLE_NAME and on conflict on COL_TABLE_NAME updates COL_DOWNLOADED_TILL to the
greater of the existing and incoming timestamp (use MAX or excluded value logic)
so concurrent callers never regress or cause redundant read-write races; modify
setDownloadedTill to execute that single conditional upsert instead of calling
getDownloadedTill and insertWithOnConflict.
| doc.data.orEmpty().toMutableMap().apply { | ||
| // Expose Firestore document ID as serverId for consistency | ||
| putIfAbsent(SyncTracker.COL_SERVER_ID, doc.id) | ||
| } |
There was a problem hiding this comment.
Use doc.id as the source of truth for serverId (don’t keep embedded value).
putIfAbsent can preserve an incorrect serverId present in document fields. That can break local matching and updates.
✅ Proposed fix
doc.data.orEmpty().toMutableMap().apply {
// Expose Firestore document ID as serverId for consistency
- putIfAbsent(SyncTracker.COL_SERVER_ID, doc.id)
+ put(SyncTracker.COL_SERVER_ID, doc.id)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| doc.data.orEmpty().toMutableMap().apply { | |
| // Expose Firestore document ID as serverId for consistency | |
| putIfAbsent(SyncTracker.COL_SERVER_ID, doc.id) | |
| } | |
| doc.data.orEmpty().toMutableMap().apply { | |
| // Expose Firestore document ID as serverId for consistency | |
| put(SyncTracker.COL_SERVER_ID, doc.id) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/FirebaseAdapter.kt`
around lines 44 - 47, The current code uses putIfAbsent when setting
SyncTracker.COL_SERVER_ID from doc.id, which preserves any incorrect embedded
serverId; change the assignment to always overwrite the field so doc.id is the
single source of truth. In the block that builds the map from doc.data (the
expression using doc.data.orEmpty().toMutableMap().apply { ... }), replace the
putIfAbsent(SyncTracker.COL_SERVER_ID, doc.id) call with a direct put/assignment
for SyncTracker.COL_SERVER_ID so the Firestore document ID (doc.id) always
replaces any embedded value.
| since: Long, | ||
| deviceId: String, | ||
| ): List<Map<String, Any?>> { | ||
| val url = "$baseUrl/sync/$tableName?since=$since&deviceId=${deviceId.encode()}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/RestAdapter.ktRepository: tejpratap46/AndroidVideoMotion
Length of output: 4830
🌐 Web query:
Java Android URLEncoder path segment encoding best practice vs query parameter encoding
💡 Result:
Key point
URLEncoder is for HTML form / query-string encoding (application/x-www-form-urlencoded), not for encoding URI path segments. It turns spaces into +, which is appropriate in query strings but not generally correct for paths (where + is a literal plus). [2]
Android best practice
Encode a path segment
Use android.net.Uri.Builder.appendPath(...), which encodes the segment and appends it to the path. [1]
If you already have an encoded segment, use appendEncodedPath(...) instead. [1]
Uri u = new Uri.Builder()
.scheme("https")
.authority("api.example.com")
.appendPath("users")
.appendPath(userId) // encodes as a *path segment*
.build();You can also use Uri.encode(s) / Uri.encode(s, allow) when you specifically need “encode as a URI component” behavior. [3]
Encode a query parameter
Use Uri.Builder.appendQueryParameter(key, value), which encodes key and value correctly for the query portion. [1]
Uri u = new Uri.Builder()
.scheme("https")
.authority("api.example.com")
.appendPath("search")
.appendQueryParameter("q", query)
.build();Why not URLEncoder for paths?
URI components have different escaping rules (RFC 3986 defines component structure and percent-encoding). [4] In particular, URLEncoder’s + behavior is a classic foot-gun for paths because + is a valid character in a path segment and should not be used to mean space there. [2]
Practical rule of thumb
- Building URLs/URIs in Android: prefer
Uri.Builder(appendPath,appendQueryParameter). [1] - Only use
URLEncoderwhen you are explicitly producing/consumingapplication/x-www-form-urlencoded(typical query-string style), and you’re sure that’s what the server expects. [2]
If you share an example input (e.g., userId or a filename that may contain /, spaces, +, %), I can show the exact encoded output you should expect for path vs query.
Encode path segments (tableName, serverId) before inserting into URL.
Raw path segments can contain characters that break routing (e.g., /, ?, &). java.net.URLEncoder is designed for query-string encoding (converts space to +), not path segments (should be %20). For Android, the recommended approach is Uri.Builder.appendPath(), but if staying with string construction, encode path segments and query values separately:
🔧 Suggested fix
- val url = "$baseUrl/sync/$tableName?since=$since&deviceId=${deviceId.encode()}"
+ val url = "$baseUrl/sync/${tableName.encodePath()}?since=$since&deviceId=${deviceId.encodeQuery()}"
- val url = "$baseUrl/sync/$tableName"
+ val url = "$baseUrl/sync/${tableName.encodePath()}"
- val url = "$baseUrl/sync/$tableName/$serverId"
+ val url = "$baseUrl/sync/${tableName.encodePath()}/${serverId.encodePath()}"
- val url = "$baseUrl/sync/$tableName/$serverId"
+ val url = "$baseUrl/sync/${tableName.encodePath()}/${serverId.encodePath()}"
- private fun String.encode() = java.net.URLEncoder.encode(this, "UTF-8")
+ private fun String.encodeQuery() = java.net.URLEncoder.encode(this, "UTF-8")
+ private fun String.encodePath() = encodeQuery().replace("+", "%20")Affects lines 41, 60, 69, 109.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/RestAdapter.kt`
at line 41, The URL construction currently injects raw path segments (e.g.,
tableName, serverId) via string concatenation (see the val url assignment and
other URL builders) which can break routing; replace these with
Uri.Builder-based construction so path segments are properly encoded: use
Uri.parse(baseUrl).buildUpon().appendPath("sync").appendPath(tableName).appendQueryParameter("since",
since.toString()).appendQueryParameter("deviceId",
deviceId.encode()).build().toString() (and similarly use appendPath for
serverId/tableName in the other URL constructions and appendQueryParameter for
query values) so all path segments and query parameters are encoded correctly.
Ensure you update every occurrence that builds a URL (the val url at the synced
fetch, the serverId/tableName usages referenced in the comment) to use
appendPath/appendQueryParameter instead of raw string concatenation.
| private suspend fun request( | ||
| method: String, | ||
| url: String, | ||
| body: JSONObject? = null, | ||
| ): String = | ||
| withContext(Dispatchers.IO) { | ||
| val connection = | ||
| (URL(url).openConnection() as HttpURLConnection).apply { | ||
| requestMethod = method | ||
| setRequestProperty("Content-Type", "application/json") | ||
| setRequestProperty("Accept", "application/json") | ||
| authTokenProvider()?.let { setRequestProperty("Authorization", "Bearer $it") } | ||
| connectTimeout = 15_000 | ||
| readTimeout = 15_000 | ||
| if (body != null) { | ||
| doOutput = true | ||
| OutputStreamWriter(outputStream).use { it.write(body.toString()) } | ||
| } | ||
| } | ||
|
|
||
| val code = connection.responseCode | ||
| val stream = if (code in 200..299) connection.inputStream else connection.errorStream | ||
| val responseText = stream.bufferedReader().use { it.readText() } | ||
|
|
||
| if (code !in 200..299) { | ||
| throw SyncException.NetworkError( | ||
| "HTTP $code on $method $url — $responseText", | ||
| ) | ||
| } | ||
| responseText | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
HttpURLConnection errorStream null non-2xx responses Java API behavior
💡 Result:
HttpURLConnection.getErrorStream() is allowed to be null even when the HTTP status is non‑2xx.
Why getErrorStream() can be null
Per the JDK API docs, getErrorStream() returns an error body only if the connection is connected and the server “sent useful data”; it returns null if the connection wasn’t connected, there was no error, or the server sent no error data/body. [1]
So for many error responses (or for servers/proxies that send no body), getErrorStream() is legitimately null.
Typical pattern for reading bodies on non-2xx
- Call
getResponseCode()(this also helps ensure the connection is established). - For < 400, read
getInputStream(). - For >= 400, read
getErrorStream()(but it may benull).
HttpURLConnection c = (HttpURLConnection) url.openConnection();
int code = c.getResponseCode(); // forces connect
InputStream body =
(code < 400) ? c.getInputStream()
: c.getErrorStream(); // may be null
if (body != null) {
try (body) { body.transferTo(OutputStream.nullOutputStream()); }
}Why you should still try to read/close it
Oracle’s HTTP keep-alive guidance recommends: if getInputStream() throws (common for HTTP >= 400), catch the exception, call getErrorStream() “(if there is any)”, read/close it to help clean up and allow connection reuse. [2]
Sources:
[1] JDK HttpURLConnection#getErrorStream Javadoc (return-null conditions) (docs.oracle.com)
[2] Oracle “HTTP Persistent Connections (keep-alive)” guide (read error stream after IOException) (docs.oracle.com)
Citations:
- 1: https://docs.oracle.com/en/java/javase/12/docs/api/java.base/java/net/HttpURLConnection.html?utm_source=openai
- 2: https://docs.oracle.com/javase/6/docs/technotes/guides/net/http-keepalive.html
🌐 Web query:
HttpURLConnection disconnect() finally block resource cleanup best practices
💡 Result:
Best practices for HttpURLConnection cleanup (disconnect() + finally)
-
Always close the response stream (prefer
try-with-resources).
Closing theInputStream/OutputStreamis the primary way to release per-request resources and allow connection reuse (keep-alive/pooling). The JDK docs explicitly call out that closing the streams may free resources for that instance while shared persistent connections may remain reusable. [1] -
Call
disconnect()in afinallyblock as a “last step” cleanup signal—don’t rely on it instead of closing streams.
disconnect()is intended to indicate you’re unlikely to make more requests to that server soon, and it may close the underlying socket if a persistent connection is idle. It does not mean theHttpURLConnectioncan be reused. [1] -
Do not return an
InputStreamfrom a method if you plan todisconnect()infinally.
AnInputStreamis tied to the connection; disconnecting (or otherwise cleaning up) makes “return the stream but close the connection” fundamentally unsafe/impossible—read the data inside the method instead, or return a wrapper that closes the connection when the stream is closed. [2], [3] -
If the request fails, close the error stream too (when present).
Otherwise the underlying connection may not be released promptly. [4] -
Order matters: read/close streams first, then
disconnect()(if you use it).
Callingdisconnect()too early and then trying to read is a common cause of “stream closed”/empty reads. [5]
Recommended pattern (GET)
HttpURLConnection conn = null;
try {
conn = (HttpURLConnection) url.openConnection();
conn.setConnectTimeout(10_000);
conn.setReadTimeout(10_000);
int code = conn.getResponseCode();
InputStream raw = (code >= 400) ? conn.getErrorStream() : conn.getInputStream();
try (InputStream in = raw) {
// read fully from 'in' (or copy to byte[]/String)
}
} finally {
if (conn != null) conn.disconnect();
}Rule of thumb: Close streams every time; use disconnect() as belt-and-suspenders cleanup / “don’t keep this around” hint, not as your primary resource-release mechanism. [1], [4]
Sources:
[1] Oracle JDK HttpURLConnection Javadoc (close streams, persistent connections, disconnect() behavior) (docs.oracle.com)
[2] Stack Overflow: cannot safely return InputStream after disconnect() (stackoverflow.com)
[3] Stack Overflow: wrapping InputStream to close connection on stream close (stackoverflow.com)
[4] Stack Overflow: safe use; close streams; close error stream; try-with-resources (stackoverflow.com)
[5] Stack Overflow: “disconnecting then expecting to read” is wrong; close ordering (stackoverflow.com)
Citations:
- 1: https://docs.oracle.com/en/java/javase/12/docs/api/java.base/java/net/HttpURLConnection.html?utm_source=openai
- 2: https://stackoverflow.com/questions/12244002/does-httpurlconnection-disconnect-close-the-inputstream?utm_source=openai
- 3: https://stackoverflow.com/questions/67888338/close-httpurlconnection-without-closing-inputstream?utm_source=openai
- 4: https://stackoverflow.com/questions/4767553/safe-use-of-httpurlconnection?utm_source=openai
- 5: https://stackoverflow.com/questions/45854033/ioexception-stream-is-closed-nullpointerexception-with-httpurlconnection-disco?utm_source=openai
🏁 Script executed:
# Check if there are other uses of errorStream or HttpURLConnection in the codebase
rg "errorStream" -t kt -t javaRepository: tejpratap46/AndroidVideoMotion
Length of output: 100
🏁 Script executed:
# Check for other HTTP handling patterns in the codebase
rg "HttpURLConnection" -t kt -t java -A 5 -B 2Repository: tejpratap46/AndroidVideoMotion
Length of output: 100
🏁 Script executed:
# Check if there's any documentation or comments about error handling
rg "SyncException|NetworkError" -t kt -t java -B 3 -A 3Repository: tejpratap46/AndroidVideoMotion
Length of output: 100
🏁 Script executed:
# Search for errorStream usage without type filter
rg "errorStream" -A 3 -B 2Repository: tejpratap46/AndroidVideoMotion
Length of output: 856
🏁 Script executed:
# Search for HttpURLConnection patterns
rg "HttpURLConnection" -A 5 -B 2Repository: tejpratap46/AndroidVideoMotion
Length of output: 3298
🏁 Script executed:
# Look for the RestAdapter file directly
find . -name "RestAdapter.kt" -o -name "RestAdapter.java"Repository: tejpratap46/AndroidVideoMotion
Length of output: 162
🏁 Script executed:
# Check the actual request method in the file
find . -name "RestAdapter.kt" -exec cat {} \; | head -150Repository: tejpratap46/AndroidVideoMotion
Length of output: 4830
request() must handle nullable streams and explicitly disconnect() connection.
errorStream is legitimately null for many error responses (per JDK HttpURLConnection API docs). The current code stream.bufferedReader() will throw NullPointerException before your SyncException is thrown, breaking error handling. Additionally, disconnect() should be called in a finally block to ensure proper resource cleanup and allow connection reuse (per Oracle best practices).
Recommended fix
private suspend fun request(
method: String,
url: String,
body: JSONObject? = null,
): String =
withContext(Dispatchers.IO) {
- val connection =
- (URL(url).openConnection() as HttpURLConnection).apply {
- requestMethod = method
- setRequestProperty("Content-Type", "application/json")
- setRequestProperty("Accept", "application/json")
- authTokenProvider()?.let { setRequestProperty("Authorization", "Bearer $it") }
- connectTimeout = 15_000
- readTimeout = 15_000
- if (body != null) {
- doOutput = true
- OutputStreamWriter(outputStream).use { it.write(body.toString()) }
- }
- }
-
- val code = connection.responseCode
- val stream = if (code in 200..299) connection.inputStream else connection.errorStream
- val responseText = stream.bufferedReader().use { it.readText() }
-
- if (code !in 200..299) {
- throw SyncException.NetworkError(
- "HTTP $code on $method $url — $responseText",
- )
- }
- responseText
+ val connection = URL(url).openConnection() as HttpURLConnection
+ try {
+ connection.apply {
+ requestMethod = method
+ setRequestProperty("Content-Type", "application/json")
+ setRequestProperty("Accept", "application/json")
+ authTokenProvider()?.let { setRequestProperty("Authorization", "Bearer $it") }
+ connectTimeout = 15_000
+ readTimeout = 15_000
+ if (body != null) {
+ doOutput = true
+ OutputStreamWriter(outputStream, Charsets.UTF_8).use { it.write(body.toString()) }
+ }
+ }
+
+ val code = connection.responseCode
+ val stream = if (code in 200..299) connection.inputStream else connection.errorStream
+ val responseText = stream?.bufferedReader()?.use { it.readText() }.orEmpty()
+
+ if (code !in 200..299) {
+ throw SyncException.NetworkError("HTTP $code on $method $url — $responseText")
+ }
+ responseText
+ } finally {
+ connection.disconnect()
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/RestAdapter.kt`
around lines 75 - 105, The request() function must guard against a null error
stream and ensure the HttpURLConnection is disconnected; update the logic inside
request() so that after getting connection.responseCode you check whether the
chosen stream (connection.inputStream or connection.errorStream) is null and
fall back to an empty string or read from an available source before calling
bufferedReader(), and wrap connection usage in try/finally (or use runCatching)
to call connection.disconnect() in the finally block; ensure the thrown
SyncException.NetworkError still includes the HTTP code, method, url and
response text, and reference the connection variable and
SyncException.NetworkError when making the fix.
| private fun JSONArray.toListOfMaps(): List<Map<String, Any?>> = (0 until length()).map { getJSONObject(it).toMap() } | ||
|
|
||
| private fun JSONObject.toMap(): Map<String, Any?> = | ||
| keys().asSequence().associateWith { key -> | ||
| when (val v = get(key)) { | ||
| JSONObject.NULL -> null | ||
| is JSONObject -> v.toMap() | ||
| is JSONArray -> v.toListOfMaps() | ||
| else -> v |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the file to confirm the code at lines 116-124
fd "RestAdapter.kt" --type fRepository: tejpratap46/AndroidVideoMotion
Length of output: 160
🏁 Script executed:
# Read the RestAdapter.kt file to examine the code at lines 116-124
cat -n modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/RestAdapter.kt | sed -n '110,130p'Repository: tejpratap46/AndroidVideoMotion
Length of output: 829
🏁 Script executed:
# Search for usages of toListOfMaps() and JSONArray in the codebase
rg "toListOfMaps\|JSONArray" --type kotlin --type java -B 2 -A 2Repository: tejpratap46/AndroidVideoMotion
Length of output: 56
🌐 Web query:
org.json JSONArray.getJSONObject() throws exception non-object element
💡 Result:
org.json.JSONArray.getJSONObject(i) only works when the element at index i is a real JSON object ({ ... }). If the element is a string/number/boolean/null/array (or a string containing JSON text like "{\"a\":1}"), getJSONObject(i) throws org.json.JSONException: JSONArray[i] is not a JSONObject (often described as “non-object element”). [1][2][3]
How to fix
- Inspect the actual element type first
Object v = arr.get(i); // may be JSONObject, JSONArray, String, Number, ...
if (v instanceof JSONObject jo) {
// OK
} else if (v instanceof JSONArray ja) {
// it's an array at i
} else {
// it's a primitive (String/Number/Boolean/null)
}(Using get(i) + instanceof avoids the immediate exception.) [4]
- If the element is a JSON object encoded as a string, parse it
This common payload mistake looks like:"users":"[...]"or["{ \"_id\": 40, ... }", "{ ... }"](objects wrapped in quotes). In that case:
Object v = arr.get(i);
JSONObject jo = (v instanceof JSONObject) ? (JSONObject) v
: new JSONObject((String) v); // parse stringified object[2][3]
- If it’s actually an array or primitive, call the matching getter
Example: if a key holds an array, usegetJSONArray, notgetJSONObject. [5]
If you paste the JSON for your array (or the element at the failing index), I can tell you exactly which case you have and the minimal code change.
Sources: [1][2][3][4][5]
🏁 Script executed:
# Search for where RestAdapter might be used or instantiated
rg "RestAdapter" --type kotlin --type java -B 1 -A 1Repository: tejpratap46/AndroidVideoMotion
Length of output: 390
🏁 Script executed:
# Check for all function calls within RestAdapter.kt to see how toListOfMaps is used
cat modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/RestAdapter.kt | head -150Repository: tejpratap46/AndroidVideoMotion
Length of output: 4830
Nested JSONArray parsing assumes object-only items and will crash on primitives.
JSONArray.toListOfMaps() calls getJSONObject(i) without type checking. When nested arrays (line 123 usage) contain primitives—strings, numbers, booleans, nulls—the call throws JSONException: JSONArray[i] is not a JSONObject. Valid JSON arrays can contain any type, so this is a runtime crash.
Suggested fix: add `toAnyList()` for mixed-type arrays
private fun JSONArray.toListOfMaps(): List<Map<String, Any?>> = (0 until length()).map { getJSONObject(it).toMap() }
+ private fun JSONArray.toAnyList(): List<Any?> =
+ (0 until length()).map { idx ->
+ when (val v = get(idx)) {
+ JSONObject.NULL -> null
+ is JSONObject -> v.toMap()
+ is JSONArray -> v.toAnyList()
+ else -> v
+ }
+ }
private fun JSONObject.toMap(): Map<String, Any?> =
keys().asSequence().associateWith { key ->
when (val v = get(key)) {
JSONObject.NULL -> null
is JSONObject -> v.toMap()
- is JSONArray -> v.toListOfMaps()
+ is JSONArray -> v.toAnyList()
else -> v
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun JSONArray.toListOfMaps(): List<Map<String, Any?>> = (0 until length()).map { getJSONObject(it).toMap() } | |
| private fun JSONObject.toMap(): Map<String, Any?> = | |
| keys().asSequence().associateWith { key -> | |
| when (val v = get(key)) { | |
| JSONObject.NULL -> null | |
| is JSONObject -> v.toMap() | |
| is JSONArray -> v.toListOfMaps() | |
| else -> v | |
| private fun JSONArray.toListOfMaps(): List<Map<String, Any?>> = (0 until length()).map { getJSONObject(it).toMap() } | |
| private fun JSONArray.toAnyList(): List<Any?> = | |
| (0 until length()).map { idx -> | |
| when (val v = get(idx)) { | |
| JSONObject.NULL -> null | |
| is JSONObject -> v.toMap() | |
| is JSONArray -> v.toAnyList() | |
| else -> v | |
| } | |
| } | |
| private fun JSONObject.toMap(): Map<String, Any?> = | |
| keys().asSequence().associateWith { key -> | |
| when (val v = get(key)) { | |
| JSONObject.NULL -> null | |
| is JSONObject -> v.toMap() | |
| is JSONArray -> v.toAnyList() | |
| else -> v | |
| } | |
| } |
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt
Outdated
Show resolved
Hide resolved
tejpratap46
force-pushed
the
feat/offline-first-sync
branch
from
2154bbf to
3f09c40
Compare
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt
Show resolved
Hide resolved
| val results = | ||
| daos | ||
| .map { dao -> | ||
| scope.async { syncTable(dao) } |
There was a problem hiding this comment.
WARNING: Concurrent SQLite writes across DAOs may cause SQLiteDatabaseLockedException
scope.async { syncTable(dao) } launches all DAOs concurrently on Dispatchers.IO. Each syncTable call performs SQLite reads and writes via BaseDao. SQLite in Android supports concurrent reads but serializes writes — if two DAOs attempt to write simultaneously and DatabaseManager does not use WAL mode or a single-writer mutex, one coroutine will receive SQLiteDatabaseLockedException (which is not a SyncException and will be caught by the generic Exception handler, silently failing the table).
Either:
- Enable WAL mode on the database (
db.enableWriteAheadLogging()), or - Use a
Mutexaround write operations inBaseDao, or - Run DAOs sequentially instead of concurrently if WAL is not enabled.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
gradle/libs.versions.toml (1)
2-3:⚠️ Potential issue | 🔴 CriticalUse published BOM versions to avoid resolution failures.
Line 2 sets
firebaseBomVersion = "34.10.0", which appears unpublished in Firebase’s official Android release notes (latest shown there is34.7.0, updated December 11, 2025). This can break Gradle resolution.
Line 3 (3.3.0) is valid but behind Supabase’s latest release (3.4.0, February 25, 2026).🔧 Suggested update
-firebaseBomVersion = "34.10.0" -supabaseBomVersion = "3.3.0" +firebaseBomVersion = "34.7.0" +supabaseBomVersion = "3.4.0"#!/bin/bash set -euo pipefail echo "Firebase BoM published versions:" curl -fsSL https://firebase.google.com/support/release-notes/android \ | rg -n "Firebase Android BoM \\(Bill of Materials\\) version|Latest SDK versions|firebase-bom" -n echo echo "Supabase-kt latest release tag:" curl -fsSL https://github.com/supabase-community/supabase-kt/releases \ | rg -n "Releases ·|Latest|3\\.4\\.0|3\\.3\\.0" -n | head -n 20Sources:
- https://firebase.google.com/support/release-notes/android
- https://github.com/supabase-community/supabase-kt/releases
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle/libs.versions.toml` around lines 2 - 3, The versions in libs.versions.toml are out of sync with published BOMs: update firebaseBomVersion from "34.10.0" to the published Firebase Android BoM version (e.g., "34.7.0") and bump supabaseBomVersion from "3.3.0" to the current supabase-kt release ("3.4.0"); modify the firebaseBomVersion and supabaseBomVersion entries in the file accordingly to use published, resolvable BOM versions.
🧹 Nitpick comments (6)
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.kt (1)
152-155: Redundant.toString()call on String parameter.The
idparameter is already aString, so callingid.toString()is unnecessary. The same issue exists indeleteByIdat line 172.🧹 Proposed fix
fun update( id: String, entity: T, - ): Int = db.update(tableName, toContentValues(entity), "$primaryKey = ?", arrayOf(id.toString())) + ): Int = db.update(tableName, toContentValues(entity), "$primaryKey = ?", arrayOf(id))And for
deleteById:- fun deleteById(id: String): Int = db.delete(tableName, "$primaryKey = ?", arrayOf(id.toString())) + fun deleteById(id: String): Int = db.delete(tableName, "$primaryKey = ?", arrayOf(id))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.kt` around lines 152 - 155, The update method unnecessarily calls id.toString() on a parameter already typed as String; remove the redundant .toString() so the whereArgs array uses id directly in update (function update) and make the same change in deleteById so deleteById uses id without calling .toString(); keep tableName and primaryKey usage unchanged.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfoInitializer.kt (1)
22-26: Consider safer null handling forcontext.While Android guarantees
contextis non-null inContentProvider.onCreate(), using!!is generally discouraged. Consider usingrequireNotNullfor a clearer error message if this assumption ever fails.🧹 Proposed fix
override fun onCreate(): Boolean { - DeviceInfo.initialize(context!!) + DeviceInfo.initialize(requireNotNull(context) { "Context must not be null in ContentProvider.onCreate()" }) return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfoInitializer.kt` around lines 22 - 26, The onCreate implementation in DeviceInfoInitializer uses the unsafe non-null assertion context!! when calling DeviceInfo.initialize; replace this with a safer null-check like using requireNotNull(context) with a clear message (or use context?.let { DeviceInfo.initialize(it) ?: return false }) so that DeviceInfo.initialize receives a non-null Context but a clearer, controlled exception/message is produced if context is unexpectedly null; update the call in DeviceInfoInitializer.onCreate accordingly.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt (1)
166-172: JSON primitive parsing may throw on unexpected numeric types.The
toAny()function falls through todoublewhenlongOrNullis null, but if the value is a string that looks numeric or another unexpected type, this could throw. The current fallback chain is reasonable for most cases, but consider adding a finalelse -> contentfallback for robustness.♻️ Proposed fix
is JsonPrimitive -> { when { isString -> content booleanOrNull != null -> boolean longOrNull != null -> long - else -> double + else -> doubleOrNull ?: content } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt` around lines 166 - 172, In the JsonPrimitive branch of the toAny() function update the fallback so unexpected numeric/string shapes don't cause an exception: replace the final else -> double with a safe fallback (e.g., else -> content) so that if isString, booleanOrNull, and longOrNull checks fail you return the raw content instead of forcing a double; modify the branch inside toAny() (the is JsonPrimitive -> { ... } block referencing isString, booleanOrNull, longOrNull, double, content) accordingly.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt (1)
76-85: Consider limiting concurrency for large DAO lists.
sync()launches all DAOs concurrently without limit. For many tables or resource-constrained devices, this could overwhelm the database or network. Consider usingSemaphoreorlimitedParallelismdispatcher.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt` around lines 76 - 85, The sync() method in SyncManager currently launches all DAOs concurrently (scope.async { syncTable(dao) }) which can overwhelm resources; change it to limit concurrency by introducing a configurable concurrency cap (e.g., maxConcurrent) on the SyncManager and use either a Semaphore (acquire before starting each async and release after syncTable returns) or Dispatchers.IO.limitedParallelism(maxConcurrent) when launching the coroutine for each dao; ensure you still awaitAll() and update _status the same way, and reference SyncManager.sync, scope, syncTable, and _status when making the change so the new concurrency control is applied around the map { dao -> scope.async { ... } } block.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncResult.kt (2)
17-19: Remove unnecessary parentheses.Per static analysis, the parentheses around the if expression are redundant.
♻️ Proposed fix
override fun toString(): String = "[$tableName] ↓$downloaded conflicts=$conflicts skipped=$skipped ↑$uploaded failed=$uploadFailed" + - (if (hasError) " ERROR=${error!!.message}" else "") + if (hasError) " ERROR=${error!!.message}" else ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncResult.kt` around lines 17 - 19, The toString override in SyncResult uses redundant parentheses around the if expression; update the override (method toString in class SyncResult) to remove the unnecessary parentheses so the concatenation uses the bare if (hasError) ... else "" expression and still appends the error message when present (reference hasError and error!!.message).
23-24: Consider usingdata objectforIdleto benefit from improvedtoString()output.Kotlin 2.3.10 (your project's version) fully supports
data object, which was stabilized in Kotlin 1.9.0. Usingdata objectwill automatically generate a cleanertoString()implementation returning"Idle"instead of"SyncStatus$Idle@hashcode". This is also consistent with the other subclasses (Running,Completed,Failed) which already usedata class.♻️ Proposed fix
sealed class SyncStatus { - object Idle : SyncStatus() + data object Idle : SyncStatus()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncResult.kt` around lines 23 - 24, Replace the plain object declaration for Idle inside the sealed class SyncStatus with a data object (i.e., change "object Idle : SyncStatus()" to "data object Idle : SyncStatus()") so it benefits from the generated toString() and matches the other data class subclasses (Running, Completed, Failed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.kt`:
- Around line 136-139: The worker currently calls
setForeground(createForegroundInfo(progressNotificationId, notification)) on
every progress update while updateNotification(...) is throttled; gate the
setForeground call behind the same lastNotificationUpdateTime check used for
updateNotification so setForeground is only invoked when you actually update the
notification (use the same timestamp comparison and update
lastNotificationUpdateTime when you call updateNotification), referencing
updateNotification, setForeground, createForegroundInfo,
lastNotificationUpdateTime, progressNotificationId and notification to locate
where to apply the change.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/MotionProjectDao.kt`:
- Around line 67-87: The fromServerRow mapping in MotionProjectDao uses unsafe
casts (e.g., row[COL_NAME] as String, (row[COL_CREATED] as Number).toLong())
that can throw at runtime; update fromServerRow to use safe casts and validation
for required fields (COL_NAME, COL_PATH, COL_CREATED, COL_UPDATED) and provide
sensible defaults or throw a clear IllegalArgumentException, and use as? for
optional fields in SyncTracker (SyncTracker.COL_UPDATED_BY, COL_CREATED_ON,
COL_UPDATED_ON, COL_UPLOADED_AT, COL_SERVER_ID) converting Numbers with
?.toLong(); alternatively wrap the parsing in a try-catch to log/handle
malformed rows rather than letting ClassCastException/NPE crash the sync.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/SyncableDao.kt`:
- Around line 19-20: Update the KDoc on SyncableDao so it references the actual
method name readSyncTracker() instead of the non-existent syncTrackerFromCursor;
specifically, change the sentence that tells concrete DAOs what to implement to
mention readSyncTracker() (and keep the mention of BaseDao unchanged) so
documentation matches the implementation in class SyncableDao.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.kt`:
- Around line 15-26: The KDoc example for SyncableEntity is inconsistent: update
the example data class User in the comment so its id property matches the
interface type (String) instead of Long; change "val id: Long = -1" to a
String-typed id (e.g., "val id: String = \"\"") and ensure any uses of
default/unsaved sentinel in the example reflect a string sentinel consistent
with SyncableEntity.id, keeping the rest of the example and the withSyncTracker
implementation unchanged.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt`:
- Around line 15-16: The MotionProject primary constructor uses a default
parameter SyncTracker(updatedBy = DeviceInfo.id) which accesses DeviceInfo.id at
instantiation and breaks unit tests if DeviceInfo isn't initialized; fix tests
or the usage by either initializing DeviceInfo in test setup (call
DeviceInfo.initialize(...) before creating MotionProject) or always supplying an
explicit syncTracker in test fixtures (create SyncTracker(...) and pass it into
MotionProject), and update any test factories/fixtures that construct
MotionProject to stop relying on the implicit DeviceInfo.id default.
---
Duplicate comments:
In `@gradle/libs.versions.toml`:
- Around line 2-3: The versions in libs.versions.toml are out of sync with
published BOMs: update firebaseBomVersion from "34.10.0" to the published
Firebase Android BoM version (e.g., "34.7.0") and bump supabaseBomVersion from
"3.3.0" to the current supabase-kt release ("3.4.0"); modify the
firebaseBomVersion and supabaseBomVersion entries in the file accordingly to use
published, resolvable BOM versions.
---
Nitpick comments:
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.kt`:
- Around line 152-155: The update method unnecessarily calls id.toString() on a
parameter already typed as String; remove the redundant .toString() so the
whereArgs array uses id directly in update (function update) and make the same
change in deleteById so deleteById uses id without calling .toString(); keep
tableName and primaryKey usage unchanged.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncResult.kt`:
- Around line 17-19: The toString override in SyncResult uses redundant
parentheses around the if expression; update the override (method toString in
class SyncResult) to remove the unnecessary parentheses so the concatenation
uses the bare if (hasError) ... else "" expression and still appends the error
message when present (reference hasError and error!!.message).
- Around line 23-24: Replace the plain object declaration for Idle inside the
sealed class SyncStatus with a data object (i.e., change "object Idle :
SyncStatus()" to "data object Idle : SyncStatus()") so it benefits from the
generated toString() and matches the other data class subclasses (Running,
Completed, Failed).
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfoInitializer.kt`:
- Around line 22-26: The onCreate implementation in DeviceInfoInitializer uses
the unsafe non-null assertion context!! when calling DeviceInfo.initialize;
replace this with a safer null-check like using requireNotNull(context) with a
clear message (or use context?.let { DeviceInfo.initialize(it) ?: return false
}) so that DeviceInfo.initialize receives a non-null Context but a clearer,
controlled exception/message is produced if context is unexpectedly null; update
the call in DeviceInfoInitializer.onCreate accordingly.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt`:
- Around line 166-172: In the JsonPrimitive branch of the toAny() function
update the fallback so unexpected numeric/string shapes don't cause an
exception: replace the final else -> double with a safe fallback (e.g., else ->
content) so that if isString, booleanOrNull, and longOrNull checks fail you
return the raw content instead of forcing a double; modify the branch inside
toAny() (the is JsonPrimitive -> { ... } block referencing isString,
booleanOrNull, longOrNull, double, content) accordingly.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt`:
- Around line 76-85: The sync() method in SyncManager currently launches all
DAOs concurrently (scope.async { syncTable(dao) }) which can overwhelm
resources; change it to limit concurrency by introducing a configurable
concurrency cap (e.g., maxConcurrent) on the SyncManager and use either a
Semaphore (acquire before starting each async and release after syncTable
returns) or Dispatchers.IO.limitedParallelism(maxConcurrent) when launching the
coroutine for each dao; ensure you still awaitAll() and update _status the same
way, and reference SyncManager.sync, scope, syncTable, and _status when making
the change so the new concurrency control is applied around the map { dao ->
scope.async { ... } } block.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
build.gradle.ktsgradle/libs.versions.tomlmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.ktmodules/motion-store/build.gradlemodules/motion-store/src/main/AndroidManifest.xmlmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/MotionProjectDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/SyncableDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/BackendAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncException.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncResult.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfo.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfoInitializer.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/FirebaseAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/RestAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/DownloadedTracker.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/SyncTracker.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/FirebaseAdapter.kt
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncException.kt
- modules/motion-store/build.gradle
- build.gradle.kts
...maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.kt
Outdated
Show resolved
Hide resolved
| override fun fromServerRow( | ||
| row: Map<String, Any?>, | ||
| localId: String, | ||
| ) = MotionProject( | ||
| id = localId, | ||
| name = row[COL_NAME] as String, | ||
| path = row[COL_PATH] as String, | ||
| sdui = row[COL_SDUI] as? String, | ||
| metadata = row[COL_METADATA] as? String, | ||
| created = (row[COL_CREATED] as Number).toLong(), | ||
| updated = (row[COL_UPDATED] as Number).toLong(), | ||
| syncTracker = | ||
| SyncTracker( | ||
| isDirty = false, | ||
| updatedBy = row[SyncTracker.COL_UPDATED_BY] as String, | ||
| createdOn = (row[SyncTracker.COL_CREATED_ON] as Number).toLong(), | ||
| updatedOn = (row[SyncTracker.COL_UPDATED_ON] as Number).toLong(), | ||
| serverId = row[SyncTracker.COL_SERVER_ID] as? String, | ||
| uploadedAt = (row[SyncTracker.COL_UPLOADED_AT] as? Number)?.toLong(), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Unsafe casts in fromServerRow may throw at runtime.
The direct casts (as String, as Number) will throw ClassCastException or NullPointerException if the server response contains unexpected types or missing required fields. This could crash the sync process for a single malformed row.
Consider using safe casts with validation or default values, or wrapping the entire mapping in a try-catch to handle malformed server data gracefully.
🛡️ Example of safer casting pattern
override fun fromServerRow(
row: Map<String, Any?>,
localId: String,
): MotionProject {
val name = row[COL_NAME] as? String
?: throw IllegalArgumentException("Missing required field: $COL_NAME")
val path = row[COL_PATH] as? String
?: throw IllegalArgumentException("Missing required field: $COL_PATH")
return MotionProject(
id = localId,
name = name,
path = path,
sdui = row[COL_SDUI] as? String,
metadata = row[COL_METADATA] as? String,
created = (row[COL_CREATED] as? Number)?.toLong() ?: System.currentTimeMillis(),
updated = (row[COL_UPDATED] as? Number)?.toLong() ?: System.currentTimeMillis(),
syncTracker = SyncTracker(
isDirty = false,
updatedBy = row[SyncTracker.COL_UPDATED_BY] as? String ?: "",
createdOn = (row[SyncTracker.COL_CREATED_ON] as? Number)?.toLong() ?: 0L,
updatedOn = (row[SyncTracker.COL_UPDATED_ON] as? Number)?.toLong() ?: 0L,
serverId = row[SyncTracker.COL_SERVER_ID] as? String,
uploadedAt = (row[SyncTracker.COL_UPLOADED_AT] as? Number)?.toLong(),
),
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/MotionProjectDao.kt`
around lines 67 - 87, The fromServerRow mapping in MotionProjectDao uses unsafe
casts (e.g., row[COL_NAME] as String, (row[COL_CREATED] as Number).toLong())
that can throw at runtime; update fromServerRow to use safe casts and validation
for required fields (COL_NAME, COL_PATH, COL_CREATED, COL_UPDATED) and provide
sensible defaults or throw a clear IllegalArgumentException, and use as? for
optional fields in SyncTracker (SyncTracker.COL_UPDATED_BY, COL_CREATED_ON,
COL_UPDATED_ON, COL_UPLOADED_AT, COL_SERVER_ID) converting Numbers with
?.toLong(); alternatively wrap the parsing in a try-catch to log/handle
malformed rows rather than letting ClassCastException/NPE crash the sync.
| * Concrete DAOs extend this instead of [BaseDao] and additionally implement | ||
| * [syncTrackerFromCursor] to extract the tracker from a cursor row. |
There was a problem hiding this comment.
Documentation references non-existent method.
The KDoc mentions "implement [syncTrackerFromCursor] to extract the tracker" but the actual method is readSyncTracker(). Update the documentation to match the implementation.
📝 Proposed fix
* Concrete DAOs extend this instead of [BaseDao] and additionally implement
- * [syncTrackerFromCursor] to extract the tracker from a cursor row.
+ * [fromCursor] using the [readSyncTracker] extension to extract the tracker from a cursor row.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Concrete DAOs extend this instead of [BaseDao] and additionally implement | |
| * [syncTrackerFromCursor] to extract the tracker from a cursor row. | |
| * Concrete DAOs extend this instead of [BaseDao] and additionally implement | |
| * [fromCursor] using the [readSyncTracker] extension to extract the tracker from a cursor row. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/SyncableDao.kt`
around lines 19 - 20, Update the KDoc on SyncableDao so it references the actual
method name readSyncTracker() instead of the non-existent syncTrackerFromCursor;
specifically, change the sentence that tells concrete DAOs what to implement to
mention readSyncTracker() (and keep the mention of BaseDao unchanged) so
documentation matches the implementation in class SyncableDao.
| * data class User( | ||
| * val id: Long = -1, | ||
| * val name: String, | ||
| * val email: String, | ||
| * override val syncTracker: SyncTracker = SyncTracker(updatedBy = DeviceInfo.id), | ||
| * ) : SyncableEntity { | ||
| * override fun withSyncTracker(tracker: SyncTracker) = copy(syncTracker = tracker) | ||
| * } | ||
| */ | ||
| interface SyncableEntity { | ||
| /** Local SQLite primary key. -1 means not yet persisted. */ | ||
| val id: String |
There was a problem hiding this comment.
Documentation example doesn't match interface signature.
The KDoc example at line 16 shows val id: Long = -1 but the interface at line 26 declares val id: String. This inconsistency could confuse implementers. Update the example to use String for the id type.
📝 Proposed fix
* data class User(
-* val id: Long = -1,
+* val id: String = "",
* val name: String,
* val email: String,
* override val syncTracker: SyncTracker = SyncTracker(updatedBy = DeviceInfo.id),
* ) : SyncableEntity {
* override fun withSyncTracker(tracker: SyncTracker) = copy(syncTracker = tracker)
* }
*/
interface SyncableEntity {
- /** Local SQLite primary key. -1 means not yet persisted. */
+ /** Local SQLite primary key. Empty string means not yet persisted. */
val id: String🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.kt`
around lines 15 - 26, The KDoc example for SyncableEntity is inconsistent:
update the example data class User in the comment so its id property matches the
interface type (String) instead of Long; change "val id: Long = -1" to a
String-typed id (e.g., "val id: String = \"\"") and ensure any uses of
default/unsaved sentinel in the example reflect a string sentinel consistent
with SyncableEntity.id, keeping the rest of the example and the withSyncTracker
implementation unchanged.
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt
Show resolved
Hide resolved
tejpratap46
force-pushed
the
feat/offline-first-sync
branch
from
3f09c40 to
a0ee3d3
Compare
| val motionStore by lazy { MotionProjectDao(database) } | ||
| val motionStoreDao by lazy { MotionProjectDao(database) } | ||
|
|
||
| val syncManager = |
There was a problem hiding this comment.
WARNING: syncManager is eagerly initialized (not lazy), but it references database which is lazy.
Because syncManager is a non-lazy property, it is initialized when LyricsApp is instantiated — before Application.onCreate() is called. This forces database (and therefore DatabaseManager.init(this)) to run at class construction time, before the Application context is fully set up. This can cause crashes or subtle initialization-order bugs.
Consider making syncManager lazy as well:
val syncManager by lazy {
SyncManager(
backend = FirebaseAdapter(),
downloadedTracker = DownloadedTrackerDao(database),
)
}| readTimeout = 15_000 | ||
| if (body != null) { | ||
| doOutput = true | ||
| OutputStreamWriter(outputStream).use { it.write(body.toString()) } |
There was a problem hiding this comment.
WARNING: OutputStreamWriter(outputStream) uses the platform default charset, which may not be UTF-8 on all Android devices/configurations.
JSON must be encoded as UTF-8 per RFC 8259. Use an explicit charset:
| OutputStreamWriter(outputStream).use { it.write(body.toString()) } | |
| OutputStreamWriter(outputStream, Charsets.UTF_8).use { it.write(body.toString()) } |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (12)
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/BackendAdapter.kt (1)
12-15:⚠️ Potential issue | 🟡 MinorRemove non-existent type parameter from KDoc.
The docs still reference
[T], but this interface is not generic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/BackendAdapter.kt` around lines 12 - 15, The KDoc for BackendAdapter still references a non-existent type parameter [T]; update the comment in BackendAdapter.kt to remove or reword the "[T] is the DTO / map type..." sentence so it no longer mentions a generic type, and if needed clarify that concrete adapters convert between their backend DTO types and Map<String, Any?> internally (so SyncManager works with plain maps), ensuring references to SyncManager remain accurate.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/SyncableDao.kt (1)
19-20:⚠️ Potential issue | 🟡 MinorKDoc references a non-existent API.
Line [20] mentions
syncTrackerFromCursor, but the implementation usesreadSyncTracker().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/SyncableDao.kt` around lines 19 - 20, The KDoc for SyncableDao incorrectly references syncTrackerFromCursor; update the documentation to reference the actual method name readSyncTracker (and/or any other relevant API in SyncableDao/BaseDao) so the doc matches the implementation, e.g., change the mention of syncTrackerFromCursor to readSyncTracker and ensure the description matches how readSyncTracker extracts the tracker from a Cursor row.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt (1)
59-60:⚠️ Potential issue | 🟠 MajorFirst-sync filter can permanently skip legacy rows.
Using
gt(uploaded_at, since)on initial sync (since = 0) may exclude rows withuploaded_at = 0/NULL, so they never get downloaded.Safer filter logic for initial sync
filter { - gt(SyncTracker.COL_UPLOADED_AT, since) + if (since == 0L) { + gte(SyncTracker.COL_UPLOADED_AT, since) + } else { + gt(SyncTracker.COL_UPLOADED_AT, since) + } neq(SyncTracker.COL_UPDATED_BY, deviceId) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt` around lines 59 - 60, The current filter uses gt(SyncTracker.COL_UPLOADED_AT, since) which on initial sync (since == 0) will omit rows whose uploaded_at is 0 or NULL; update the predicate to include those legacy rows by replacing the strict gt check with a compound condition that accepts NULL/zero values — e.g. (isNull(SyncTracker.COL_UPLOADED_AT) OR gt(SyncTracker.COL_UPLOADED_AT, since)) or explicitly checking for uploaded_at <= 0 for the initial sync — while keeping the neq(SyncTracker.COL_UPDATED_BY, deviceId) clause unchanged so legacy rows are included for download on first sync.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.kt (1)
3-3:⚠️ Potential issue | 🟡 MinorFix stale import and inconsistent ID documentation.
ContentValuesimport is unused, and Line [16]/Line [25] still describeLong/-1while the interface usesString.Proposed documentation/import fix
package com.tejpratapsingh.motionstore.domain -import android.content.ContentValues import com.tejpratapsingh.motionstore.tables.SyncTracker @@ -* val id: Long = -1, +* val id: String = "", @@ - /** Local SQLite primary key. -1 means not yet persisted. */ + /** Local SQLite primary key. Empty string means not yet persisted. */ val id: StringAlso applies to: 16-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.kt` at line 3, Remove the unused ContentValues import and update the KDoc for the SyncableEntity identifier to match the actual type: change any references that mention Long or -1 to describe the String-based ID used by the interface (refer to SyncableEntity and its id property/method names in the file) and adjust wording to reflect the correct default/empty value semantics for a String ID.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.kt (1)
81-96:⚠️ Potential issue | 🟠 MajorMake cursor advancement atomic to avoid races.
getDownloadedTill+ conditional insert is a read/write race under concurrent sync calls and can cause redundant or regressive behavior.Atomic upsert approach
fun setDownloadedTill( tableName: String, timestamp: Long, ) { - val current = getDownloadedTill(tableName) - if (timestamp <= current) return - - val values = - ContentValues().apply { - put(COL_TABLE_NAME, tableName) - put(COL_DOWNLOADED_TILL, timestamp) - } - dbManager - .getDb() - .insertWithOnConflict(TABLE_NAME, null, values, SQLiteDatabase.CONFLICT_REPLACE) + dbManager.getDb().execSQL( + """ + INSERT INTO $TABLE_NAME ($COL_TABLE_NAME, $COL_DOWNLOADED_TILL) + VALUES (?, ?) + ON CONFLICT($COL_TABLE_NAME) DO UPDATE SET + $COL_DOWNLOADED_TILL = MAX($COL_DOWNLOADED_TILL, excluded.$COL_DOWNLOADED_TILL) + """.trimIndent(), + arrayOf(tableName, timestamp), + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.kt` around lines 81 - 96, The current setDownloadedTill reads current via getDownloadedTill then conditionally inserts, which creates a race; replace the read-then-write with a single atomic upsert SQL using dbManager.getDb() so the DB decides whether to insert or update without a separate read. Implement setDownloadedTill to run one statement that inserts (COL_TABLE_NAME, COL_DOWNLOADED_TILL) values and uses SQLite UPSERT/ON CONFLICT on COL_TABLE_NAME to update COL_DOWNLOADED_TILL to the greater of existing and incoming (e.g. DO UPDATE SET COL_DOWNLOADED_TILL = MAX(COL_DOWNLOADED_TILL, excluded.COL_DOWNLOADED_TILL)) so updates are atomic and never regress under concurrent syncs; use TABLE_NAME, COL_TABLE_NAME, COL_DOWNLOADED_TILL and dbManager.getDb() to locate/replace the logic.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt (5)
219-224:⚠️ Potential issue | 🟡 MinorAdd logging for failed uploads.
The comment states "Log and continue" but exceptions are silently swallowed. Without logging, debugging sync failures becomes difficult.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt` around lines 219 - 224, The catch blocks in SyncManager (the catch (e: SyncException) and catch (e: Exception) branches that increment failed) silently swallow errors; update both to log the exception and context before incrementing failed—use the class logger (or existing logger instance) to emit a descriptive message including the exception, the row/id being processed (where available in the scope), and whether it was a SyncException or generic Exception; ensure you call logger.error(...) (or logger.warn for recoverable cases) with the exception object so stack traces are preserved and then increment failed as currently done.
148-159:⚠️ Potential issue | 🟠 MajorDirty local changes may be silently overwritten.
The download phase runs before upload. If a local row is dirty (
isDirty = true) and the server has a row withupdatedOn >= localEntity.syncTracker.updatedOn, the server data overwrites the local dirty row at line 156. The local changes are lost and will never be uploaded.Consider checking
localEntity.syncTracker.isDirtybefore overwriting, or merge changes based on your conflict resolution policy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt` around lines 148 - 159, The code path that treats server wins (condition serverUpdatedOn >= localEntity.syncTracker.updatedOn) currently unconditionally converts the server row via dao.fromServerRow and calls dao.update, which will silently overwrite any local dirty changes; modify this branch to first check localEntity.syncTracker.isDirty and handle accordingly (e.g., skip overwriting and increment a conflict counter, attempt a merge, or enqueue a re-upload) instead of blindly calling dao.update; use the existing dao.fromServerRow only after resolving dirty-local semantics and ensure conflicts++/saved++ are adjusted to reflect your chosen resolution.
127-175:⚠️ Potential issue | 🟠 MajorDownload cursor advances before confirming all writes succeeded.
If
dao.insert()ordao.update()throws an exception (e.g., database constraint violation), the loop continues andhighWaterMarkstill advances at line 171-173. Those failed server rows will be skipped permanently on future syncs.Consider wrapping the download loop in a transaction or only advancing the cursor after all operations succeed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt` around lines 127 - 175, The loop that processes serverRows advances the download cursor via downloadedTracker.setDownloadedTill(dao.tableName, highWaterMark) even if dao.insert() or dao.update() fail, causing missed rows; modify SyncManager's download logic (the serverRows processing loop and the post-loop cursor advance) so the cursor is only advanced after all writes succeed — either wrap the entire loop in a single transaction (use your DAO/DB transaction API or a withTransaction helper) or collect/validate every row write and abort/rollback on first exception before calling downloadedTracker.setDownloadedTill; ensure exceptions from dao.fromServerRow, dao.insert, or dao.update cause a rollback and prevent updating highWaterMark/downloadedTracker, leaving DownloadStats unchanged in failure cases.
76-85:⚠️ Potential issue | 🟡 MinorConcurrent SQLite writes may cause lock contention.
The
syncfunction launches all DAO syncs concurrently viascope.async. If the underlying database doesn't use WAL mode, concurrent write operations across different DAOs may causeSQLiteDatabaseLockedException.Consider enabling WAL mode in
DatabaseManageror documenting that WAL is required for concurrent table syncs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt` around lines 76 - 85, The sync method in SyncManager launches all DAOs with scope.async which can cause SQLiteDatabaseLockedException when concurrent writes occur; either enable WAL mode in your DB setup (e.g., in DatabaseManager initialization) so concurrent writes across tables are allowed, or change SyncManager.sync to run syncTable(dao) sequentially (replace parallel scope.async usage) and document the requirement; reference SyncManager.sync, scope.async, syncTable(dao) for the async change and DatabaseManager for enabling WAL.
245-249:⚠️ Potential issue | 🟠 MajorScope cannot be reused after
cancel().Calling
cancel()destroys the internalCoroutineScope. Any subsequent calls tosync()orsyncTable()will fail withJobCancellationException. Either recreate the scope after cancellation or document thatSyncManageris unusable aftercancel().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt` around lines 245 - 249, The cancel() method cancels the internal CoroutineScope (scope) but does not recreate it, so subsequent calls to sync() or syncTable() will throw JobCancellationException; fix by either recreating a fresh CoroutineScope and Job after cancellation (e.g., reset scope = CoroutineScope(SupervisorJob() + dispatcher) and any associated Job/state) or document that SyncManager is single-use after cancel(); update cancel() to reset _status to SyncStatus.Idle and reinitialize scope and any dependent fields (referencing cancel(), scope, sync(), syncTable(), _status, and SyncManager) or add clear documentation on unusability if you choose the single-use approach.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt (1)
3-18:⚠️ Potential issue | 🟡 MinorDefault parameter relies on
DeviceInfo.idinitialization.The default value
SyncTracker(updatedBy = DeviceInfo.id)accessesDeviceInfo.idwhenever aMotionProjectis created without explicitly providingsyncTracker. Unit tests or any context whereDeviceInfoInitializer.onCreate()hasn't run will fail withIllegalStateException.Consider either:
- Using an empty default (
updatedBy = "") and lettingSyncManagerstampupdatedByat upload time (which it already does), or- Ensuring test fixtures explicitly provide
syncTrackeror callDeviceInfo.initialize()in test setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt` around lines 3 - 18, MotionProject's default syncTracker uses DeviceInfo.id which can throw IllegalStateException if DeviceInfo isn't initialized; change the default in the MotionProject constructor to use an empty updatedBy (e.g., SyncTracker(updatedBy = "")) so creation won't depend on DeviceInfo, and let SyncManager stamp the real updatedBy later, or alternately ensure tests explicitly provide a syncTracker or call DeviceInfo.initialize() in setup; update MotionProject (and any test fixtures constructing MotionProject without syncTracker) to avoid referencing DeviceInfo.id at construction time.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/MotionProjectDao.kt (1)
67-87:⚠️ Potential issue | 🟠 MajorUnsafe casts may throw at runtime.
Direct casts (
as String,as Number) will throwClassCastExceptionorNullPointerExceptionif the server response contains unexpected types or missing required fields. This could crash the sync process for a single malformed row.Use safe casts with validation for required fields:
Safer casting pattern
override fun fromServerRow( row: Map<String, Any?>, localId: String, ) = MotionProject( id = localId, - name = row[COL_NAME] as String, - path = row[COL_PATH] as String, + name = row[COL_NAME] as? String + ?: throw IllegalArgumentException("Missing required field: $COL_NAME"), + path = row[COL_PATH] as? String + ?: throw IllegalArgumentException("Missing required field: $COL_PATH"), sdui = row[COL_SDUI] as? String, metadata = row[COL_METADATA] as? String, - created = (row[COL_CREATED] as Number).toLong(), - updated = (row[COL_UPDATED] as Number).toLong(), + created = (row[COL_CREATED] as? Number)?.toLong() ?: System.currentTimeMillis(), + updated = (row[COL_UPDATED] as? Number)?.toLong() ?: System.currentTimeMillis(), syncTracker = SyncTracker( isDirty = false, - updatedBy = row[SyncTracker.COL_UPDATED_BY] as String, - createdOn = (row[SyncTracker.COL_CREATED_ON] as Number).toLong(), - updatedOn = (row[SyncTracker.COL_UPDATED_ON] as Number).toLong(), + updatedBy = row[SyncTracker.COL_UPDATED_BY] as? String ?: "", + createdOn = (row[SyncTracker.COL_CREATED_ON] as? Number)?.toLong() ?: 0L, + updatedOn = (row[SyncTracker.COL_UPDATED_ON] as? Number)?.toLong() ?: 0L, serverId = row[SyncTracker.COL_SERVER_ID] as? String, uploadedAt = (row[SyncTracker.COL_UPLOADED_AT] as? Number)?.toLong(), ), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/MotionProjectDao.kt` around lines 67 - 87, fromServerRow uses unsafe casts (as String / as Number) that can throw at runtime; change to safe casts (as? String, as? Number) and validate required fields before constructing MotionProject and SyncTracker (check COL_NAME, COL_PATH, COL_CREATED, COL_UPDATED and SyncTracker.COL_UPDATED_BY, COL_CREATED_ON, COL_UPDATED_ON exist and are the expected types), convert Number toLong via safe calls (?.toLong()), and decide a clear failure strategy (return null or throw an IllegalArgumentException with a descriptive message and log) so malformed rows don’t crash the sync; update fromServerRow and the SyncTracker construction to use these safe casts and validations referencing the same symbols (fromServerRow, MotionProject, SyncTracker, COL_NAME, COL_PATH, COL_CREATED, COL_UPDATED, SyncTracker.COL_UPDATED_BY, SyncTracker.COL_CREATED_ON, SyncTracker.COL_UPDATED_ON, SyncTracker.COL_SERVER_ID, SyncTracker.COL_UPLOADED_AT).
🧹 Nitpick comments (3)
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.kt (1)
153-156: Remove redundanttoString()onStringIDs.Line [155] already receives
id: String, soid.toString()is unnecessary noise.Small cleanup
fun update( id: String, entity: T, - ): Int = db.update(tableName, toContentValues(entity), "$primaryKey = ?", arrayOf(id.toString())) + ): Int = db.update(tableName, toContentValues(entity), "$primaryKey = ?", arrayOf(id))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.kt` around lines 153 - 156, In BaseDao.kt update call replace the redundant id.toString() with the String id directly: locate the db.update(...) invocation that currently passes arrayOf(id.toString()) (in the method taking id: String, entity: T) and change it to arrayOf(id) so you stop calling toString() on an already-String identifier.modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/LyricsApp.kt (1)
40-49:workManagerConfigurationgetter recreatesConfigurationon every access.The
get()syntax creates a newConfigurationandSyncWorkerFactoryinstance each timeworkManagerConfigurationis accessed. While WorkManager typically only accesses this once during initialization, it's cleaner to use a backing property or lazy initialization.Optional improvement
- override val workManagerConfiguration: Configuration - get() = - Configuration - .Builder() - .setWorkerFactory( - SyncWorkerFactory( - syncManager = syncManager, - daos = listOf(motionStoreDao), - ), - ).build() + override val workManagerConfiguration: Configuration by lazy { + Configuration + .Builder() + .setWorkerFactory( + SyncWorkerFactory( + syncManager = syncManager, + daos = listOf(motionStoreDao), + ), + ).build() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/LyricsApp.kt` around lines 40 - 49, The workManagerConfiguration getter currently constructs a new Configuration and SyncWorkerFactory on every access; change it to use a backing property or lazy-initialized val so the Configuration (and SyncWorkerFactory with syncManager and motionStoreDao) is created once and reused. Locate the workManagerConfiguration property and replace the get() builder with a single lazily initialized Configuration instance (or a private backing property) that constructs the Builder, sets the WorkerFactory(SyncWorkerFactory(...)), and returns the built Configuration.modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfoInitializer.kt (1)
22-26: Consider usingrequireContext()pattern instead of!!.While
contextis guaranteed to be non-null inonCreate(), using!!is less idiomatic. Consider using a null check with an early return or explicit error message for clarity.Optional improvement
override fun onCreate(): Boolean { - DeviceInfo.initialize(context!!) + val ctx = context ?: return false + DeviceInfo.initialize(ctx) return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfoInitializer.kt` around lines 22 - 26, DeviceInfoInitializer.onCreate currently force-unwraps context with context!! when calling DeviceInfo.initialize; change this to use a safe, idiomatic approach such as obtaining a non-null Context via requireContext() or performing an explicit null-check/early return and logging a clear error before returning false, then call DeviceInfo.initialize(requireContext()) (or DeviceInfo.initialize(ctx) after the null-check) so you avoid the !! operator and handle the nullability explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/lyrics-maker/build.gradle`:
- Around line 6-7: The build adds alias(libs.plugins.google.services) and
alias(libs.plugins.crashlytics) but never declares the Firebase Crashlytics SDK
or the required google-services.json; update the lyrics-maker module to add the
crashlytics runtime dependency (use the existing catalog symbol
libs.firebase.crashlytics in the lyrics-maker dependencies block) and place a
valid google-services.json file in the modules/lyrics-maker/ directory so the
google-services plugin can initialize properly.
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/LyricsApp.kt`:
- Around line 20-24: The eager initialization of syncManager is accessing
database (via DownloadedTrackerDao(database)) before the Application is
attached; change syncManager to a lazy-initialized property in LyricsApp so the
SyncManager and DownloadedTrackerDao(database) are created only after the
Application is ready (e.g. replace the current val syncManager = ... with a
lazily-initialized syncManager that constructs SyncManager(backend =
FirebaseAdapter(), downloadedTracker = DownloadedTrackerDao(database))),
ensuring DatabaseManager.init(this) is invoked when the app is attached.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/SyncableDao.kt`:
- Around line 61-67: The insertLocal implementation currently calls
insert(entity) without forcing the sync state; update insertLocal (in
SyncableDao.insertLocal) to set the entity's SyncTracker to the local-created
defaults (isDirty = true and serverId = null) before calling insert so it always
marks new rows as dirty for upload (use the entity's setter/constructor or
helper that applies a SyncTracker with isDirty=true, serverId=null, then call
insert).
---
Duplicate comments:
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.kt`:
- Around line 81-96: The current setDownloadedTill reads current via
getDownloadedTill then conditionally inserts, which creates a race; replace the
read-then-write with a single atomic upsert SQL using dbManager.getDb() so the
DB decides whether to insert or update without a separate read. Implement
setDownloadedTill to run one statement that inserts (COL_TABLE_NAME,
COL_DOWNLOADED_TILL) values and uses SQLite UPSERT/ON CONFLICT on COL_TABLE_NAME
to update COL_DOWNLOADED_TILL to the greater of existing and incoming (e.g. DO
UPDATE SET COL_DOWNLOADED_TILL = MAX(COL_DOWNLOADED_TILL,
excluded.COL_DOWNLOADED_TILL)) so updates are atomic and never regress under
concurrent syncs; use TABLE_NAME, COL_TABLE_NAME, COL_DOWNLOADED_TILL and
dbManager.getDb() to locate/replace the logic.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/MotionProjectDao.kt`:
- Around line 67-87: fromServerRow uses unsafe casts (as String / as Number)
that can throw at runtime; change to safe casts (as? String, as? Number) and
validate required fields before constructing MotionProject and SyncTracker
(check COL_NAME, COL_PATH, COL_CREATED, COL_UPDATED and
SyncTracker.COL_UPDATED_BY, COL_CREATED_ON, COL_UPDATED_ON exist and are the
expected types), convert Number toLong via safe calls (?.toLong()), and decide a
clear failure strategy (return null or throw an IllegalArgumentException with a
descriptive message and log) so malformed rows don’t crash the sync; update
fromServerRow and the SyncTracker construction to use these safe casts and
validations referencing the same symbols (fromServerRow, MotionProject,
SyncTracker, COL_NAME, COL_PATH, COL_CREATED, COL_UPDATED,
SyncTracker.COL_UPDATED_BY, SyncTracker.COL_CREATED_ON,
SyncTracker.COL_UPDATED_ON, SyncTracker.COL_SERVER_ID,
SyncTracker.COL_UPLOADED_AT).
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/SyncableDao.kt`:
- Around line 19-20: The KDoc for SyncableDao incorrectly references
syncTrackerFromCursor; update the documentation to reference the actual method
name readSyncTracker (and/or any other relevant API in SyncableDao/BaseDao) so
the doc matches the implementation, e.g., change the mention of
syncTrackerFromCursor to readSyncTracker and ensure the description matches how
readSyncTracker extracts the tracker from a Cursor row.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/BackendAdapter.kt`:
- Around line 12-15: The KDoc for BackendAdapter still references a non-existent
type parameter [T]; update the comment in BackendAdapter.kt to remove or reword
the "[T] is the DTO / map type..." sentence so it no longer mentions a generic
type, and if needed clarify that concrete adapters convert between their backend
DTO types and Map<String, Any?> internally (so SyncManager works with plain
maps), ensuring references to SyncManager remain accurate.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.kt`:
- Line 3: Remove the unused ContentValues import and update the KDoc for the
SyncableEntity identifier to match the actual type: change any references that
mention Long or -1 to describe the String-based ID used by the interface (refer
to SyncableEntity and its id property/method names in the file) and adjust
wording to reflect the correct default/empty value semantics for a String ID.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.kt`:
- Around line 59-60: The current filter uses gt(SyncTracker.COL_UPLOADED_AT,
since) which on initial sync (since == 0) will omit rows whose uploaded_at is 0
or NULL; update the predicate to include those legacy rows by replacing the
strict gt check with a compound condition that accepts NULL/zero values — e.g.
(isNull(SyncTracker.COL_UPLOADED_AT) OR gt(SyncTracker.COL_UPLOADED_AT, since))
or explicitly checking for uploaded_at <= 0 for the initial sync — while keeping
the neq(SyncTracker.COL_UPDATED_BY, deviceId) clause unchanged so legacy rows
are included for download on first sync.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.kt`:
- Around line 219-224: The catch blocks in SyncManager (the catch (e:
SyncException) and catch (e: Exception) branches that increment failed) silently
swallow errors; update both to log the exception and context before incrementing
failed—use the class logger (or existing logger instance) to emit a descriptive
message including the exception, the row/id being processed (where available in
the scope), and whether it was a SyncException or generic Exception; ensure you
call logger.error(...) (or logger.warn for recoverable cases) with the exception
object so stack traces are preserved and then increment failed as currently
done.
- Around line 148-159: The code path that treats server wins (condition
serverUpdatedOn >= localEntity.syncTracker.updatedOn) currently unconditionally
converts the server row via dao.fromServerRow and calls dao.update, which will
silently overwrite any local dirty changes; modify this branch to first check
localEntity.syncTracker.isDirty and handle accordingly (e.g., skip overwriting
and increment a conflict counter, attempt a merge, or enqueue a re-upload)
instead of blindly calling dao.update; use the existing dao.fromServerRow only
after resolving dirty-local semantics and ensure conflicts++/saved++ are
adjusted to reflect your chosen resolution.
- Around line 127-175: The loop that processes serverRows advances the download
cursor via downloadedTracker.setDownloadedTill(dao.tableName, highWaterMark)
even if dao.insert() or dao.update() fail, causing missed rows; modify
SyncManager's download logic (the serverRows processing loop and the post-loop
cursor advance) so the cursor is only advanced after all writes succeed — either
wrap the entire loop in a single transaction (use your DAO/DB transaction API or
a withTransaction helper) or collect/validate every row write and abort/rollback
on first exception before calling downloadedTracker.setDownloadedTill; ensure
exceptions from dao.fromServerRow, dao.insert, or dao.update cause a rollback
and prevent updating highWaterMark/downloadedTracker, leaving DownloadStats
unchanged in failure cases.
- Around line 76-85: The sync method in SyncManager launches all DAOs with
scope.async which can cause SQLiteDatabaseLockedException when concurrent writes
occur; either enable WAL mode in your DB setup (e.g., in DatabaseManager
initialization) so concurrent writes across tables are allowed, or change
SyncManager.sync to run syncTable(dao) sequentially (replace parallel
scope.async usage) and document the requirement; reference SyncManager.sync,
scope.async, syncTable(dao) for the async change and DatabaseManager for
enabling WAL.
- Around line 245-249: The cancel() method cancels the internal CoroutineScope
(scope) but does not recreate it, so subsequent calls to sync() or syncTable()
will throw JobCancellationException; fix by either recreating a fresh
CoroutineScope and Job after cancellation (e.g., reset scope =
CoroutineScope(SupervisorJob() + dispatcher) and any associated Job/state) or
document that SyncManager is single-use after cancel(); update cancel() to reset
_status to SyncStatus.Idle and reinitialize scope and any dependent fields
(referencing cancel(), scope, sync(), syncTable(), _status, and SyncManager) or
add clear documentation on unusability if you choose the single-use approach.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt`:
- Around line 3-18: MotionProject's default syncTracker uses DeviceInfo.id which
can throw IllegalStateException if DeviceInfo isn't initialized; change the
default in the MotionProject constructor to use an empty updatedBy (e.g.,
SyncTracker(updatedBy = "")) so creation won't depend on DeviceInfo, and let
SyncManager stamp the real updatedBy later, or alternately ensure tests
explicitly provide a syncTracker or call DeviceInfo.initialize() in setup;
update MotionProject (and any test fixtures constructing MotionProject without
syncTracker) to avoid referencing DeviceInfo.id at construction time.
---
Nitpick comments:
In
`@modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/LyricsApp.kt`:
- Around line 40-49: The workManagerConfiguration getter currently constructs a
new Configuration and SyncWorkerFactory on every access; change it to use a
backing property or lazy-initialized val so the Configuration (and
SyncWorkerFactory with syncManager and motionStoreDao) is created once and
reused. Locate the workManagerConfiguration property and replace the get()
builder with a single lazily initialized Configuration instance (or a private
backing property) that constructs the Builder, sets the
WorkerFactory(SyncWorkerFactory(...)), and returns the built Configuration.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.kt`:
- Around line 153-156: In BaseDao.kt update call replace the redundant
id.toString() with the String id directly: locate the db.update(...) invocation
that currently passes arrayOf(id.toString()) (in the method taking id: String,
entity: T) and change it to arrayOf(id) so you stop calling toString() on an
already-String identifier.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfoInitializer.kt`:
- Around line 22-26: DeviceInfoInitializer.onCreate currently force-unwraps
context with context!! when calling DeviceInfo.initialize; change this to use a
safe, idiomatic approach such as obtaining a non-null Context via
requireContext() or performing an explicit null-check/early return and logging a
clear error before returning false, then call
DeviceInfo.initialize(requireContext()) (or DeviceInfo.initialize(ctx) after the
null-check) so you avoid the !! operator and handle the nullability explicitly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
gradle/libs.versions.tomlmodules/lyrics-maker/build.gradlemodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/LyricsApp.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.ktmodules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.ktmodules/motion-store/build.gradlemodules/motion-store/src/main/AndroidManifest.xmlmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/MotionProjectDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/SyncableDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/BackendAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncException.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncResult.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DatabaseManager.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfo.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfoInitializer.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/FirebaseAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/RestAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/DownloadedTracker.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/SyncTracker.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/worker/SyncWorker.kt
🚧 Files skipped from review as they are similar to previous changes (7)
- modules/motion-store/build.gradle
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/DownloadedTracker.kt
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/FirebaseAdapter.kt
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/SyncTracker.kt
- gradle/libs.versions.toml
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncException.kt
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfo.kt
modules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/LyricsApp.kt
Outdated
Show resolved
Hide resolved
| /** | ||
| * Insert a new entity and mark it dirty. The returned ID is the SQLite rowid. | ||
| * The SyncTracker defaults (isDirty=true, serverId=null) represent a | ||
| * locally-created row not yet seen by the server. | ||
| */ | ||
| fun insertLocal(entity: T): Long = insert(entity) | ||
|
|
There was a problem hiding this comment.
insertLocal should enforce dirty state to match its contract.
The comment says it “mark[s] it dirty,” but Line [66] inserts as-is; a clean tracker here can prevent expected upload.
Possible implementation adjustment
fun insertLocal(entity: T): Long = insert(entity)
+ // safer variant:
+ // val tracker = entity.syncTracker.copy(isDirty = true, updatedOn = System.currentTimeMillis())
+ // `@Suppress`("UNCHECKED_CAST")
+ // return insert(entity.withSyncTracker(tracker) as T)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/SyncableDao.kt`
around lines 61 - 67, The insertLocal implementation currently calls
insert(entity) without forcing the sync state; update insertLocal (in
SyncableDao.insertLocal) to set the entity's SyncTracker to the local-created
defaults (isDirty = true and serverId = null) before calling insert so it always
marks new rows as dirty for upload (use the entity's setter/constructor or
helper that applies a SyncTracker with isDirty=true, serverId=null, then call
insert).
a0ee3d3 to
640a25d
Compare
| val created = docRef.get().await() | ||
| return created.data.orEmpty().toMutableMap().apply { | ||
| put(SyncTracker.COL_SERVER_ID, docRef.id) | ||
| put(SyncTracker.COL_UPLOADED_AT, created.getLong(SyncTracker.COL_UPLOADED_AT)) |
There was a problem hiding this comment.
CRITICAL: DocumentSnapshot.getLong() returns null for Firestore Timestamp fields
The uploaded_at field is written as FieldValue.serverTimestamp(), which Firestore stores as a Timestamp object — not a Long. Calling getLong(SyncTracker.COL_UPLOADED_AT) on a Timestamp field always returns null, so the returned map will have null for uploadedAt. This causes SyncManager to silently fall back to System.currentTimeMillis() (line 202), meaning the download cursor will never advance correctly from server-assigned timestamps.
To fix, read the Timestamp and convert it to epoch-ms:
put(SyncTracker.COL_UPLOADED_AT, created.getTimestamp(SyncTracker.COL_UPLOADED_AT)?.toDate()?.time)|
|
||
| return payload.apply { | ||
| put(SyncTracker.COL_SERVER_ID, serverId) | ||
| put(SyncTracker.COL_UPLOADED_AT, updatedDoc.getLong(SyncTracker.COL_UPLOADED_AT)) |
There was a problem hiding this comment.
CRITICAL: Same Timestamp → Long conversion bug as in create()
updatedDoc.getLong(SyncTracker.COL_UPLOADED_AT) returns null because uploaded_at is a Firestore Timestamp, not a Long. The markSynced() call in SyncManager will receive null and fall back to System.currentTimeMillis(), causing the local uploadedAt to diverge from the server value.
Fix:
put(SyncTracker.COL_UPLOADED_AT, updatedDoc.getTimestamp(SyncTracker.COL_UPLOADED_AT)?.toDate()?.time)| readTimeout = 15_000 | ||
| if (body != null) { | ||
| doOutput = true | ||
| OutputStreamWriter(outputStream).use { it.write(body.toString()) } |
There was a problem hiding this comment.
WARNING: OutputStreamWriter uses the platform default charset
OutputStreamWriter(outputStream) uses the JVM default charset, which may not be UTF-8 on all Android devices (though it is on modern ones). Since the Content-Type header declares application/json, the body must be UTF-8 encoded.
| OutputStreamWriter(outputStream).use { it.write(body.toString()) } | |
| OutputStreamWriter(outputStream, Charsets.UTF_8).use { it.write(body.toString()) } |
| val results = | ||
| daos | ||
| .map { dao -> | ||
| scope.async { syncTable(dao) } |
There was a problem hiding this comment.
WARNING: Concurrent table syncs overwrite each other's _status
Each syncTable() call immediately sets _status.value = SyncStatus.Running(dao.tableName) (line 91). When multiple tables run concurrently via scope.async, they race to update the shared _status flow, so observers will see only the last table's name and miss intermediate states. The Running status set on line 77 (before the async block) is immediately overwritten by the first coroutine to start.
Consider passing the table name only to the outer Running state and removing the inner _status.value = SyncStatus.Running(dao.tableName) from syncTable(), or use a set of currently-running table names.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
modules/lyrics-maker/build.gradle (1)
6-7:⚠️ Potential issue | 🟠 MajorCrashlytics setup looks incomplete; verify runtime SDK + config file.
With
alias(libs.plugins.crashlytics)(Line 7) and Firebase deps (Lines 69-70), this app module still needs Crashlytics runtime wiring (com.google.firebase:firebase-crashlytics) and a validgoogle-services.jsoninmodules/lyrics-maker/; otherwise crash reporting is not initialized (and build can fail if the JSON is missing).#!/bin/bash set -euo pipefail echo "== google-services.json presence ==" fd -t f '^google-services\.json$' modules/lyrics-maker . echo echo "== Crashlytics symbols in version catalog ==" rg -n -C3 'firebase-crashlytics|crashlytics|firebase = \[' gradle/libs.versions.toml echo echo "== lyrics-maker firebase/crashlytics wiring ==" rg -n -C3 'plugins|google\.services|crashlytics|firebase\.bom|bundles\.firebase' modules/lyrics-maker/build.gradleAlso applies to: 69-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/lyrics-maker/build.gradle` around lines 6 - 7, The Crashlytics plugin is declared via alias(libs.plugins.crashlytics) but the module is missing runtime wiring and the config file; add the Crashlytics runtime dependency (com.google.firebase:firebase-crashlytics) to the lyrics-maker module’s dependencies (match other firebase deps/bom usage) and ensure a valid google-services.json is present in the modules/lyrics-maker/ directory; also verify the version catalog (libs.versions.toml) contains an entry for firebase-crashlytics and that the module’s build.gradle applies the google-services plugin (alias(libs.plugins.google.services)) so Crashlytics initialization can run at build/runtime.
🧹 Nitpick comments (2)
modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.kt (2)
172-172: RedundanttoString()on already-Stringid.Same issue as in
update().🧹 Proposed fix
- fun deleteById(id: String): Int = db.delete(tableName, "$primaryKey = ?", arrayOf(id.toString())) + fun deleteById(id: String): Int = db.delete(tableName, "$primaryKey = ?", arrayOf(id))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.kt` at line 172, The deleteById method is calling toString() on id even though id is already a String; remove the redundant .toString() in deleteById (and mirror the same change you made for update()) so the db.delete call becomes db.delete(tableName, "$primaryKey = ?", arrayOf(id)) and uses the id String directly.
152-155: RedundanttoString()call on String parameter.The
idparameter is already aString, so callingid.toString()is unnecessary.🧹 Proposed fix
fun update( id: String, entity: T, - ): Int = db.update(tableName, toContentValues(entity), "$primaryKey = ?", arrayOf(id.toString())) + ): Int = db.update(tableName, toContentValues(entity), "$primaryKey = ?", arrayOf(id))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.kt` around lines 152 - 155, In the BaseDao.update method remove the redundant id.toString() call: when calling db.update(tableName, toContentValues(entity), "$primaryKey = ?", arrayOf(id.toString())), pass the id directly (arrayOf(id)) since id is already a String; update the call site in the update function accordingly (symbols: BaseDao.update, db.update, tableName, toContentValues, primaryKey).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.kt`:
- Around line 9-16: The KDoc contains an orphaned fragment; update the class
documentation for DownloadedTrackerDao to either remove the fragment or complete
it into a full sentence (for example: "This DAO extends BaseDao — it is
framework-internal and has no sync metadata of its own."). Edit the KDoc block
above the DownloadedTrackerDao declaration and ensure the reference to BaseDao
remains correct and the sentence reads coherently.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/FirebaseAdapter.kt`:
- Around line 61-65: The code reads a nullable Long via
created.getLong(SyncTracker.COL_UPLOADED_AT) and inserts it directly into the
map, which can yield null and later NPEs; update the FirebaseAdapter block that
builds the return map (the variables docRef, created and the key
SyncTracker.COL_UPLOADED_AT) to guard against null by using a safe fallback
(e.g., created.getLong(SyncTracker.COL_UPLOADED_AT) ?: 0L or another sensible
default) or by only putting the key when the value is non-null so downstream
code never receives a null Long.
---
Duplicate comments:
In `@modules/lyrics-maker/build.gradle`:
- Around line 6-7: The Crashlytics plugin is declared via
alias(libs.plugins.crashlytics) but the module is missing runtime wiring and the
config file; add the Crashlytics runtime dependency
(com.google.firebase:firebase-crashlytics) to the lyrics-maker module’s
dependencies (match other firebase deps/bom usage) and ensure a valid
google-services.json is present in the modules/lyrics-maker/ directory; also
verify the version catalog (libs.versions.toml) contains an entry for
firebase-crashlytics and that the module’s build.gradle applies the
google-services plugin (alias(libs.plugins.google.services)) so Crashlytics
initialization can run at build/runtime.
---
Nitpick comments:
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.kt`:
- Line 172: The deleteById method is calling toString() on id even though id is
already a String; remove the redundant .toString() in deleteById (and mirror the
same change you made for update()) so the db.delete call becomes
db.delete(tableName, "$primaryKey = ?", arrayOf(id)) and uses the id String
directly.
- Around line 152-155: In the BaseDao.update method remove the redundant
id.toString() call: when calling db.update(tableName, toContentValues(entity),
"$primaryKey = ?", arrayOf(id.toString())), pass the id directly (arrayOf(id))
since id is already a String; update the call site in the update function
accordingly (symbols: BaseDao.update, db.update, tableName, toContentValues,
primaryKey).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
gradle/libs.versions.tomlmodules/lyrics-maker/build.gradlemodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/LyricsApp.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/activity/SearchActivity.ktmodules/lyrics-maker/src/main/java/com/tejpratapsingh/lyricsmaker/presentation/worker/LyricsMotionWorker.ktmodules/metadata-extractor/src/main/java/com/tejpratapsingh/motion/metadataextractor/presentation/ShareReceiverActivity.ktmodules/motion-store/build.gradlemodules/motion-store/src/main/AndroidManifest.xmlmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/BaseDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/MotionProjectDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/SyncableDao.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/BackendAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncException.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncResult.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DatabaseManager.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfo.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfoInitializer.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/FirebaseAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/RestAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SupabaseAdapter.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/SyncManager.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/DownloadedTracker.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/SyncTracker.ktmodules/motion-store/src/main/java/com/tejpratapsingh/motionstore/worker/SyncWorker.kt
🚧 Files skipped from review as they are similar to previous changes (7)
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncableEntity.kt
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/MotionProject.kt
- modules/motion-store/src/main/AndroidManifest.xml
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/domain/SyncException.kt
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/tables/DownloadedTracker.kt
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfo.kt
- modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/DeviceInfoInitializer.kt
| /** | ||
| * Tracks the high-water mark of downloaded server data per table. | ||
| * | ||
| * One row per table. The [downloadedTill] value is the highest [SyncTracker.uploadedAt] | ||
| * seen from the server so far. On the next sync cycle, we ask the server for all rows | ||
| * where uploadedAt > downloadedTill, giving us a clean incremental download cursor. | ||
| * extending [BaseDao] — it is framework-internal and has no sync metadata of its own. | ||
| */ |
There was a problem hiding this comment.
Incomplete KDoc edit leaves orphaned sentence fragment.
Line 15 contains "extending [BaseDao] — it is framework-internal and has no sync metadata of its own." which appears to be a leftover from a previous edit attempt. This sentence fragment is missing its subject.
📝 Proposed fix
/**
* Tracks the high-water mark of downloaded server data per table.
*
* One row per table. The [downloadedTill] value is the highest [SyncTracker.uploadedAt]
* seen from the server so far. On the next sync cycle, we ask the server for all rows
* where uploadedAt > downloadedTill, giving us a clean incremental download cursor.
- * extending [BaseDao] — it is framework-internal and has no sync metadata of its own.
+ *
+ * This DAO extends [BaseDao] but is framework-internal and has no sync metadata of its own.
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Tracks the high-water mark of downloaded server data per table. | |
| * | |
| * One row per table. The [downloadedTill] value is the highest [SyncTracker.uploadedAt] | |
| * seen from the server so far. On the next sync cycle, we ask the server for all rows | |
| * where uploadedAt > downloadedTill, giving us a clean incremental download cursor. | |
| * extending [BaseDao] — it is framework-internal and has no sync metadata of its own. | |
| */ | |
| /** | |
| * Tracks the high-water mark of downloaded server data per table. | |
| * | |
| * One row per table. The [downloadedTill] value is the highest [SyncTracker.uploadedAt] | |
| * seen from the server so far. On the next sync cycle, we ask the server for all rows | |
| * where uploadedAt > downloadedTill, giving us a clean incremental download cursor. | |
| * | |
| * This DAO extends [BaseDao] but is framework-internal and has no sync metadata of its own. | |
| */ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/dao/DownloadedTrackerDao.kt`
around lines 9 - 16, The KDoc contains an orphaned fragment; update the class
documentation for DownloadedTrackerDao to either remove the fragment or complete
it into a full sentence (for example: "This DAO extends BaseDao — it is
framework-internal and has no sync metadata of its own."). Edit the KDoc block
above the DownloadedTrackerDao declaration and ensure the reference to BaseDao
remains correct and the sentence reads coherently.
| val created = docRef.get().await() | ||
| return created.data.orEmpty().toMutableMap().apply { | ||
| put(SyncTracker.COL_SERVER_ID, docRef.id) | ||
| put(SyncTracker.COL_UPLOADED_AT, created.getLong(SyncTracker.COL_UPLOADED_AT)) | ||
| } |
There was a problem hiding this comment.
getLong() returns null if the field doesn't exist or isn't a number, causing NPE.
Line 64 calls created.getLong(...) which can return null. Putting null into the map might cause issues downstream when the caller expects a non-null Long.
🛡️ Proposed fix
return created.data.orEmpty().toMutableMap().apply {
put(SyncTracker.COL_SERVER_ID, docRef.id)
- put(SyncTracker.COL_UPLOADED_AT, created.getLong(SyncTracker.COL_UPLOADED_AT))
+ put(SyncTracker.COL_UPLOADED_AT, created.getLong(SyncTracker.COL_UPLOADED_AT) ?: System.currentTimeMillis())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val created = docRef.get().await() | |
| return created.data.orEmpty().toMutableMap().apply { | |
| put(SyncTracker.COL_SERVER_ID, docRef.id) | |
| put(SyncTracker.COL_UPLOADED_AT, created.getLong(SyncTracker.COL_UPLOADED_AT)) | |
| } | |
| val created = docRef.get().await() | |
| return created.data.orEmpty().toMutableMap().apply { | |
| put(SyncTracker.COL_SERVER_ID, docRef.id) | |
| put(SyncTracker.COL_UPLOADED_AT, created.getLong(SyncTracker.COL_UPLOADED_AT) ?: System.currentTimeMillis()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@modules/motion-store/src/main/java/com/tejpratapsingh/motionstore/infra/FirebaseAdapter.kt`
around lines 61 - 65, The code reads a nullable Long via
created.getLong(SyncTracker.COL_UPLOADED_AT) and inserts it directly into the
map, which can yield null and later NPEs; update the FirebaseAdapter block that
builds the return map (the variables docRef, created and the key
SyncTracker.COL_UPLOADED_AT) to guard against null by using a safe fallback
(e.g., created.getLong(SyncTracker.COL_UPLOADED_AT) ?: 0L or another sensible
default) or by only putting the key when the value is non-null so downstream
code never receives a null Long.
640a25d to
a268876
Compare
| supabase-realtime-kt = { module = "io.github.jan-tennert.supabase:realtime-kt" } | ||
|
|
||
| firebase-bom = { module = "com.google.firebase:firebase-bom", version.ref = "firebaseBomVersion" } | ||
| firebase-analytics = { module = "com.google.firebase:firebase-analytics" } |
There was a problem hiding this comment.
WARNING: Duplicate firebase-analytics library alias
firebase-analytics (line 96) and google-firebase-analytics (line 53) both resolve to the same artifact com.google.firebase:firebase-analytics. The firebase bundle references google-firebase-analytics, so firebase-analytics is unused and will cause confusion. Remove the duplicate:
| firebase-analytics = { module = "com.google.firebase:firebase-analytics" } |
Delete line 96 (firebase-analytics = { module = "com.google.firebase:firebase-analytics" }) to avoid the redundant alias.
| val results = | ||
| daos | ||
| .map { dao -> | ||
| scope.async { syncTable(dao) } |
There was a problem hiding this comment.
WARNING: _status race condition in concurrent sync
Each scope.async { syncTable(dao) } coroutine calls _status.value = SyncStatus.Running(dao.tableName) independently. When multiple tables sync concurrently, these writes race and the status will reflect whichever table wrote last — not a meaningful aggregate state. Consider updating _status only from the sync() caller (already done at line 77) and removing the redundant _status.value = SyncStatus.Running(dao.tableName) assignment inside syncTable(), or use a thread-safe counter to track in-progress tables.
tejpratap46
force-pushed
the
feat/offline-first-sync
branch
from
a268876 to
be8f8b0
Compare
| val created = docRef.get().await() | ||
| return created.data.orEmpty().toMutableMap().apply { | ||
| put(SyncTracker.COL_SERVER_ID, docRef.id) | ||
| put(SyncTracker.COL_UPLOADED_AT, created.getLong(SyncTracker.COL_UPLOADED_AT)) |
There was a problem hiding this comment.
CRITICAL: getLong() returns null for Firestore Timestamp fields
FieldValue.serverTimestamp() stores the value as a Firestore Timestamp object, not a numeric Long. DocumentSnapshot.getLong() only works for numeric fields and returns null for Timestamp fields. This means uploadedAt will always be null in the returned map, causing SyncManager to silently fall back to System.currentTimeMillis() (a client-side timestamp), which defeats the purpose of using server timestamps and breaks sync ordering across devices.
Use getTimestamp() and convert to epoch milliseconds:
| put(SyncTracker.COL_UPLOADED_AT, created.getLong(SyncTracker.COL_UPLOADED_AT)) | |
| put(SyncTracker.COL_UPLOADED_AT, created.getTimestamp(SyncTracker.COL_UPLOADED_AT)?.toDate()?.time) |
|
|
||
| return payload.apply { | ||
| put(SyncTracker.COL_SERVER_ID, serverId) | ||
| put(SyncTracker.COL_UPLOADED_AT, updatedDoc.getLong(SyncTracker.COL_UPLOADED_AT)) |
There was a problem hiding this comment.
CRITICAL: Same getLong() / Timestamp mismatch as in create()
updatedDoc.getLong(SyncTracker.COL_UPLOADED_AT) returns null because FieldValue.serverTimestamp() stores a Firestore Timestamp, not a Long. The uploadedAt in the returned map will always be null, causing SyncManager to fall back to a client-side timestamp.
Use getTimestamp() instead:
| put(SyncTracker.COL_UPLOADED_AT, updatedDoc.getLong(SyncTracker.COL_UPLOADED_AT)) | |
| put(SyncTracker.COL_UPLOADED_AT, updatedDoc.getTimestamp(SyncTracker.COL_UPLOADED_AT)?.toDate()?.time) |
| readTimeout = 15_000 | ||
| if (body != null) { | ||
| doOutput = true | ||
| OutputStreamWriter(outputStream).use { it.write(body.toString()) } |
There was a problem hiding this comment.
WARNING: OutputStreamWriter created without an explicit charset
Although Android defaults to UTF-8, omitting the charset is fragile and can cause encoding issues on non-standard configurations. Always specify the charset explicitly:
| OutputStreamWriter(outputStream).use { it.write(body.toString()) } | |
| OutputStreamWriter(outputStream, Charsets.UTF_8).use { it.write(body.toString()) } |
| supabase-realtime-kt = { module = "io.github.jan-tennert.supabase:realtime-kt" } | ||
|
|
||
| firebase-bom = { module = "com.google.firebase:firebase-bom", version.ref = "firebaseBomVersion" } | ||
| firebase-analytics = { module = "com.google.firebase:firebase-analytics" } |
There was a problem hiding this comment.
WARNING: Duplicate Firebase Analytics library definition
firebase-analytics (line 96) maps to the same Maven artifact com.google.firebase:firebase-analytics as the pre-existing google-firebase-analytics (line 53). The firebase bundle at line 173 references google-firebase-analytics, so firebase-analytics is unused. Having two aliases for the same artifact causes confusion and may lead to duplicate dependency declarations. Remove one of the two definitions and update the bundle reference to use a single consistent alias.
tejpratap46
force-pushed
the
feat/offline-first-sync
branch
from
be8f8b0 to
35b2e4b
Compare
1 participant
Summary by CodeRabbit
New Features
Chores