fix: correctness batch from the second review#34
Merged
Conversation
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
enabled auto-merge
July 14, 2026 14:16
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.
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, soisLoggedInstays false until the emailis verified. But it never set
authInterceptor.pendingToken, which the login path does set. Since/v1/auth/resend-verificationis authenticated, "Resend Email" straight after registering went out withno bearer and 401d - and
runCatching { api.resendVerification() }with noonFailureswallowed it, sothe UI cheerfully claimed the mail had been re-sent.
Set
pendingTokenat registration, and report the result.AwaitingEmailVerificationis now a data classcarrying its own
error/resent, because routing a resend failure throughAuthUiState.Errorwouldhave 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
Nothing ever set
deleteErrorback to null except a successful delete, which you cannot perform while amodal is up. Added
clearDeleteError().Backup download lost across the file picker
pendingDownloadJobIdwas a plainrememberheld across the SAFCreateDocumentround trip. That pickeris 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
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
scopeis 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 { ... }fromonUpdate. OnceonUpdatereturns 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/exporthad no@Streaming. Retrofit therefore callsbody().bytes()internally, so the callersbyteStream().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:testPlayReleaseUnitTestgreen.(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
s force simulation is O(n^2) per step for up to 600 steps, and it runs on the main thread viawithFrameNanos`. Correctness fix is here; moving it off the main thread (and/or Barnes-Hut) is its own PR.(
CreateChannelScreenloses its form on a plain rotation). Separate PR.