Close the subscription list's offline write hole, and stop four catch blocks hiding defects - #157
Merged
Conversation
The subscription list was the one write-capable screen with no offline
guard, and the one most likely to be open without a network - it serves
itself from the SQLite mirror. Swipe-to-delete and mark-as-paid stayed
enabled, and both reported a lost write with read wording ("You appear to
be offline"), which reads as "we will sync it". There is no outbox, so
the write was simply gone. Mark-paid is the worse of the two: the row
keeps its OVERDUE chip either way, so nothing on screen contradicts the
misreading.
It now follows the same contract as the other four write-capable view
models: IConnectivityService injected, IsOffline re-read on load and
after a failed write, CanSubmit gating both affordances in XAML, and
ToWriteFailureMessage on the write paths. IsBusy is new here rather than
reused from IsLoading, which drives the pull-to-refresh spinner - a
double tap on mark-paid would settle two billing periods off one payment.
Also stops four catch blocks laundering defects into offline errors. The
two load paths were the dangerous ones: they fall back to cache, so a bug
of ours showed stale rows under "showing last synced data" on a phone
that was perfectly online, with nothing logged. All four now filter on
ApiErrorMapper.IsApiFailure, as the other 21 blocks already did.
And corrects two comments that still described the removed nightly
billing-date job - both told the reader the opposite of the rule in
CLAUDE.md, inviting the exact change it forbids.
Closes #152
Closes #153
Closes #154
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three review findings from a full-repo pass, all in the mobile client.
#155from the same pass is already in via #156 and is not touched here.Closes #152 — the list screen's offline write hole
The subscription list was the one write-capable screen with no offline guard, and it is the one most likely to be open without a network — it serves itself from the SQLite mirror and says so ("Offline — showing last synced data"). Two writes sat on it unguarded:
Both stayed enabled with no network, and both reported the resulting lost write with read wording — "You appear to be offline." — which reads as "we'll sync it". There is no outbox (#144), so the write was simply gone.
Mark-paid is the worse of the two. The row keeps its OVERDUE chip whether the call landed or not, so nothing on screen contradicts the misreading — the user believes they cleared a charge that is still outstanding.
It now follows the contract the other four write-capable view models already share:
IConnectivityServiceIsOfflineCanSubmitIsEnabledin XAMLToDisplayMessageToWriteFailureMessageIsBusyis new here rather than reused fromIsLoading, which drives the pull-to-refresh spinner. Mark-paid is not idempotent — it advances the billing date one cycle per call — so a double tap would settle two periods off one payment.Closes #153 — four catch blocks laundering defects into offline errors
ApiErrorMapper.IsApiFailureexists to be a catch filter; 21 of 25 blocks used it, four did not. The two load paths were the dangerous ones: they fall back to cache, so a defect of ours (a null inRebuildGroups, a bad cache round trip, a converter throwing) showed stale rows under "showing last synced data" on a phone that was perfectly online, with nothing logged. That is the hardest class of bug to get a report about, because it looks like it is working as designed.All four now filter.
SubscriptionDetailViewModel:253is left alone — its broad catch is documented as intentional and nothing falls back to cache there.Closes #154 — comments describing a job that no longer exists
RelativeDate.cstold the reader a past billing date "means the app is looking at stale data" because "the server's billing-date job rolls them forward". That job was deliberately removed; a past date is the intended overdue signal and is whatSubscriptionDto.IsOverduereads. The comment invited exactly the change CLAUDE.md forbids. Same drift inRenewalNotificationPlanner.cs.Verification
Test coverage added, since the absence of it is why #152 went unnoticed:
OfflineWriteGuardTestsnow includes the list in bothCanSubmitassertions, plus a failed mark-paid, a failed swipe-delete, and the drop-mid-session caseSubscriptionListViewModelTests/DashboardViewModelTestsassert a defect propagates instead of turning into a cache fallbackNo API, schema or contract changes.