Conversation
Found while testing the field workflow on a physical iPhone: tapping Download on the embedding pack showed a loading wheel that never stopped, with no error and no way to retry. Two independent defects combine to produce that. ganeshaApiClient called fetch with no deadline. A connection that is accepted but never answered -- a captive portal, a dropped cellular handover, a backend that stalls mid-response -- leaves the promise pending forever. Every screen that awaits it spins indefinitely. Requests now abort after 30s (generous, since these can be the first call after an Azure Function cold start) and report a distinct `timeout` code, kept separate from `network-error`: one means "no network", the other means "the server took the call and went quiet", and those want different responses from someone standing in a reserve. SignInScreen.handleSignIn cleared its spinner on each exit path rather than in a finally, and the path after getUserProfile() had no protection at all. Combined with the missing timeout, a stalled profile lookup left the button spinning with the session already stored -- so there was no way to tell whether sign-in had succeeded. The spinner now clears in a finally, and a profile failure says the session was saved rather than "Sign-in failed", which was inviting a pointless second sign-in. handleDownloadPack already had a correct try/finally; it was spinning because the awaited API call never settled, not because it leaked state. Not fixed here: RNFS.downloadFile sets no connection/read timeout either. It is a different failure mode -- it reports progress and honours an abort signal -- and on the reported symptom nothing had reached disk, no staging file, so the stall was upstream of the transfer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follows the same defect as the request timeout in this branch, on the path that was actually hanging in the field. react-native-app-auth bounds neither authorize() nor refresh(). On a marginal link the interactive flow gets all the way through -- the browser opens, the person authenticates, Entra redirects, ASWebAuthenticationSession intercepts the callback and the sheet closes -- and then the POST that trades the authorization code for tokens stalls. authorize() never settles. The sign-in screen spins forever with no error, and because OIDExternalUserAgentIOS discards the BOOL from resumeExternalUserAgentFlowWithURL: and reports every failure as "user cancelled", nothing upstream can tell the difference. Reproduced on a device syslog showing the link at -74dBm with 40% beacon loss. That is an ordinary reserve connection, not an edge case, and it is the condition this app is built for. The interactive budget is 180s: it has to cover reading a password manager and approving an MFA push on a second device, so a short deadline would fail people who were otherwise succeeding. It is there to bound the hang, not to be reached. Refresh is non-interactive and gets 30s, and a refresh that cannot complete reports "no valid session" rather than throwing, matching how an expired refresh token is already handled. Still unbounded: RNFS.downloadFile in fileDownloadService. It reports progress and honours an abort signal, so it is visible rather than silent, but it wants the same treatment before this ships to a reserve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
iOS sign-in has never worked. It fails silently, and the trailing slash in the redirect URI is why. Entra normalises a custom-scheme redirect that carries no path, so a request sent as `org.ganesha.elebook://oauthredirect` comes back as `org.ganesha.elebook://oauthredirect/`. AppAuth-iOS compares the callback against the configured redirect component by component, path included, so '' != '/' and shouldHandleURL: rejects it. OIDExternalUserAgentIOS then discards the BOOL from resumeExternalUserAgentFlowWithURL:, so the rejection is never reported and the authorization session waits forever. The person sees a spinner that never stops, with no error and nothing to retry. Android never noticed. Its intent filter matches on the scheme alone (appAuthRedirectScheme in android/app/build.gradle), so the extra slash is irrelevant there. Same config, same Entra registration, opposite outcomes. Captured on an iPhone 15 Pro by routing the flow through the external browser so the callback surfaced in application(_:open:): scheme: org.ganesha.elebook host: oauthredirect path: '/' resumed: REJECTED (url mismatch) full: org.ganesha.elebook://oauthredirect/?code=1.AQkA3Rhcn71... Storing the redirect the way Entra returns it makes the comparison succeed on iOS and changes nothing on Android, which never inspected the path. The pre-fix spelling is accepted and normalised so existing configs self-heal rather than failing validation. One migration consequence: getTokenStorageService digests redirectUrl, so the Keychain/Keystore service name moves and anyone already signed in is signed out once and has to sign in again. Observations, packs and the local database are untouched. iOS loses nothing, never having been able to sign in. The field contributes no isolation in any case -- it is validated to a single constant, so every deployment shares it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two more ways the same silent failure reached the field, found while testing
the model download on a physical iPhone.
RNFS.downloadFile never asked for a background session, so the transfer ran
on a foreground URLSession and stopped the moment iOS suspended the app --
the screen locking part-way through an 80MB model was enough. The watchdog
then correctly reported "no data for 60s", which was true and useless: the
data stopped because iOS stopped it. Nine attempts moved roughly 245MB for
an 82MB file and installed nothing, failing at scattered offsets that
tracked when the person looked away rather than anything about the link.
This is a regression, not an oversight. Upstream removed the foreground path
deliberately ("use background downloads exclusively", 02b637e) and
AppDelegate still services handleEventsForBackgroundURLSession -- plumbing
for a background session nothing was requesting. The rewrite of this service
lost the flag.
Restoring it needs the watchdog to cope with suspension too. JS timers do not
run while the app is suspended and fire late on wake, so a deadline armed
before suspension would trip instantly against a transfer that had been
progressing in the background the whole time. The watchdog now re-arms when
the app becomes active and judges inactivity from then.
Separately, prepareMiewidModel collapsed every non-checksum failure into
status 'missing' and dropped outcome.code and outcome.message on the floor.
A stalled transfer, an unreachable host, a 404 and a cancellation all
rendered as "status: missing", with the real reason going to logger.warn,
which release builds compile out. The record now carries the reason and the
screen shows it: "timeout: download received no data for 60s" instead of one
word. That one change turned an unexplained failure into a diagnosis on the
first retry.
The field is optional because records persisted by earlier builds do not
have it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-timeout-and-signin-spinner
Every job that installs the Android SDK started dying in setup: Error: The process '.../cmdline-tools/16.0/bin/sdkmanager' failed with exit code 1 "To get started with the Android SDK Preview, you must agree to..." android-actions/setup-android@v3 follows a floating cmdline-tools default that moved to 16.0, whose sdkmanager demands agreement to the Android SDK *Preview* licence -- which accept-android-sdk-licenses does not cover. lint, test and android-build all failed within 30 seconds on every open PR, while typecheck stayed green because it is the only job that never installs the SDK. Nothing in the tree caused it and nothing in the tree could fix it. Pinning the tools version takes the preview channel out of the path. The value is a guess that CI has to confirm; if 13.0 is not a tag this action publishes, the setup step will say so plainly and the pin can move. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
blclo
changed the title
10 of 13 tasks
The previous pin used '13.0', which this action cannot resolve to a download -- setup failed with a bare HTTP 404 on all three SDK jobs. cmdline-tools-version takes the published build number; 11076708 is command-line tools 11.0, comfortably off the preview channel that was demanding an unaccepted licence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two attempts, neither worked, and I cannot test this locally.
Pinning cmdline-tools by version string ('13.0') 404s -- the input takes a
published build number. Pinning by build number (11076708) resolves and
installs, and sdkmanager still exits 1 while walking the licence list. So the
tools version was never the problem: licence acceptance itself is failing on
the runner, and android-actions/setup-android@v3 is not getting past it.
Backing the pin out rather than leaving a speculative change that does not
help and misleads the next person. CI remains broken for every job that
installs the Android SDK -- lint, test and android-build, on every open PR,
independent of this branch. typecheck stays green because it is the only job
that never touches the SDK.
Fixing it properly means pinning setup-android to a known-good release or
raising it upstream, which wants someone who can iterate against CI directly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 23, 2026
Collaborator
|
@blclo, while building a follow-up I think I found why the iOS download still hangs. In react-native-fs 2.20.0, Android's react-native-fs always settles, which is why Android never showed it. It is the same class of bug as itinance/react-native-fs#568. Your watchdog turns the hang into a 60 s timeout and a restart from zero. #45 is stacked on this branch. It resumes the transfer in place, waits for the foreground before the next request, and shows download progress on the Packs screen. It's JS-only, so a Metro reload of your dev build is enough. Could you run the iOS test plan in its description on your iPhone? Android regression data for this PR, from a Pixel 9a (Android 17) running a debug build of this branch plus #45:
The same run found that a failed download crashes the Android app. That is a pre-existing react-native-fs bug, and it also fires when your watchdog calls |
4 tasks done
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
2 participants
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.
Summary
Everything here is one defect wearing different clothes: iOS failed silently. Sign-in spun forever with no error, downloads died with no reason, and release builds logged nothing at all. Each fix either removes a way to fail invisibly, or fixes what the silence was hiding.
The root cause: iOS sign-in has never worked
Entra normalises a custom-scheme redirect that carries no path, so a request sent as
org.ganesha.elebook://oauthredirectcomes back asorg.ganesha.elebook://oauthredirect/. AppAuth-iOS compares the callback against the configured redirect component by component — including path — so''!='/'andshouldHandleURL:rejects it.OIDExternalUserAgentIOSthen discards theBOOLfromresumeExternalUserAgentFlowWithURL:, so the rejection is never reported and the session waits forever.Android never noticed. Its intent filter matches on the scheme alone (
appAuthRedirectScheme), so the extra slash is irrelevant. Identical config, identical Entra registration, opposite outcomes — which is why every Android tester succeeded, every iOS tester failed, and no error existed to investigate.Captured on an iPhone 15 Pro by routing the flow through the external browser so the callback surfaced in
application(_:open:):Downloads died whenever the screen locked
RNFS.downloadFilenever asked for a background session, so the transfer ran on a foregroundURLSessionand stopped the instant iOS suspended the app. The watchdog then reported "no data for 60s" — true and useless, since iOS is what stopped the data. Nine attempts moved ~245 MB for an 82 MB file and installed nothing, failing at scattered offsets that tracked when the person looked away, not the link.A regression, not an oversight: upstream removed the foreground path deliberately (
02b637e, "use background downloads exclusively") andAppDelegatestill serviceshandleEventsForBackgroundURLSession— plumbing for a session nothing requested. The rewrite of this service lost the flag.Restoring it requires the watchdog to survive suspension too: JS timers freeze while suspended and fire late on wake, so a deadline armed beforehand would trip instantly against a transfer that had progressed fine. It now re-arms on
active.Nothing had a deadline
ganeshaApiClientfetchtimeoutcodeauthorize()refresh()RNFS.downloadFileSignInScreenalso cleared its spinner per-exit-path rather than in afinally, so a stalled profile call left it spinning with the session already stored — signed in, with no way to tell.Failures said nothing useful
prepareMiewidModelcollapsed every non-checksum failure intostatus: 'missing', droppingoutcome.codeandoutcome.message; the real reason went tologger.warn, which release builds compile out. A stall, a 404, an unreachable host and a cancellation all rendered as one word. The record now carries the reason and the screen shows it — that single change turned an unexplained failure into a diagnosis on the first retry.CI
Every job installing the Android SDK began failing in setup on an Android SDK Preview licence prompt from
cmdline-tools16.0. Unrelated to this tree, but it blocks merging anything, so the tools version is pinned here.Type of Change
Screenshots / Screen Recordings
No layout change. Visible difference: failures now name themselves instead of spinning forever.
Checklist
General
Testing
npm test)1050 tests pass plus
tsc --noEmit. iOS is checked on a real basis: the redirect fix was confirmed on an iPhone 15 Pro, where sign-in now completes — it never had before. The download fix is not yet confirmed end to end; the model had not finished downloading when this was written, so treat that half as reasoned and unit-tested rather than proven.Android needs a regression check before merge. The reasoning that it is unaffected is sound — scheme-only intent filter,
backgroundignored on its download path — but it is reasoning, not a run, and the redirect value is one Android also sends.React Native Specific
Remainder N/A — no components, styling or lists touched.
Security
Additional Notes
Everyone signed in gets signed out once.
getTokenStorageServicedigestsredirectUrl, so the Keychain/Keystore service name moves and existing tokens are orphaned. Observations, packs and the local database are untouched. Costs one sign-in, Android only — iOS never had a session to lose. Worth a line in release notes.Add
org.ganesha.elebook://oauthredirect/to the Entra app registration before rolling out. Entra accepted the slash-less request and returned the slash-bearing redirect, so it normalises both, but registering exactly what the app now sends removes the assumption. Keep the existing entry so in-flight builds keep working.The CI pin is a guess CI must confirm. If
13.0is not a tag this action publishes, the setup step will say so and the pin can move.Worth fixing beyond this PR
CFNetworkDownload_*.tmpfiles accumulate intmp/with nothing cleaning them up.loggeris__DEV__-gated, so release builds emit nothing. Diagnosing this needed a custom IPA with the gate removed. A field app that runs where no debugger can follow should keep a minimal breadcrumb trail.OIDExternalUserAgentIOSignoring theBOOLturns aURLMismatchinto an infinite hang, and reports every failure as "user cancelled" — whichSignInScreensuppressed without an alert. Three layers of silence over one character.