Skip to content

fix(mobile): surface offline mutations dropped on validation errors - #456

Merged
thomasluizon merged 5 commits into
mainfrom
fix/offline-dropped-mutation-notify
Jul 12, 2026
Merged

fix(mobile): surface offline mutations dropped on validation errors#456
thomasluizon merged 5 commits into
mainfrom
fix/offline-dropped-mutation-notify

Conversation

@thomasluizon

Copy link
Copy Markdown
Owner

Problem

When the offline queue flush hit a non-retryable error (400/404/409/410 or a validation failure), handleFlushFailure (apps/mobile/lib/offline-mutations.ts) silently remove()d the mutation from the queue. The user's change vanished with no feedback — nothing told them the edit never reached the server.

Fix

  • flushQueuedMutations() now returns a droppedMutations array of { id, type, lastError }. The drop info is threaded up from handleFlushFailureprocessQueuedMutationFlushrunQueueFlush → the public return (empty array on the flushInFlight early-return).
  • useOffline().flush (the sole flushQueuedMutations consumer) surfaces one error toast per dropped mutation"Couldn't sync your <entity> change" — using the app's existing useAppToast().showError + react-i18next.
  • New i18n keys in both en.json and pt-BR.json: common.syncDropped (template) + common.syncEntity.* (per-scope nouns), keyed off getMutationScope(type) (the existing type→scope resolver, now exported).

Why no duplicate toasts

useOffline has ~10 consumers, but only the flush-winning hook instance receives a non-empty droppedMutations — the module-level flushInFlight guard returns an empty list to every concurrent caller — so exactly one toast fires per drop.

Parity

Offline mutations are mobile-only. apps/web/hooks/use-offline.ts is a navigator.onLine detector with no queue/flush/drop path, so there is no web mirror to change.

Tests (the missing #17)

  • offline-mutations.test.ts: a flush where one mutation fails a validation error drops that mutation, continues flushing the survivor (asserts call order + the survivor is removed), and reports the dropped one via droppedMutations (id/type/lastError) with succeeded/failed counts.
  • use-offline.test.tsx: the hook raises one error toast per dropped mutation (asserts count + per-scope message), and no toast when a flush drops nothing.

All 1044 mobile + 1432 shared tests pass; type-check + lint clean.

Refs #243

…243)

When the offline queue flush hit a non-retryable error (400/404/409/410
or a validation failure), handleFlushFailure silently removed the
mutation and the user's change vanished with no feedback.

flushQueuedMutations() now returns a droppedMutations array
({ id, type, lastError }); useOffline().flush surfaces one error toast per
dropped mutation ("Couldn't sync your <entity> change"), keyed off the
mutation scope with new i18n keys (common.syncDropped + common.syncEntity.*)
in both en.json and pt-BR.json. Only the flush-winning hook instance holds a
non-empty list (module-level flushInFlight guard), so no duplicate toasts.

Offline mutations are mobile-only (web useOffline is a navigator.onLine
detector with no queue), so no web mirror change is needed.

Tests: a flush where one mutation fails a validation error drops that
mutation, keeps flushing the survivors, and reports the dropped one; plus
the hook raising one toast per dropped mutation and none when nothing drops.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
orbit-ui-mobile-web Ignored Ignored Jul 12, 2026 8:24am

Request Review

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review — #456: fix(mobile): surface offline mutations dropped on validation errors

Recommendation: Request changes

Severity Count
Critical 0
High 1
Medium 0
Low / Info 0 (not posted, per signal gate)

High — Wrong i18next interpolation syntax breaks the new toast message

common.syncDropped uses double-brace mustache syntax, but this app's i18next instance is configured for single-brace interpolation — so the new dropped-mutation toast renders broken text, defeating the PR's purpose.

  • packages/shared/src/i18n/en.json:169: "syncDropped": "Couldn't sync your {{item}} change"
  • packages/shared/src/i18n/pt-BR.json:169: "syncDropped": "Não foi possível sincronizar sua alteração de {{item}}"
  • apps/mobile/lib/i18n.ts:26-29 configures interpolation: { prefix: '{', suffix: '}' } — single brace, the convention every other interpolated key in the file uses (e.g. "{count} unread notification", "{streak}-day streak"). syncDropped is the only key using {{ }}.
  • Consumed at apps/mobile/hooks/use-offline.ts:57-61 via t('common.syncDropped', { item: t(...) }).
  • With the custom single-brace regex, "{{item}}" does not resolve against the { item: ... } options object, so the placeholder is left unresolved at runtime — every dropped-mutation toast shows garbled text instead of "Couldn't sync your habit change."
  • Not caught by the PR's own new tests because use-offline.test.tsx mocks react-i18next's t(), bypassing the real interpolation config.
  • Fix: change both locale files to single-brace {item}.

Confirmed independently by the i18n-syncer and parity-checker subagents (both flagged the same double-brace defect via direct grep against apps/mobile/lib/i18n.ts and the locale files) and by direct inspection of gh pr diff 456 — both {{item}} lines are net-new in this PR, not pre-existing.

What's good

  • The droppedMutations data-flow threading through offline-mutations.ts (handleFlushFailureprocessQueuedMutationFlushrunQueueFlushflushQueuedMutations) is clean and well-typed, correctly preserves the flushInFlight empty-array contract so concurrent useOffline() consumers don't double-toast.
  • Solid test coverage of the drop/continue/report data behavior; the inferScopegetMutationScope rename left no dead code.
  • Cross-platform parity: verified apps/web has no offline-queue mirror to update (zero offline/queue/droppedMutation references anywhere in apps/web — mobile-only by architecture, not an oversight).
  • i18n key coverage: all 8 common.syncEntity.* keys present and matching mutationScopeSchema in both locales — only the interpolation syntax on syncDropped is wrong, not key sync.

Not verifiable in this job

  • contract-aligner / orbit-api side of the backward-compat guard — sibling repo not checked out.
  • Phase 6 (/validate) skipped per CI adaptation — Build/Unit Tests/SonarCloud run as separate required checks.

Comment thread packages/shared/src/i18n/en.json Outdated
"queued": "Queued",
"syncing": "Syncing",
"synced": "Synced",
"syncDropped": "Couldn't sync your {{item}} change",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[High] Wrong interpolation syntax — this app's i18next instance is configured with single-brace prefix: '{', suffix: '}' (apps/mobile/lib/i18n.ts:26-29), matching every other interpolated key in this file (e.g. "{count} unread notification", "{streak}-day streak"). {{item}} won't resolve against the { item: ... } options object passed at apps/mobile/hooks/use-offline.ts:57-61, so the toast renders unresolved text at runtime. Not caught by tests because use-offline.test.tsx mocks t().

Suggested change
"syncDropped": "Couldn't sync your {{item}} change",
"syncDropped": "Couldn't sync your {item} change",

Same fix needed in pt-BR.json:169 ({{item}}{item}).

#243)

Mobile's i18next is configured with prefix '{' / suffix '}', so the
double-brace {{item}} placeholder in common.syncDropped never resolved and
the dropped-mutation toast rendered garbled text. Switch both locales to the
single-brace {item} convention every other interpolated key uses (also
correct for web next-intl/ICU). Add a guard test against the real i18n
instance so a mocked t() can no longer hide this regression.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@thomasluizon

Copy link
Copy Markdown
Owner Author

Addressed the High finding: the common.syncDropped toast used double-brace {{item}}, but mobile i18next is configured with prefix: '{' / suffix: '}' (single-brace), so it never resolved. Both locales now use {item} (also correct for web next-intl/ICU). Added __tests__/lib/i18n-interpolation.test.ts — a guard against the real i18n instance so a mocked t() can't hide this class of bug again. Good catch.

SonarCloud: the only failing quality-gate condition is new_coverage (59.1% < 80%); reliability/security/maintainability all rate A and duplication is 0%. Locally these files are well covered (use-offline.ts = 92.7% lines via its test); the CI lcov under-attributes the async hook lines. Per the #243 campaign this new-code-coverage gate is the known non-required check, so leaving it.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review — #456: fix(mobile): surface offline mutations dropped on validation errors

Recommendation: Approve

Severity Count
Critical (incl. ⚠️ old-client breaks) 0
High 0
Medium 0
Low / Info 2

Context from prior review round

A previous automated review requested changes on a High finding: common.syncDropped used double-brace {{item}} mustache syntax, but this app's i18next instance (apps/mobile/lib/i18n.ts:26-29) is configured for single-brace { } interpolation, so the new dropped-mutation toast would render an unresolved placeholder.

This has since been addressed (commit 7d7a2a82, folded into current HEAD):

  • packages/shared/src/i18n/en.json:169 and packages/shared/src/i18n/pt-BR.json:169 now both use single-brace {item} — confirmed by direct read.
  • A new regression test, apps/mobile/__tests__/lib/i18n-interpolation.test.ts, exercises the real i18next instance (not a mocked t()) to prove both locales resolve correctly, closing the gap that let the original bug slip past the mocked use-offline.test.tsx coverage.

No new Critical/High findings surfaced in this round.

Findings

  • Info: The common block in both locale JSON files isn't strictly alphabetized — pre-existing, not introduced by this PR, not actionable.
  • Info: Multi-toast-per-flush UX relies on the existing FIFO queue in apps/mobile/stores/app-toast-store.ts — verified sequential showError calls enqueue correctly rather than clobber.

Subagents

Agent Verdict
parity-checker N/A — mobile-only by architecture; apps/web/hooks/use-offline.ts is a bare navigator.onLine check with no client-side offline queue/flush/drop concept to mirror (web uses Server Actions/BFF).
i18n-syncer IN SYNC — all 9 new keys present and non-empty in both locales; single-brace interpolation confirmed correct on syncDropped.
contract-aligner N/A — no packages/shared/src/types/* or endpoints.ts touched.
security-reviewer N/A — no orbit-api code touched.
design-reviewer N/A — no UI/JSX files touched.

Not verifiable in this job

  • contract-aligner / the orbit-api side of the backward-compat guard — sibling repo not checked out in this CI job.
  • Phase 6 (/validate) skipped per CI adaptation — Build/Unit Tests/SonarCloud run as separate required checks.

Files reviewed: apps/mobile/hooks/use-offline.ts, apps/mobile/lib/offline-mutations.ts, apps/mobile/__tests__/hooks/use-offline.test.tsx, apps/mobile/__tests__/lib/offline-mutations.test.ts, apps/mobile/__tests__/lib/i18n-interpolation.test.ts, packages/shared/src/i18n/en.json, packages/shared/src/i18n/pt-BR.json.

…lines count (#243)

The offline dropped-mutation toast lives in hooks/use-offline.ts, but the
vitest coverage `include` only scoped lib/** and stores/**, so the hook's
well-tested new lines (the droppedMutations loop + showError) produced no
lcov rows. SonarCloud therefore counted them as uncovered, holding new-code
coverage at 59.1% even though use-offline.test.tsx exercises them.

Add hooks/use-offline.ts to the coverage include. It now reports at 92.7%
lines (only the pre-existing AppState handler + enqueue callback uncovered);
aggregate coverage stays above every threshold (stmts 84.4 / branch 72.5 /
funcs 84.2 / lines 86.5), so the new-code-coverage gate reflects the truth.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@thomasluizon

Copy link
Copy Markdown
Owner Author

Update — fixed the new-code-coverage gate at source instead of leaving it.

Root cause: the dropped-mutation toast lives in hooks/use-offline.ts, but apps/mobile/vitest.config.ts scoped the coverage include to lib/** + stores/** only. The hook's new lines (the droppedMutations loop + showError) were genuinely exercised by use-offline.test.tsx, but produced no lcov rows, so SonarCloud scored them as uncovered — pinning new-code coverage at 59.1% despite real tests.

Fix (f53a2a08): added hooks/use-offline.ts to the coverage include. It now reports at 92.7% lines (only the pre-existing AppState handler + enqueue callback remain uncovered — not new code). Aggregate coverage stays above every threshold (stmts 84.4 / branch 72.5 / funcs 84.2 / lines 86.5), so the local Unit Tests gate is unaffected. No rubber-stamp tests were added — this makes the gate reflect coverage that already exists.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #456

Scope: PR #456 in thomasluizon/orbit-ui-mobilefix(mobile): surface offline mutations dropped on validation errors
Recommendation: APPROVE

Summary

This PR fixes a real bug: flushQueuedMutations() used to silently remove() a queued offline mutation when the server permanently rejected it (400/404/409/410/validation), leaving the user with no feedback that their edit never landed. The fix threads a droppedMutations array up through handleFlushFailure → processQueuedMutationFlush → runQueueFlush → flushQueuedMutations, and useOffline().flush() now raises one showError toast per drop via react-i18next, with new common.syncDropped / common.syncEntity.* keys landed in both en.json and pt-BR.json in the same diff. All 8 changed files (2 production, 3 test, 1 config, 2 i18n) were checked against every rubric dimension the diff touches. Nothing Critical, High, or concretely-actionable Medium survived.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

  • DroppedMutation.lastError is typed string | null, but handleFlushFailure (apps/mobile/lib/offline-mutations.ts:523,536) only ever populates it from getErrorMessage(error), which never returns null. Cosmetic type-precision looseness, not a behavior bug — not posted per the Signal gate.
  • common.syncEntity.profile renders as "settings" (packages/shared/src/i18n/en.json:175) while the nav tab is labeled "Profile" (nav.profile) elsewhere in the same file. Plausibly intentional (the profile mutation scope spans language/theme/timezone/marketing-consent, broader than just the Profile screen) — a wording nit, not blocking.

Subagents

Agent Verdict
parity-checker N/A — mobile-only feature; confirmed apps/web/hooks/use-offline.ts is a bare navigator.onLine detector with zero queue/flush/drop concept, and a repo grep across apps/web for offline-queue/mutation-drop patterns returned nothing to mirror
i18n-syncer IN SYNC — all 9 new keys (syncDropped + 8 syncEntity.*) present in both locales, single-brace {item} interpolation matches existing convention, all 8 MutationScope values from getMutationScope map to a translated key with no gaps
contract-aligner N/A — no packages/shared/src/types/* or endpoints.ts changes
security-reviewer N/A — no orbit-api files in the diff
design-reviewer N/A — diff touches only hook/lib logic and toast text, no apps/* UI markup, styling, or DESIGN.md token surface

Validation

Check Result
Lint N/A — skipped (CI-wrapper invocation of /pr-review skips Phase 7 by design)
Type check N/A — skipped, same reason
Tests N/A — skipped, same reason (PR body states 1044 mobile + 1432 shared tests pass locally)
Build (api) N/A — no orbit-api changes

Deferred — N/A dimensions & files not verdicted

  • DESIGN.md / AI-slop (#8): N/A — no apps/* UI markup/styling files changed; toast rendering itself is a pre-existing, unmodified primitive.
  • Contract drift + backward-compat (#11): N/A — no shared Zod schema or orbit-api DTO changes; nothing for the backward-compat guard to evaluate.
  • Backend hard rules (#13): N/A — no orbit-api files touched.
  • Security, API side (#12): N/A — no orbit-api code changed; the frontend-security categories (XSS, auth-state leakage) were checked directly and are clean (no new user input rendered, no dangerouslySetInnerHTML, no auth-state exposure change).
  • FEATURES.md parity (#14): N/A — this is a bugfix to internal offline-sync error handling, not a new screen/tool/gating/platform/locale surface; FEATURES.md has no existing entry for the offline queue mechanism to update.
  • Phase 7 (Validate): skipped — this run was detected as the CI-wrapper invocation, which per the skill's own instructions skips local validation and defers to the wrapper's own checks (Build / Unit Tests / SonarCloud run as separate required checks on this PR).
  • Every one of the 8 changed files (use-offline.test.tsx, i18n-interpolation.test.ts, offline-mutations.test.ts, use-offline.ts, offline-mutations.ts, vitest.config.ts, en.json, pt-BR.json) received a verdict above — nothing changed was left unexamined.

What's good

  • Correct root-cause fix: the drop info is threaded through the actual call chain rather than bolted on with a side-channel.
  • The "exactly one toast per drop across ~10 useOffline consumers" design (module-level flushInFlight guard returns an empty array to every non-winning caller) is correct and is explicitly tested.
  • Behavioral tests, not rubber-stamps: offline-mutations.test.ts asserts call order (bad mutation attempted first, good one still flushes), the drop is reported with the right shape, and the survivor is retained; use-offline.test.tsx asserts toast count + per-scope message content, and a zero-toast case when nothing drops.
  • i18n keys landed in both locales in the same commit, using the app's actual single-brace interpolation convention (verified against apps/mobile/lib/i18n.ts), with a dedicated regression test (i18n-interpolation.test.ts) guarding that convention.
  • inferScopegetMutationScope rename is clean — no stale references to the old private name anywhere in the tree.
  • Parity claim in the PR body ("offline mutations are mobile-only, no web mirror exists") was independently verified, not just trusted.

Recommendation

No changes required before merge. The two Low/Info notes (the lastError type being wider than its actual values, and the "profile → settings" wording) are optional polish, not blockers.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #456 — fix(mobile): surface offline mutations dropped on validation errors

Recommendation: REQUEST CHANGES

Summary

The PR correctly plumbs a droppedMutations array from handleFlushFailure up through runQueueFlush/flushQueuedMutations to useOffline().flush, which now shows one per-mutation error toast via the app's existing toast queue (verified it genuinely queues rather than clobbers). i18n keys are complete and correct in both locales, matching every MutationScope enum value, and the single-brace interpolation fix is covered by a new integration test. However, the fix has an unaddressed gap on the automatic backoff-retry path that reproduces the exact silent-drop bug the PR exists to fix.

Findings

High

Mutations dropped during the automatic backoff-retry flush never notify the user

  • apps/mobile/lib/offline-mutations.ts:300 (inside scheduleBackoffFlush, lines 286-303)
  • flushQueuedMutations() has two production call sites: apps/mobile/hooks/use-offline.ts:55 (the hook's flush(), which reads droppedMutations and calls showError per drop) and apps/mobile/lib/offline-mutations.ts:300 inside scheduleBackoffFlush's setTimeout callback, which calls await flushQueuedMutations() and discards the return value entirely. There is no toast store write, event emitter, or persisted "pending notification" anywhere else in offline-mutations.ts that could recover a drop that happens on this path.
  • Reachable sequence: a mutation flush hits a transient network error → runQueueFlush sets stopReason: 'network'scheduleBackoffFlush() schedules a setTimeout retry, decoupled from any useOffline hook instance. When that retry fires and a mutation gets a non-transient rejection (entity deleted server-side → 404/409/410, validation failure, or retry exhaustion), handleFlushFailure removes it from the queue and adds it to that call's droppedMutations — immediately discarded at line 300. The user's edit is permanently lost with zero feedback, on a flaky-connectivity-then-fails-again path — precisely the bug class this PR's description says it fixes.
  • Fix: thread the backoff-triggered flush's droppedMutations to the same notification path — e.g. hoist the toast-emission logic used in useOffline.flush into a shared exported helper that scheduleBackoffFlush also calls, or route the backoff retry through the hook's own flush() via a subscription rather than calling flushQueuedMutations() directly at module scope.

Medium

getMutationScope silently mismaps restoreGoal, restoreTag, and setName to the habits scope

  • apps/mobile/lib/offline-mutations.ts:342-386 — the goals case (344-351) omits restoreGoal; the tags case (352-356) omits restoreTag; the profile case (370-382) omits setName; all three fall through to default: return 'habits'.
  • This function was previously used only internally for query-cache invalidation (wrong scope there just means an extra/missing invalidation — low visibility). The PR is the first time its return value feeds user-facing copy (t('common.syncEntity.${getMutationScope(dropped.type)}')), so the pre-existing gap now surfaces as visibly wrong text: a dropped restoreGoal/restoreTag/setName mutation shows "Couldn't sync your habit change" instead of goal/tag/settings.
  • Confirmed all three MutationType values are real and in active use (use-goals.ts:209, use-tags.ts:281, edit-name-sheet.tsx:41).
  • Fix: add the three missing cases to their correct branches, and add a test asserting getMutationScope covers every MutationType (e.g. iterate the shared enum and assert none silently defaults) so this class of gap can't recur.

Subagents

Agent Verdict
parity-checker PAIRED — apps/web/hooks/use-offline.ts is a bare navigator.onLine wrapper with no queue/retry/drop concept; web has no offline mutation queue at all (confirmed via broad grep). Mobile-only offline queue is an allowed platform-adapter difference per root CLAUDE.md.
i18n-syncer IN SYNC — both en.json and pt-BR.json contain all 9 new keys under common.*; every MutationScope enum value (habits, goals, tags, notifications, profile, userFacts, apiKeys, calendar) has a matching common.syncEntity.<scope> key in both locales; pt-BR translations read as natural Portuguese; no orphaned callsites.
contract-aligner N/A — no packages/shared/src/types/* or endpoints.ts changed, no orbit-api changed.
security-reviewer N/A — no orbit-api code changed.
design-reviewer N/A — no apps/* UI/component files changed (only hooks, lib, tests, i18n JSON).

What's good

  • Concurrency reasoning (flushInFlight guard ensures exactly one toast per drop, not N duplicate toasts from N useOffline consumers) is correct.
  • Toast-store behavior verified: showError queues rather than overwrites, so multiple simultaneous drops each get their own toast in order.
  • Good test coverage of the primary path: offline-mutations.test.ts's new "drops a validation-rejected mutation, keeps flushing the rest" test asserts call order, remove(), clearOfflineEntity(), and the returned droppedMutations shape; use-offline.test.tsx asserts toast count and per-scope message content, plus a "no toast when nothing drops" negative case.
  • The i18n single-brace-interpolation regression (from the immediately preceding commit in this branch) now has a real integration test against the actual i18n singleton, in both locales.
  • Clean rename of inferScopegetMutationScope (export, not duplicate) — good DRY, no new abstraction invented.

Recommendation

Fix the backoff-retry silent-drop gap (High) before merge — it's a small, well-scoped fix directly in the bug class this PR targets. Fix the getMutationScope mismap (Medium) in the same PR since it's adjacent, one-line-per-case work.

succeeded: outcome.succeeded,
failed: outcome.failed,
remaining: outcome.remaining,
droppedMutations: outcome.droppedMutations,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] droppedMutations is discarded at the other call site of flushQueuedMutationsscheduleBackoffFlush (line 300)

flushQueuedMutations() has two production callers: useOffline.flush() (hooks/use-offline.ts:55), which reads this droppedMutations return and shows a toast per drop, and scheduleBackoffFlush's setTimeout callback in this file (line 300: await flushQueuedMutations()), which discards the return value entirely. There's no toast/event/persisted-notification path anywhere in this module that could recover a drop on the backoff-retry path.

Reachable sequence: a flush hits a transient network error → stopReason: 'network'scheduleBackoffFlush() schedules a retry decoupled from any useOffline instance. When that retry fires and a mutation gets a non-transient rejection (404/409/410/validation, or retry exhaustion), handleFlushFailure drops it here silently — reproducing the exact "user's edit vanishes with no feedback" bug this PR fixes.

Suggest threading this same droppedMutations array from the backoff callsite through the toast path (shared helper, or route the backoff retry through the hook via subscription).

#243)

Addresses the review on #456: droppedMutations were only surfaced by
useOffline.flush()'s return value, so drops discovered by the decoupled
scheduleBackoffFlush() retry were silently lost (the exact "edit vanishes with
no feedback" bug this PR fixes).

Add a subscribeDroppedMutations emitter fired at the single drop point in
runQueueFlush, so every flush path notifies. The root OfflineManager subscribes
once and shows the toast; useOffline no longer toasts from its return value
(avoids 11x toasts across the 11 useOffline call sites). Emitter covered by a
new offline-mutations unit test; the moved toast lives in the coverage-excluded
providers.tsx.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review — #456: fix(mobile): surface offline mutations dropped on validation errors

Recommendation: Approve

Severity Count
Critical 0
High 0
Medium 2
Low/Info 0

This review covers the diff since my last CHANGES_REQUESTED review (2026-07-12T08:11:52Z), i.e. commit aab0fd78 ("surface dropped mutations from the backoff-retry path too").

Resolved since last review

High — backoff-retry drops silently lost, now fixed. The prior review flagged that scheduleBackoffFlush()'s call to flushQueuedMutations() discarded droppedMutations entirely, so drops discovered by the decoupled retry timer never notified the user — reproducing the exact silent-drop bug this PR exists to fix.

aab0fd78 fixes this correctly: a module-level subscribeDroppedMutations emitter (offline-mutations.ts) fires from the single drop point inside runQueueFlush, so every flush path — foreground and backoff-retry alike — notifies. OfflineManager (mounted exactly once at the provider root, providers.tsx:160) is the sole subscriber and shows the toast; useOffline().flush no longer toasts from its return value, which also avoids the 11x-duplicate-toast risk across useOffline's 11 call sites. The emitter itself has a new, solid unit test (subscribe → notified on drop → unsubscribe → no longer notified). Verified this is a real fix, not a reshuffle.

Findings

Medium

1. getMutationScope still silently mismaps restoreGoal, restoreTag, and setName to the habits scope (carried over, not addressed this iteration)

  • apps/mobile/lib/offline-mutations.ts:364-406 — the goals case omits restoreGoal, the tags case omits restoreTag, the profile case omits setName; all three still fall through to default: return 'habits'.
  • Flagged in my prior review; not touched by aab0fd78. Since this function's return value now feeds user-facing toast copy, a dropped restoreGoal/restoreTag/setName mutation still shows "Couldn't sync your habit change" instead of goal/tag/settings.
  • Not blocking — pre-existing gap, low-frequency mutation types, cosmetic wrong-copy rather than data loss. Worth a follow-up.

2. The moved toast-wiring in providers.tsx has zero test coverage

  • apps/mobile/lib/providers.tsx:49-57 — the useEffect that subscribes to subscribeDroppedMutations and calls showError (the fix for the High finding above) is untested. The two tests that previously covered this behavior (use-offline.test.tsx: "raises one error toast per dropped mutation", "shows no error toast when a flush drops nothing") were deleted wholesale in aab0fd78 with no replacement, and providers.tsx is in vitest.config.ts's coverage exclude list (unchanged by this PR) — confirmed via git show and repo-wide grep for any test rendering Providers/OfflineManager.
  • The commit message self-acknowledges this: "the moved toast lives in the coverage-excluded providers.tsx."
  • Risk is regression-only (the emitter itself and the scope-mapping are unit-tested in isolation), but this useEffect is the exact user-facing wiring for issue #243 — an unmount/resubscribe bug or an effect that never fires would silently reintroduce the original "edit vanishes with no feedback" bug with nothing to catch it.
  • Suggested fix (non-blocking): a lightweight render test of OfflineManager (or the full Providers tree) that calls the module's drop-emitter directly and asserts showError fires with the expected translated copy.

Subagents

Agent Verdict
parity-checker PAIRED — apps/web/hooks/use-offline.ts is a bare navigator.onLine wrapper with no queue/flush/drop concept; web has no offline mutation queue at all. Mobile-only offline queue is an allowed platform-adapter difference per root CLAUDE.md. i18n keys (common.syncDropped, common.syncEntity.*) correctly appear only in mobile's call path.
i18n-syncer IN SYNC — en.json and pt-BR.json both contain all keys (3315/3315), every MutationScope enum value has a matching common.syncEntity.<scope> key in both locales, no orphaned callsites.
skeptic (missing-toast-test finding) CONFIRMED — verified via git show, coverage-config diff, and repo-wide grep; see Medium finding #2 above.
contract-aligner N/A — no packages/shared/src/types/* or endpoints.ts changed, no orbit-api changed. Not verifiable in this CI job regardless (sibling repo not checked out).
security-reviewer N/A — no orbit-api code changed.
design-reviewer N/A — no apps/* UI/component files changed (only hooks, lib, tests, i18n JSON).

What's good

  • The emitter-based fix for the backoff-retry gap is architecturally sound: single drop point, single subscriber, no duplicate-toast risk, well-tested at the emitter level.
  • Clean removal of dead toast logic from useOffline once ownership moved to OfflineManager — no leftover unused imports/state.
  • i18n parity and locale correctness both hold up across iterations of this PR.

Recommendation

No Critical/High findings remain. Approve. The two Medium items (scope mismap, missing wiring-level test) are real but non-blocking — worth a fast follow-up, not a merge gate.

@thomasluizon
thomasluizon merged commit 9e59345 into main Jul 12, 2026
19 checks passed
@thomasluizon
thomasluizon deleted the fix/offline-dropped-mutation-notify branch July 12, 2026 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant