From e444177292a0121f878bbbe86ec3081fb6b81908 Mon Sep 17 00:00:00 2001 From: Samuel Newman Date: Tue, 4 Aug 2026 20:58:55 +0300 Subject: [PATCH] address review feedback across the stack Co-Authored-By: Claude Fable 5 --- src/lib/lexClient.ts | 7 ++++- src/screens/Login/LoginForm.tsx | 4 ++- src/screens/Signup/state.ts | 5 ++-- src/state/queries/profile.ts | 14 +++++----- src/state/session/index.tsx | 26 ++++++++++++++++++- src/state/session/session-core.ts | 13 +++++++--- src/state/session/session-data.ts | 43 ++++++++++++++++++++++++++----- 7 files changed, 91 insertions(+), 21 deletions(-) diff --git a/src/lib/lexClient.ts b/src/lib/lexClient.ts index 2a865d763c..568f9c142f 100644 --- a/src/lib/lexClient.ts +++ b/src/lib/lexClient.ts @@ -40,7 +40,12 @@ export function createLexClient( * Requests use PLAIN `fetch`, not `networkAwareFetch`: the host is untrusted * input, and a typo'd or dead service must not be reported as the app losing * network reachability. + * + * `appLabelers: null` suppresses the global `Client.appLabelers` static: these + * are `com.atproto.server` calls to a host the user typed, which have no use + * for moderation labels, and the header would disclose the app's configured + * moderation authorities to an arbitrary third-party server. */ export function createServiceClient(service: string): Client { - return createLexClient({service}) + return createLexClient({service}, {appLabelers: null}) } diff --git a/src/screens/Login/LoginForm.tsx b/src/screens/Login/LoginForm.tsx index dfd6ccc227..4f34d3afaf 100644 --- a/src/screens/Login/LoginForm.tsx +++ b/src/screens/Login/LoginForm.tsx @@ -169,7 +169,9 @@ export const LoginForm = ({ ) } else { logger.warn('Failed to login', {error: errMsg}) - setError(cleanError(errMsg)) + /* the error object, not its stringification: cleanError only + * extracts the clean server message from a live LexError */ + setError(cleanError(err)) } } } diff --git a/src/screens/Signup/state.ts b/src/screens/Signup/state.ts index b012f34427..c2d8684d42 100644 --- a/src/screens/Signup/state.ts +++ b/src/screens/Signup/state.ts @@ -354,7 +354,6 @@ export function useSubmitSignup() { onboardingDispatch({type: 'start'}) } catch (err) { const e = err as Error - let errMsg = e.toString() if ( matchXrpcError(e, com.atproto.server.createAccount) === 'InvalidInviteCode' @@ -368,7 +367,9 @@ export function useSubmitSignup() { return } - const error = cleanError(errMsg) + /* the error object, not its stringification: cleanError only extracts + * the clean server message from a live LexError */ + const error = cleanError(e) const isHandleError = error.toLowerCase().includes('handle') dispatch({type: 'setIsLoading', value: false}) diff --git a/src/state/queries/profile.ts b/src/state/queries/profile.ts index 190f4a3998..fb3f467743 100644 --- a/src/state/queries/profile.ts +++ b/src/state/queries/profile.ts @@ -206,21 +206,21 @@ export function useProfileUpdateMutation() { appviewClient, profile.did, checkCommitted || - (profile => { + (fresh => { if (typeof newUserAvatar !== 'undefined') { - if (newUserAvatar === null && profile.avatar) { + if (newUserAvatar === null && fresh.avatar) { // url hasn't cleared yet return false - } else if (profile.avatar === profile.avatar) { + } else if (fresh.avatar === profile.avatar) { // url hasn't changed yet return false } } if (typeof newUserBanner !== 'undefined') { - if (newUserBanner === null && profile.banner) { + if (newUserBanner === null && fresh.banner) { // url hasn't cleared yet return false - } else if (profile.banner === profile.banner) { + } else if (fresh.banner === profile.banner) { // url hasn't changed yet return false } @@ -229,8 +229,8 @@ export function useProfileUpdateMutation() { return true } return ( - profile.displayName === updates.displayName && - profile.description === updates.description + fresh.displayName === updates.displayName && + fresh.description === updates.description ) }), ) diff --git a/src/state/session/index.tsx b/src/state/session/index.tsx index d3045932d3..7cce22fff7 100644 --- a/src/state/session/index.tsx +++ b/src/state/session/index.tsx @@ -157,7 +157,19 @@ export function Provider({children}: React.PropsWithChildren<{}>) { sessionEvent: AtpSessionEvent, sessionData?: SessionData, ) => { - if (sessionEvent === 'update' && sessionData) { + /* + * Only the live bundle may reset the expiry-rescue bookkeeping: a stale + * bundle's late update would otherwise clear the failed-generation set + * that bounds the rescue loop. (Its dispatch below is separately dropped + * by the reducer's identity guard.) + */ + if ( + sessionEvent === 'update' && + sessionData && + (store.getState().currentBundleState.bundle as unknown as + | SessionBundle + | PublicSessionBundle) === bundle + ) { failedExpiryTokensRef.current.get(accountDid)?.clear() } @@ -505,6 +517,18 @@ export function Provider({children}: React.PropsWithChildren<{}>) { if (after === before) { throw new Error('Failed to refresh session') } + /* + * The user may have logged out or switched accounts while the refresh was + * in flight. Reporting success then would run the caller's success path + * (dialogs closing, success toasts, SignupQueued advancing) against an + * account that is no longer active, so a stale bundle rejects instead. + */ + if ( + (store.getState().currentBundleState.bundle as unknown) !== + (bundle as unknown) + ) { + throw new Error('The session changed while it was being refreshed') + } /* * The session's `onUpdated` hook dispatches the new tokens into the store, * but that lands a render away; this snapshot exposes them immediately. diff --git a/src/state/session/session-core.ts b/src/state/session/session-core.ts index 6269cc7f29..cce2a0e2d5 100644 --- a/src/state/session/session-core.ts +++ b/src/state/session/session-core.ts @@ -95,9 +95,16 @@ export function buildBundle( session: PasswordSession, storedPdsUrl?: string, ): SessionBundle { - const agent = storedPdsUrl - ? routeSessionToPds(session, storedPdsUrl) - : session + /* + * The stored url is persisted data and may be malformed (legacy writes, + * corruption). `routeSessionToPds` feeds it to `new URL()` on every request, + * so an invalid value would throw from every client call; discard it here + * and let the session route against its own service instead. + */ + const agent = + storedPdsUrl && URL.canParse(storedPdsUrl) + ? routeSessionToPds(session, storedPdsUrl) + : session return { session, appviewClient: buildAppviewClient(agent), diff --git a/src/state/session/session-data.ts b/src/state/session/session-data.ts index 711f6ffe9b..31bcc098ce 100644 --- a/src/state/session/session-data.ts +++ b/src/state/session/session-data.ts @@ -1,4 +1,3 @@ -import {getPdsEndpoint, isValidDidDoc} from '@atproto/common-web' import {type SessionData} from '@atproto/lex-password-session' import {jwtDecode} from 'jwt-decode' @@ -7,6 +6,42 @@ import {isJwtExpired} from '#/lib/jwt' import {hasProp} from '#/lib/type-guards' import {type SessionAccount} from './types' +/** + * The PDS endpoint declared by a DID document, or `undefined`. + * + * This deliberately mirrors `extractPdsUrl` in `@atproto/lex-password-session`, + * the predicate `PasswordSession` uses to route its own requests: the first + * service entry whose `id` ends with `#atproto_pds`, taking its + * `serviceEndpoint` if it parses as a URL. A stricter predicate here (schema + * validation, `type` checks) would let the PERSISTED `pdsUrl` disagree with + * the host the live session actually routes to, so the next cold start would + * seed the wrong endpoint. + */ +function extractPdsUrl(didDoc: SessionData['didDoc']): string | undefined { + const services = prop(didDoc, 'service') + if (!Array.isArray(services)) { + return undefined + } + const pds = services.find(service => { + const id = prop(service, 'id') + return typeof id === 'string' && id.endsWith('#atproto_pds') + }) + const endpoint = prop(pds, 'serviceEndpoint') + return typeof endpoint === 'string' && URL.canParse(endpoint) + ? endpoint + : undefined +} + +/** + * Read a property off an unknown value the way JS optional chaining would, + * without narrowing assumptions about the shape of a `LexMap`. + */ +function prop(value: unknown, key: string): unknown { + return typeof value === 'object' && value !== null + ? (value as Record)[key] + : undefined +} + /** Whether an access token was issued for a queued (waitlisted) signup. */ export function isSignupQueued(accessJwt: string | undefined) { if (accessJwt) { @@ -40,11 +75,7 @@ export function sessionDataToSessionAccount( return undefined } const normalizedService = new URL(service).toString() - const didDocPdsUrl = - session.didDoc && isValidDidDoc(session.didDoc) - ? getPdsEndpoint(session.didDoc) - : undefined - const pdsUrl = didDocPdsUrl ?? storedPdsUrl + const pdsUrl = extractPdsUrl(session.didDoc) ?? storedPdsUrl return { service: normalizedService, did: session.did,