fix(client): profile picture upload + display on android (#253, #256) — second pass #260

Merged

james merged 1 commit from 253-256-android-profile-picture-2 into main 2026-06-23 17:27:39 +00:00

Conversation 1 Commits 1 Files changed 6 +516 -26

james commented 2026-06-23 17:22:04 +00:00

Owner

Copy link

Background

PR #257 shipped first-pass fixes for #253 (upload fails on Android) and #256 (uploaded picture doesn't render on Android). Both symptoms persisted post-merge. This PR digs into the actual root causes.

What was actually broken

Upload (#253) — request.arrayBuffer() throws on FormData under RN's whatwg-fetch

PR #257 made buildPictureFormData() branch on Platform.OS and append { uri, name, type } on native. That part was right. The off-origin URL-rewriter middleware in apps/client/lib/apiClient.ts is what killed the upload at runtime.

Path on Android:

1. The hook calls client.POST("/api/profile/picture", { body: form, bodySerializer: passthrough }).
2. openapi-fetch builds a Request whose body is the FormData. RN's whatwg-fetch stores it as _bodyFormData (the live object, not bytes).
3. The rewriter's onRequest fires, sees the URL needs the runtime server URL spliced in, and calls extractRequestBody(request).
4. extractRequestBody sees multipart/form-data in the Content-Type and calls request.arrayBuffer().
5. whatwg-fetch's arrayBuffer() falls back to this.blob(), which throws "could not read FormData body as blob" when _bodyFormData is set. (fetch.js#L283)
6. The hook's try catches it and the screen shows the generic "Upload failed. Try again."

The first-pass tests in apiClientBody.test.ts and picture.test.ts passed because node's Request (undici) does serialise FormData into a readable stream — the bug only manifests on RN's polyfill. PR #257 never had a test that would have caught the platform-specific behaviour.

Fix: detect multipart in the rewriter and skip body extraction. Clone via new Request(next, request), which uses whatwg-fetch's documented pattern (_bodyInit is carried over unchanged). RN's fetch then sees a Request whose body is still the live FormData with the { uri, name, type } descriptor, and streams the file off disk at send time. The Authorization header is re-applied explicitly after the clone — belt-and-braces against the same RN Headers-iteration quirk the JSON-rebuild branch has long-standing workaround comments about.

Display (#256) — token cache is empty at first paint; nothing re-renders when it fills

PR #257 added getCachedAccessToken() (sync read of an in-memory mirror of SecureStore) and made the Avatar feed it to getProfilePictureSource(). The helper returns null when the bearer is missing, the consumer falls back to initials — correct so far.

The cache is only populated when getAccessToken() is awaited. That happens on every authenticated request via getAuthHeader(). On a cold launch the Avatar mounts as part of the Profile screen render; the underlying useProfile() query fires getAccessToken(), but the Avatar's render and the SecureStore round-trip race. The cache loses, the source builder returns null, the Avatar paints initials.

Worse: the Avatar's useEffect depends only on [pictureUploadedAt, offOrigin]. Once the cache fills, nothing in the dep array changes, so the Avatar never re-resolves and stays on initials until the screen unmounts.

Fix: two-part.

1. loadAccessToken() is called from the root layout's mount effect — same shape as loadServerUrl(). The bearer cache is warm before any screen mounts.
2. subscribeAccessToken() lets render-path consumers re-render on cache transitions. The Avatar subscribes (off-origin only — the PWA's source never depends on the bearer) so it re-resolves the instant the token arrives. Both write paths (setAccessToken, setTokens, clearTokens) and the first successful getAccessToken() read notify subscribers.

Why PR #257's tests didn't catch this

• The body-extraction test in apiClientBody.test.ts ran in node, where Request's FormData serialisation works. RN's whatwg-fetch was never exercised.
• There was no integration test exercising the full URL-rewriter middleware with a FormData body. The first PR added unit tests for buildPictureFormData() and getProfilePictureSource() but not the rewriter — which is the actual broken layer.
• For the display: pictureSrc.test.ts covers the helper, but it's a pure function. The "cache empty at first paint" failure is a state-machine bug between the storage cache and the React component, not a helper bug.

New tests

• apps/client/tests/apiClient.test.ts: drives a real POST through apiClient with FormData and a JSON body. Asserts:
  • URL is rewritten with the runtime base in front.
  • Multipart content-type / boundary is preserved (re-serialisation would have invalidated the boundary).
  • Picture bytes survive byte-for-byte (test uses 0xFE / 0xFF, the bytes a UTF-8 round-trip would mangle).
  • Authorization header is present on both branches.
  • JSON branch still rebuilds the body correctly.
• apps/client/tests/storage.test.ts: covers loadAccessToken(), the new subscribeAccessToken() pub/sub, and verifies unsubscribe works and PWA stays a no-op.

Verification

Local gates all pass on this branch:

• pnpm -F @carol/client typecheck ✔
• pnpm -F @carol/client lint ✔
• pnpm -F @carol/client test — 90 tests, including the 7 new ones ✔
• pnpm -F @carol/client export:web ✔
• pnpm -F @carol/api-client typecheck / lint / test / check ✔
• pnpm -F @carol/api typecheck / lint / test ✔

I was not able to run the fix on a real Android device from this worktree — that requires the Android build pipeline and a connected device or emulator, which isn't available in this environment. Confirmation should be:

1. Build the APK via pnpm -F @carol/client build:android, sideload, point at a Carol instance.
2. Profile → "Change picture" → pick an image. Expect: upload succeeds, server returns 201, avatar replaces initials with the picked image.
3. Force-stop, re-launch. Expect: avatar paints from initials → picture as the access-token cache warms (sub-second on a warm SecureStore).

Surprises

• RN's whatwg-fetch never reads FormData via the body-reader methods. It stores _bodyFormData and lets the native networking layer serialise at send time. Any middleware that tries request.arrayBuffer() / request.text() / request.blob() on a FormData-bodied Request throws. This is the spec-defined behaviour for FormData bodies in whatwg-fetch, but it's easy to miss if you only test against node.
• <Image source.headers> works on Android — RN's RCTImageLoader does forward the headers map to OkHttp via getSizeWithHeaders. The display fix's source-building logic was correct; the cache-timing was the only issue.
• The token cache being purely demand-loaded was a latent bug surfaced by the new sync read. Before #256, nothing ever read the bearer synchronously. Adding getCachedAccessToken() made the race visible.

Reopens #253.
Reopens #256.

## Background PR [#257](https://forge.wynning.tech/james/carol/pulls/257) shipped first-pass fixes for [#253](https://forge.wynning.tech/james/carol/issues/253) (upload fails on Android) and [#256](https://forge.wynning.tech/james/carol/issues/256) (uploaded picture doesn't render on Android). Both symptoms persisted post-merge. This PR digs into the actual root causes. ## What was actually broken ### Upload (#253) — `request.arrayBuffer()` throws on FormData under RN's whatwg-fetch PR #257 made `buildPictureFormData()` branch on `Platform.OS` and append `{ uri, name, type }` on native. That part was right. **The off-origin URL-rewriter middleware in `apps/client/lib/apiClient.ts` is what killed the upload at runtime.** Path on Android: 1. The hook calls `client.POST("/api/profile/picture", { body: form, bodySerializer: passthrough })`. 2. openapi-fetch builds a `Request` whose body is the FormData. RN's whatwg-fetch stores it as `_bodyFormData` (the live object, not bytes). 3. The rewriter's `onRequest` fires, sees the URL needs the runtime server URL spliced in, and calls `extractRequestBody(request)`. 4. `extractRequestBody` sees `multipart/form-data` in the Content-Type and calls `request.arrayBuffer()`. 5. **whatwg-fetch's `arrayBuffer()` falls back to `this.blob()`, which throws `"could not read FormData body as blob"` when `_bodyFormData` is set.** ([fetch.js#L283](https://github.com/JakeChampion/fetch/blob/v3.6.20/fetch.js#L283)) 6. The hook's `try` catches it and the screen shows the generic "Upload failed. Try again." The first-pass tests in `apiClientBody.test.ts` and `picture.test.ts` passed because node's Request (undici) does serialise FormData into a readable stream — the bug only manifests on RN's polyfill. PR #257 never had a test that would have caught the platform-specific behaviour. **Fix:** detect multipart in the rewriter and skip body extraction. Clone via `new Request(next, request)`, which uses whatwg-fetch's documented pattern (`_bodyInit` is carried over unchanged). RN's fetch then sees a Request whose body is still the live FormData with the `{ uri, name, type }` descriptor, and streams the file off disk at send time. The Authorization header is re-applied explicitly after the clone — belt-and-braces against the same RN Headers-iteration quirk the JSON-rebuild branch has long-standing workaround comments about. ### Display (#256) — token cache is empty at first paint; nothing re-renders when it fills PR #257 added `getCachedAccessToken()` (sync read of an in-memory mirror of SecureStore) and made the Avatar feed it to `getProfilePictureSource()`. The helper returns `null` when the bearer is missing, the consumer falls back to initials — correct so far. **The cache is only populated when `getAccessToken()` is awaited.** That happens on every authenticated request via `getAuthHeader()`. On a cold launch the Avatar mounts as part of the Profile screen render; the underlying `useProfile()` query fires `getAccessToken()`, but the Avatar's render and the SecureStore round-trip race. The cache loses, the source builder returns null, the Avatar paints initials. Worse: the Avatar's `useEffect` depends only on `[pictureUploadedAt, offOrigin]`. Once the cache fills, **nothing in the dep array changes**, so the Avatar never re-resolves and stays on initials until the screen unmounts. **Fix:** two-part. 1. `loadAccessToken()` is called from the root layout's mount effect — same shape as `loadServerUrl()`. The bearer cache is warm before any screen mounts. 2. `subscribeAccessToken()` lets render-path consumers re-render on cache transitions. The Avatar subscribes (off-origin only — the PWA's source never depends on the bearer) so it re-resolves the instant the token arrives. Both write paths (`setAccessToken`, `setTokens`, `clearTokens`) and the first successful `getAccessToken()` read notify subscribers. ## Why PR #257's tests didn't catch this - The body-extraction test in `apiClientBody.test.ts` ran in node, where Request's FormData serialisation works. RN's whatwg-fetch was never exercised. - There was no integration test exercising the full URL-rewriter middleware with a FormData body. The first PR added unit tests for `buildPictureFormData()` and `getProfilePictureSource()` but not the rewriter — which is the actual broken layer. - For the display: `pictureSrc.test.ts` covers the helper, but it's a pure function. The "cache empty at first paint" failure is a state-machine bug between the storage cache and the React component, not a helper bug. ## New tests - `apps/client/tests/apiClient.test.ts`: drives a real POST through `apiClient` with FormData and a JSON body. Asserts: - URL is rewritten with the runtime base in front. - Multipart content-type / boundary is preserved (re-serialisation would have invalidated the boundary). - Picture bytes survive byte-for-byte (test uses 0xFE / 0xFF, the bytes a UTF-8 round-trip would mangle). - Authorization header is present on both branches. - JSON branch still rebuilds the body correctly. - `apps/client/tests/storage.test.ts`: covers `loadAccessToken()`, the new `subscribeAccessToken()` pub/sub, and verifies unsubscribe works and PWA stays a no-op. ## Verification Local gates all pass on this branch: - `pnpm -F @carol/client typecheck` ✔ - `pnpm -F @carol/client lint` ✔ - `pnpm -F @carol/client test` — 90 tests, including the 7 new ones ✔ - `pnpm -F @carol/client export:web` ✔ - `pnpm -F @carol/api-client typecheck` / `lint` / `test` / `check` ✔ - `pnpm -F @carol/api typecheck` / `lint` / `test` ✔ I was **not able to run the fix on a real Android device** from this worktree — that requires the Android build pipeline and a connected device or emulator, which isn't available in this environment. Confirmation should be: 1. Build the APK via `pnpm -F @carol/client build:android`, sideload, point at a Carol instance. 2. Profile → "Change picture" → pick an image. Expect: upload succeeds, server returns 201, avatar replaces initials with the picked image. 3. Force-stop, re-launch. Expect: avatar paints from initials → picture as the access-token cache warms (sub-second on a warm SecureStore). ## Surprises - **RN's whatwg-fetch never reads FormData via the body-reader methods.** It stores `_bodyFormData` and lets the native networking layer serialise at send time. Any middleware that tries `request.arrayBuffer()` / `request.text()` / `request.blob()` on a FormData-bodied Request throws. This is the spec-defined behaviour for FormData bodies in whatwg-fetch, but it's easy to miss if you only test against node. - **`<Image source.headers>` works on Android** — RN's `RCTImageLoader` does forward the headers map to OkHttp via `getSizeWithHeaders`. The display fix's source-building logic was correct; the cache-timing was the only issue. - **The token cache being purely demand-loaded was a latent bug surfaced by the new sync read.** Before #256, nothing ever read the bearer synchronously. Adding `getCachedAccessToken()` made the race visible. Reopens #253. Reopens #256.

james added 1 commit 2026-06-23 17:22:05 +00:00

fix(client): profile picture upload + display on android (#253, #256) — second pass ... 7c4fde06ab

The first attempt in PR #257 was based on incomplete hypotheses. The
upload's RN-shaped FormData was fine; the URL-rewriter middleware
that re-reads the body via `request.arrayBuffer()` was the actual
killer — RN's whatwg-fetch polyfill throws "could not read FormData
body as blob" on that call, so the upload failed before reaching the
network. Skip body extraction for multipart bodies and clone via
`new Request(next, request)`, which lifts the FormData (and any
`{ uri, name, type }` file descriptor) through unchanged.

The display fix wasn't enough either: the avatar reads the bearer
synchronously from a cache that's only filled when an authenticated
request resolves. On a cold launch the avatar mounts before any
request finishes; the cache returns null, the source builder returns
null, the avatar paints initials, and nothing in its dependency
array changes when the token later arrives. Eager-preload the cache
at app boot via `loadAccessToken()` and let the avatar subscribe to
cache transitions so it re-renders the moment the bearer is ready.

Both bugs are pinned by new tests:
- apiClient.test.ts asserts the FormData survives the rewriter
  byte-for-byte, including the Authorization header that
  authMiddleware sets just above.
- storage.test.ts asserts the new subscribe path fires on every
  write and that `loadAccessToken()` warms the cache from
  SecureStore.

Reopens #253.
Reopens #256.

forgejo-actions commented 2026-06-23 17:26:26 +00:00

Copy link

📊 Test coverage

Patch coverage: no testable lines changed.

Overall (app/, lib/, db/, excluding UI per ADR-0019):

Metric	Value	Soft target
Lines	82.9% ✅	≥ 50%
Branches	75.6% ✅	≥ 75%
Functions	91.8%	informational

Soft thresholds per ADR-0019. Coverage is informational and does not block merge.

<!-- coverage-comment --> ## 📊 Test coverage **Patch coverage:** no testable lines changed. **Overall** (`app/`, `lib/`, `db/`, excluding UI per ADR-0019): | Metric | Value | Soft target | |---|---|---| | Lines | 82.9% ✅ | ≥ 50% | | Branches | 75.6% ✅ | ≥ 75% | | Functions | 91.8% | informational | Soft thresholds per [ADR-0019](docs/adr/0019-coverage-soft-targets.md). Coverage is informational and does not block merge.

james merged commit 25df2822ee into main 2026-06-23 17:27:39 +00:00

james deleted branch 253-256-android-profile-picture-2 2026-06-23 17:27:39 +00:00

james referenced this pull request from a commit 2026-06-23 17:27:40 +00:00

Merge pull request 'fix(client): profile picture upload + display on android (#253, #256) — second pass' (#260) from 253-256-android-profile-picture-2 into main

james referenced this pull request from a commit 2026-06-23 18:33:47 +00:00

fix(client): detect FormData via RN polyfill private field, not just Content-Type

james referenced this pull request 2026-06-23 18:34:12 +00:00

fix(client): profile picture upload + display on android (#253, #256) #263

james referenced this pull request from a commit 2026-06-23 19:07:32 +00:00

fix(client): strip wrong Content-Type on multipart so RN regenerates it

james referenced this pull request from a commit 2026-06-23 19:30:31 +00:00

fix(client): bypass URL rewriter for picture upload (#253, fourth attempt)

james referenced this pull request from a commit 2026-06-23 20:11:00 +00:00

fix(client): switch Avatar to expo-image so source.headers reach the network (#256)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

area:auth

Authentication and authorization

  

area:ci

Forgejo Actions, build, test, security, release

  

area:db

Database abstraction, migrations, persistence

  

area:infra

Containerization, deployment, foundational tooling

  

area:native

React Native app (Android + Linux Flatpak)

  

area:pwa

PWA / web frontend

  

area:service

Backend service, API routes, persistence

  

epic

High-level tracker; links several feature tickets

  

feature

A user-facing feature

  

foundation

Foundational work that other tickets build on

No labels

area:auth

area:ci

area:db

area:infra

area:native

area:pwa

area:service

epic

feature

foundation

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

james/carol!260

No description provided.