debug(mobile): no-issue: add sync debug logs and disable server overrides#10140
debug(mobile): no-issue: add sync debug logs and disable server overrides#10140jaskfla wants to merge 3 commits into
Conversation
…ides Add in-app sync debug logging to help diagnose initial sync failures, and temporarily disable server overrides so dev builds can use the normal server selector. Co-authored-by: Cursor <cursoragent@cursor.com>
jaskfla
requested review from
chris-bes,
dannash100,
edmofro,
passcod and
rohan-bes
as code owners
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d3dec22. Configure here.
| const defaultMetaServer = 'https://meta.tamanu.app'; | ||
| const metaServer = metaServerOverride || defaultMetaServer; | ||
| const metaServer = defaultMetaServer; | ||
| // const metaServer = metaServerOverride || defaultMetaServer; |
There was a problem hiding this comment.
Build server overrides ignored
High Severity
The PR disables reading serverOverrides.json for every build, not only dev. App Center still writes that file from SERVER_OVERRIDES, but fetchServers always uses the default meta server and public server list, so pinned central servers and custom meta URLs no longer apply.
Reviewed by Cursor Bugbot for commit d3dec22. Configure here.
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
|
🦸 Review Hero Summary Below consensus threshold (4 unique issues not confirmed by majority)
Nitpicks
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed.
|
| tableName, | ||
| recordId: row.id, | ||
| columns: Object.keys(row), | ||
| data: row, |
There was a problem hiding this comment.
[Bugs & Correctness] critical
Logging data: row on insert failure writes the full row payload (which can include patient clinical data) to both the in-memory SyncDebugLog and directly to console.log (via SyncDebugLog.log). In React Native, console.log output is visible via ADB logcat and system log viewers. The project rules explicitly state patient data must never be logged at INFO level or above. Remove data: row from the log entry; the existing recordId, columns (just column names), and error.message are sufficient for diagnosis without exposing record content.
| console.log(`[SyncDebug] ${message}`, data ?? ''); | ||
| } | ||
|
|
||
| getLogs(): SyncDebugLogEntry[] { |
There was a problem hiding this comment.
[Design & Architecture] suggestion
getLogs() is defined but never called anywhere in this PR. SyncErrorDisplay calls formatLogs() directly. Either remove getLogs() (formatting is a display concern and belongs in the component anyway) or invert it: have the component call getLogs() and format the entries itself, and drop formatLogs(). Right now both methods exist with overlapping purposes, and one is dead code.
| @@ -1,5 +1,6 @@ | |||
| export * from './MobileSyncManager'; | |||
| export * from './CentralServerConnection'; | |||
| export * from './SyncDebugLog'; | |||
There was a problem hiding this comment.
[Design & Architecture] suggestion
SyncDebugLog is a debug utility that shouldn't be part of the sync service's public API. Exporting it from index.ts means any consumer of the sync module can depend on it, turning an internal debugging aid into a stable interface. If SyncErrorDisplay needs it, import directly from the source file ('~/services/sync/SyncDebugLog') rather than promoting it to public API.
| import { StyledText, StyledView } from '../../styled/common'; | ||
| import { theme } from '../../styled/theme'; | ||
| import * as overrides from '/root/serverOverrides.json'; | ||
| // TEMPORARY: server overrides commented out to allow normal server select in dev builds |
There was a problem hiding this comment.
[Design & Architecture] suggestion
The server overrides import and usage are commented out with "TEMPORARY" markers rather than removed or placed behind a proper flag. Commented-out code is opaque about intent — it's unclear when or whether it will be re-enabled, and the import * as overrides from '/root/serverOverrides.json' line (even commented) signals a path that only exists on specific devices. If this is being disabled for the release, delete the dead code; if it needs to return, use a build-time flag or a config value instead of re-introducing file-system-path imports.
| if (this.logs.length > MAX_LOGS) { | ||
| this.logs.shift(); | ||
| } | ||
| console.log(`[SyncDebug] ${message}`, data ?? ''); |
There was a problem hiding this comment.
[Performance] suggestion
Every call to SyncDebugLog.log() unconditionally invokes console.log. This method is called in hot sync paths — saveChangesForModel fires once per model per batch during both initial and incremental sync. In React Native, console.log is synchronous and non-trivial (it serialises arguments and bridges to the JS thread). On an initial sync with many models and batches, this compounds. Consider gating on a debug flag (__DEV__ or an explicit env var) so it does not run in production/release builds.
| await repository.insert(row); | ||
| } catch (error: any) { | ||
| throw new Error(`Insert failed with '${error.message}', recordId: ${row.id}`); | ||
| SyncDebugLog.log(`Insert failed for ${tableName}`, { |
There was a problem hiding this comment.
[Performance] suggestion
The insert-failure log stores the entire row object (data: row) in the in-memory SyncDebugLog buffer. Healthcare sync records can be large (nested JSON fields, blobs, etc.). If a batch fails completely and triggers row-by-row fallback, up to chunkSize log entries are queued — each holding a full record object. Combined with the MAX_LOGS=100 cap using Array.shift() (O(n) on every overflow), this can hold large live objects in memory that the GC cannot collect for the lifetime of the sync session. Consider logging only recordId and error rather than the complete row for the per-row failure case.
|
🦸 Review Hero Summary Below consensus threshold (6 unique issues not confirmed by majority)
Nitpicks
Local fix prompt (copy to your coding agent)Fix these issues identified on the pull request. One commit per issue fixed.
|
|
Android builds 📱
|
|
You know it seems not a bad idea to have some version of this service persist permanently, but instead of displaying the logs through the app it sends them up to central (or canopy?) any time it hits a sync error. Acknowledging scope creep here, just consider it when you are secondary next week. |
Cherry-pick fix from #10008 into the debug branch. When batch insert fails, retry per-row with raw SQL instead of repository.insert so @RelationId-backed FK columns are preserved. Keeps SyncDebugLog logging from the debug work. Co-authored-by: Cursor <cursoragent@cursor.com>
🍹
|
Mock SyncDebugLog and add getTableName to the test model stub so debug logging in saveChangesForModel does not break existing unit tests. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Not saying it's impossible, but everything that talks to canopy has a trust relationship with it, and that's just not the case with mobile. So it implies adding on a third separate authentication mode to canopy; going via central is probably easier at this point. |
3 participants
Changes
Temporary debug PR for investigating initial sync failures on mobile.
SyncDebugLogservice that collects sync lifecycle and insert-failure details during sync runsserverOverrides.jsonusage so dev builds use the normal meta server list_Auto-Deploy
Options
Tests
Review Hero
Remember to...