Skip to content

fix: correctness batch from the second review#34

Merged
SiteRelEnby merged 2 commits into
mainfrom
fix/review-batch2-correctness
Jul 14, 2026
Merged

fix: correctness batch from the second review#34
SiteRelEnby merged 2 commits into
mainfrom
fix/review-batch2-correctness

Conversation

@SiteRelEnby

Copy link
Copy Markdown
Collaborator

Second of three PRs from the latest review (security batch is #33; self-hosting + CI is next). Seven
independent bugs, none of which needed a user to hit them first.

Registration cannot resend its verification email

register() holds the new tokens in ViewModel fields only, so isLoggedIn stays false until the email
is verified. But it never set authInterceptor.pendingToken, which the login path does set. Since
/v1/auth/resend-verification is authenticated, "Resend Email" straight after registering went out with
no bearer and 401d - and runCatching { api.resendVerification() } with no onFailure swallowed it, so
the UI cheerfully claimed the mail had been re-sent.

Set pendingToken at registration, and report the result. AwaitingEmailVerification is now a data class
carrying its own error / resent, because routing a resend failure through AuthUiState.Error would
have dropped the user back to the login form (the step selector only shows the verify step while the state
is AwaitingEmailVerification).

A failed history delete traps you in a dialog

onDismissRequest = { },                                        // back / scrim: dead
confirmButton = { TextButton(onClick = { viewModel.loadInitial() }) { Text("OK") } }   // never clears deleteError

Nothing ever set deleteError back to null except a successful delete, which you cannot perform while a
modal is up. Added clearDeleteError().

Backup download lost across the file picker

pendingDownloadJobId was a plain remember held across the SAF CreateDocument round trip. That picker
is a separate activity, so ours can be recreated behind it; on return the id is null, the if (uri != null && jobId != null) guard silently drops the download, and the user gets nothing after picking a location.
rememberSaveable.

Relationships: one failed load poisons the node

if (loadedFor == scope to nodeId) return
loadedFor = scope to nodeId        // set BEFORE the request

Latched before the load ran and never cleared on failure, so a single offline load meant that member or
group could never load relationships again for the life of the ViewModel. Latch on success only, add a
Retry. The post-mutation reload also dropped its error, showing a stale list after a save that actually
worked; it now says "Saved, but couldn`t refresh the list".

Relationship graph can render the wrong scope

No generation guard: tap Members then Groups, and if the members response lands second it writes itself
into state while scope is still "groups". Cancel the in-flight request and clear the stale graph.

Widgets can be killed mid-refresh

All six receivers did CoroutineScope(Dispatchers.IO).launch { ... } from onUpdate. Once onUpdate
returns the receiver is done and the process is killable, so a network refresh can be cut off and the
widget just stays stale. Shared refreshWidgets() helper: goAsync() to hold the broadcast open,
finish() in a finally, and an 8s bound so a hung refresh gets dropped rather than turning into an ANR
(the broadcast deadline is 10s foreground / 60s background).

JSON export was buffered, not streamed

/v1/export had no @Streaming. Retrofit therefore calls body().bytes() internally, so the callers byteStream().copyTo(output)- which *looks* like streaming, and whose KDoc says "streamed straight into [uri]" - was copying from a byte array already sitting in the heap. Large system, low-heap device, OOM. The zip download already had@Streaming`; added it to the JSON export and to the admin dossier export.

Verification

  • :app:assemblePlayRelease :app:testPlayReleaseUnitTest green.
  • Device pass worth doing on: register a fresh account and hit Resend; force a history delete failure
    (airplane mode) and dismiss the dialog; rotate during the backup file picker; open a member with the
    server unreachable and hit Retry; flip the graph tabs quickly.

Deferred to follow-ups

  • The graphs force simulation is O(n^2) per step for up to 600 steps, and it runs on the main thread via withFrameNanos`. Correctness fix is here; moving it off the main thread (and/or Barnes-Hut) is its own PR.
  • Channel / reminder editor drafts are held in plain ViewModel state and are lost on process death
    (CreateChannelScreen loses its form on a plain rotation). Separate PR.

Seven independent bugs, all found by review rather than by users, which is the
better order.

Registration: register() kept the new tokens in ViewModel fields only (so
isLoggedIn stays false until verification) but never set the interceptor's
pendingToken the way the login path does. resend-verification is authenticated,
so "Resend Email" right after registering went out with no bearer and 401'd, and
a bare runCatching {} swallowed the failure, so the UI claimed it had been sent.
Set pendingToken at registration, report the outcome either way, and give
AwaitingEmailVerification its own error/notice so a failed resend doesn't flip to
the Error state and dump the user back on the login form.

History: the "Delete failed" dialog had a no-op onDismissRequest and nothing ever
cleared deleteError, so OK just reloaded the list behind a dialog that re-rendered
forever. Added clearDeleteError() and wired every exit to it.

Export: the pending job id for a full-backup download lived in a plain remember
across the SAF create-document round trip. The picker is a separate activity, so a
recreation lost the id and the download was dropped with no message after the user
had already chosen where to save it. rememberSaveable.

Relationships editor: loadedFor was latched before the request ran and never
cleared on failure, so a single failed load permanently suppressed retry for that
node. Latch on success only, and add a Retry. The post-mutation reload also
swallowed its error, leaving a stale list after a successful save; it now says so.

Relationship graph: no request-generation guard, so flipping Members -> Groups
could paint the (slower) members response under the Groups tab. Cancel the previous
request and clear the stale graph while the new one loads.

Widgets: all six receivers launched a network refresh into an ad-hoc
CoroutineScope from onUpdate. Once onUpdate returns the receiver is finished and
the process can be killed mid-request. Shared refreshWidgets() helper that holds
the broadcast open with goAsync() and bounds the work inside the broadcast
deadline.

Export API: /v1/export had no @streaming, so Retrofit buffered the whole body into
a byte array before the caller ever saw it. The caller looks like it streams
(byteStream().copyTo()) but was copying from memory, so a large system's export
could exhaust the heap. The zip download already had @streaming; added it to the
JSON export and the admin dossier.
@SiteRelEnby
SiteRelEnby enabled auto-merge July 14, 2026 14:16
@SiteRelEnby
SiteRelEnby merged commit 7d3b7f1 into main Jul 14, 2026
1 check passed
@SiteRelEnby
SiteRelEnby deleted the fix/review-batch2-correctness branch July 14, 2026 14:21
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