Om/offline improvement

[Omprakash-48]

No description provided.

[Omprakash-48]

PR #36 Round-4 Review — Reply

Every one of the 28 findings was verified against om/offline_improvement (head d9d01d8) and is answered point-by-point below.

Disposition tally (28):

• 17 fixed — B1, B2, B3, B5, B6, H2, H3, H5, H8, M1, M6, M7, M8, D1, D2, D4, D5 (committed in 4a52249).
• 1 real, fix rejected — B4 (the proposed LIKE-escape would break Link-picker search).
• 1 real, accepted by design — H6 (migration kept atomic on purpose).
• 9 false positives — H1, H4, H7, H9, M2, M3, M4, M5, D3 (no change; evidence below).

---

✅ Fixed (17) — committed in 4a52249, full suite 1311 green, analyze + format clean

Blockers

ID	File	What was done
B1	push_engine.dart	runOnce() now guarded by _running + _rerunRequested (body moved to _drainOnce()). A bare _running flag would drop rows enqueued mid-drain, so the rerun flag re-drains once after a concurrent caller arrives. Test: push_engine_test "concurrent runOnce".
B2	write_queue.dart	Per-task outcomes are buffered in-txn; completers resolve after db.transaction(...) commits. On outer-txn failure, queued callers are failed — no pre-durable success signal. Test: write_queue_test "caller does not observe success when outer txn fails".
B3	cursor.dart	toJson() is now if (isNull && !complete) return null, so a completed-but-empty cursor serializes {complete:true}; pull_engine.dart persists it and the zero-row doctype graduates to incremental. Test: cursor_test "completed empty cursor serializes".
B5	form_screen.dart, offline_repository.dart	Added hardDeleteLocalMirror (best-effort, child cascade) called after the successful online API delete in both the form_screen online branch and the repo's !offlineMode.enabled branch, so the local docs__ row isn't orphaned. Provenance note: no hardDeleteDocument was ever removed from form_screen, and repository.hardDeleteDocument does not exist — the new method is hardDeleteLocalMirror. Tests: offline_repository_delete_cascade_test.
B6	base_field.dart	FieldStyle (the class is FieldStyle, not BaseFieldStyle) showLabel/showDescription reverted to default true; the internal FrappeFormBuilder passes explicit false where it intends to hide them. Note: a null style already rendered labels via the != false guard, so the "silent breaking change for all consumers" framing only applied to an explicitly-constructed style. Test: field_style_test.

High

ID	File	What was done
H2	response_writeback.dart:152-167	The positional child-row fallback now assigns its rows-affected to updated, and a 0 double-miss emits a debugPrint naming the orphaned child instead of silently dropping its server_name writeback. We initially read this as benign; that was wrong — the fallback exists because mobile_uuid isn't always echoed and Frappe renumbers idx 0→1 (hence 0-based pos matching, comment 128-135), so a local row can miss both keys and re-push next sync. Low severity, but a real silent gap. Test: response_writeback_test "double-miss".
H3	attachment_pipeline.dart, pending_attachment_dao.dart	Upload result (file_url/name) is persisted via recordUpload before markDone; on retry, if a result is already recorded the upload is skipped → no duplicate File doctype record. (The reviewer's "separate try/catch" alone was insufficient — markFailed leaves the row pending and the next drain re-uploads; idempotency is the actual fix.) Test: attachment_pipeline_test "failed done-state write does not re-upload".
H5	frappe_sdk.dart	_initialMetaAndDataSync() now coalesces on a _metaSyncInFlight completer inside the method itself (body → _runInitialMetaAndDataSync()), so login() / verifyLoginOtp() / initialize() can't run concurrent meta syncs regardless of entry point.
H8	offline_repository.dart	The child-cascade loop and the outer parent delete now rethrow real errors and swallow only no such table / no such column (_isBenignSchemaAbsence). A genuine failure rolls back the whole transaction — no partial delete reported as success. Tests: offline_repository_delete_cascade_test (2).

Medium / hygiene

ID	File	What was done
M1	connectivity_watcher.dart	onError handler added to the stream subscription (logs + swallows plugin stream errors, e.g. Android-13 permission revocation). Test: connectivity_watcher_test.
M6	frappe_sdk.dart	Bare print(...) → debugPrint(...) on the production paths.
M7	frappe_mobile_sdk.dart	RunFn dropped from the public barrel show clause.
M8	pubspec.yaml	meta: any → meta: ">=1.9.0 <3.0.0".

Docs (release-2.0)

ID	File	What was done
D1	breaking-changes / whats-new / migrating-from-1.x	sdk.unifiedResolver → sdk.resolver (the real getter). Note: there were 5 occurrences, not 4 — migrating-from-1.x.md had one the review missed.
D2	whats-new.md	offlineRepository.createDocument(...) → saveDocument(...).
D4	OFFLINE_FIRST.md	Removed the MigrationBlockedScreen section — the class doesn't exist in lib/ and isn't exported.
D5	OFFLINE_FIRST.md	Rewrote the obsolete two-store narrative to the single-store (docs__<doctype>) design; the legacy documents table was dropped in the v2→v3 migration (app_database.dart:151).

---

⚠️ B4 — real fact, but do NOT apply the proposed fix

filter_parser.dart:188 passes the LIKE value unescaped — but the %/_ wildcards are caller-supplied, not user input. The one construction site wraps the query itself: link_option_service.dart:347 builds '%$query%'. The proposed "escape %/_ + ESCAPE '\'" would escape those wrapper % and break every Link-picker search. Frappe's server-side LIKE doesn't escape these either, so behavior already matches the server. No change. (If literal _/% typed inside a search term ever causes loose matches, we'd escape only the inner term while preserving the wrapper — speculative, and not what was proposed.)

---

⏸️ H6 — real, accepted by design (no code change)

meta_migration.dart — the __norm backfill is already row-chunked (chunkSize = 500), but the whole migration (ALTERs + backfill) is intentionally one atomic transaction, which is why the write lock is held for its duration. Two tests pin that rollback guarantee (meta_migration_test.dart "partial __norm updates do not persist" and "wrapped in a transaction"). The reviewer's literal "batch in chunks of 200-500" is already implemented and does not by itself release the lock; only per-chunk commits would, at the cost of all-or-nothing atomicity. Given typical doctype row counts on the boot/migration path, we keep it atomic. (Option to split — schema ALTERs atomic, idempotent __norm backfill in separate batched commits afterward — is documented in the action plan if row counts ever grow.)

---

❌ False positives — no change (with evidence)

ID	Claim	Why it doesn't hold
H1	_eq(null,null) returns false → null fields overwrite server	three_way_merge.dart:71 runs if (identical(a,b)) return true; before the null-bail (72). identical(null,null) == true, so _eq(null,null) is already true.
H4	syncFromApi() removed no-throw contract → callers crash on result==null	permission_service.dart:45 propagates by documented design (the timeout/maxRetries:0 boot fast-fail). The sole internal caller frappe_sdk.dart try/catches it and ignores the return value — no result==null caller exists. (If external consumers relied on the v1 null-return, that's a public-API note worth a changelog line, but the described crash does not occur.)
H7	resolveConflict deletes outbox before applying snapshot	Wrong file and reversed: it lives in sync_controller.dart:205 (not offline_repository), and applies the snapshot (applySingleDoc, 242) before markDone (243). Already correct.
H9	nullable columns hard-cast → crash on NULL	outbox_row.dart:114-137 uses bare as String/as int only on the NOT-NULL columns (id, doctype, mobile_uuid, operation, state, created_at); every nullable column uses as String?/as int?. No cast can throw on a schema-valid row.
M2	_drain() in finally masks the original error	concurrency_pool.dart:52 delivers the error via completeError in the catch, before the finally/_drain (55). The error is already on the completer; _drain can't mask it.
M3	dataForDepends evaluates a stale snapshot	form_builder.dart:1287-1289 evaluates visibility against the live formValues (state.value + _formData); dataForDepends is derived from that live data, not a stale snapshot.
M4	_forceExited set after c.complete()	offline_transition_service.dart:185 sets _forceExited = true before c.complete() (187). The finding describes the reverse.
M5	stream emits before DB write returns	session_user_service.dart:35-40 await _db.update(...) completes before _controller.add(...). Emit is correctly after persistence.
D3	cancelInitialSync() documented but not implemented	It exists at sync_controller.dart:150. The doc is correct.

[deepak-dhwani]

Minor Observation (non-blocking)

sync_controller.dart (pre-existing) and offline_repository.hardDeleteLocalMirror both use print() + // ignore: avoid_print in best-effort catch blocks.
The pattern is intentional (visibility in release builds) but worth a project-wide sweep — a thin debugPrint-based wrapper that strips in release is cleaner
than accumulating lint suppressors.

---

Approved. Every round-4 finding is resolved, correctly rejected, or confirmed as a false positive, all verified in code at 4a52249. Ready to merge.