docs: document mobile sdk APIs using JSDoc #922
Conversation
WalkthroughIntroduces a native event bridge with cached per-module emitters and public addListener/removeListener, expands public types (HTTP, MRZ, adapters, events, WebSocket), updates SelfClient to support on/emit and revised generateProof with timeout handling, adjusts client config typing, and updates tests to new HTTP response shape. Several files add documentation comments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App code
participant Client as SelfClient
participant Adapters as Adapters (logger, clock, scanner, crypto, network)
rect rgb(240,248,255)
note over Client: Event subscription
App->>Client: on('error', handler)
Client-->>App: Unsubscribe
end
rect rgb(245,245,255)
note over Client: Proof generation with timeout
App->>Client: generateProof(req, { timeoutMs, signal, onProgress })
Client->>Adapters: validate crypto/network presence
Client->>Adapters: clock.sleep(timeoutMs, signal)
alt timeout fires
Client->>Client: emit('error', sdkError('timeout', ...))
Client-->>App: Promise resolves ProofHandle (pending/stub)
else completes early
Client-->>App: ProofHandle/result()
end
end
sequenceDiagram
autonumber
participant JS as JS Bridge
participant Native as Native Module
participant E as Emitter Cache
rect rgb(245,255,245)
note over JS: addListener
JS->>E: getEmitter(moduleName)
alt Native has addListener/removeListeners
E->>E: cache new NativeEventEmitter(Native)
else fallback
E->>E: cache shared NativeEventEmitter
note right of E: dev-only warning
end
JS->>E: emitter.addListener(eventName, handler)
JS-->>JS: return unsubscribe
end
rect rgb(255,248,240)
note over JS: removeListener
JS->>E: lookup emitter
JS->>E: emitter?.removeListener?.(eventName, handler)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/mobile-sdk-alpha/src/types/public.ts (1)
225-231: Return Unsubscribe and avoid any in WS listener registration to prevent leaks and improve safetyCurrent callbacks return void, giving no way to detach listeners and risking leaks. Also, any defeats type safety.
export interface WsConn { send: (data: string | ArrayBufferView | ArrayBuffer) => void; close: () => void; - onMessage: (cb: (data: any) => void) => void; - onError: (cb: (e: any) => void) => void; - onClose: (cb: () => void) => void; + onMessage: (cb: (data: unknown) => void) => Unsubscribe; + onError: (cb: (e: unknown) => void) => Unsubscribe; + onClose: (cb: () => void) => Unsubscribe; }Aligns with SelfClient.on API consistency and supports proper cleanup.
packages/mobile-sdk-alpha/src/config/merge.ts (1)
3-20: Critical security flaw confirmed: undefined values can override required defaultsAfter analyzing the codebase, I've verified that this is indeed a critical security and runtime stability issue. The
Configinterface defines all properties as optional (endpoints?,timeouts?, etc.), butmergeConfigcurrently acceptsConfiginstead ofPartial<Config>as documented, and more critically, it doesn't protect against undefined value overwrites.Confirmed vulnerabilities:
- Consumer can pass
{ endpoints: { api: undefined } }which overwrites the required API endpoint with undefined- This breaks the
Required<Config>return type contract at runtime- Tests show the function is used in
createSelfClientwhere invalid configs could compromise SDK functionality- The JSDoc clearly states "Partial configuration" but signature accepts full
ConfigCritical locations requiring fixes:
packages/mobile-sdk-alpha/src/config/merge.ts:12- Function signature must usePartial<Config>packages/mobile-sdk-alpha/src/config/merge.ts:16-19- Shallow merges must filter undefined valuesApply this diff:
-import type { Config } from '../types/public'; +import type { Config } from '../types/public'; + +function omitUndefined<T extends Record<string, unknown>>(obj?: Partial<T>): Partial<T> { + if (!obj) return {}; + return Object.fromEntries(Object.entries(obj).filter(([, v]) => v !== undefined)) as Partial<T>; +} -export function mergeConfig(base: Required<Config>, override: Config): Required<Config> { +export function mergeConfig(base: Required<Config>, override: Partial<Config>): Required<Config> { return { ...base, ...override, - endpoints: { ...base.endpoints, ...(override.endpoints ?? {}) }, - timeouts: { ...base.timeouts, ...(override.timeouts ?? {}) }, - features: { ...base.features, ...(override.features ?? {}) }, - tlsPinning: { ...base.tlsPinning, ...(override.tlsPinning ?? {}) }, + endpoints: { ...base.endpoints, ...omitUndefined<typeof base.endpoints>(override.endpoints) }, + timeouts: { ...base.timeouts, ...omitUndefined<typeof base.timeouts>(override.timeouts) }, + features: { ...base.features, ...omitUndefined<typeof base.features>(override.features) }, + tlsPinning: { ...base.tlsPinning, ...omitUndefined<typeof base.tlsPinning>(override.tlsPinning) }, }; }packages/mobile-sdk-alpha/src/bridge/nativeEvents.native.ts (2)
8-30: Fail fast (or explicitly gated) when native module shape is missing to avoid silent event loss in productionFalling back to a module-less
NativeEventEmitter()will often not deliver events on iOS, silently breaking features in prod while only logging in dev. Prefer hard-failing in prod (or gate with an explicit allowlist) so integration issues are surfaced early.Apply this diff:
const hasExpectedShape = !!mod && typeof (mod as any).addListener === 'function' && typeof (mod as any).removeListeners === 'function'; if (!hasExpectedShape) { - if (typeof __DEV__ !== 'undefined' && __DEV__) { - // eslint-disable-next-line no-console - console.warn( - `[nativeEvents] Native module '${moduleName}' is missing or does not implement addListener/removeListeners; ` + - 'falling back to a shared emitter. Events may not fire on iOS.', - ); - } - emitters[moduleName] = new NativeEventEmitter(); + const msg = + `[nativeEvents] Native module '${moduleName}' is missing or does not implement addListener/removeListeners. ` + + 'Events may not fire on iOS. Configure the native bridge correctly.'; + if (typeof __DEV__ !== 'undefined' && __DEV__) { + // eslint-disable-next-line no-console + console.warn(msg); + // As a dev-only convenience, create a shared emitter to keep flows unblocked. + emitters[moduleName] = new NativeEventEmitter(); + } else { + throw new Error(msg); + } } else { emitters[moduleName] = new NativeEventEmitter(mod); }
41-49: Track subscriptions to make removeListener reliable and prevent leaksRelying on
emitter.removeListener?.(...)may no-op on newer RN builds and will leak handlers if callers don’t keep the unsubscribe closure. Track subscriptions soremoveListenercan deterministically remove by (module, event, handler).Add a subscription registry near the top (after
emitters):// Place after `const emitters...` const subscriptions = new Map< string, // moduleName Map<string, Map<(...args: any[]) => void, NativeEventSubscription>> >();Update addListener to record subs:
export const addListener: NativeEventBridge['addListener'] = (moduleName, eventName, handler) => { const emitter = getEmitter(moduleName); const sub: NativeEventSubscription = emitter.addListener(eventName, handler); - return () => sub.remove(); + // Track subscription + let byEvent = subscriptions.get(moduleName); + if (!byEvent) { + byEvent = new Map(); + subscriptions.set(moduleName, byEvent); + } + let byHandler = byEvent.get(eventName); + if (!byHandler) { + byHandler = new Map(); + byEvent.set(eventName, byHandler); + } + byHandler.set(handler, sub); + // Unsubscribe closure also cleans the registry + return () => { + sub.remove(); + byHandler?.delete(handler); + }; };Make removeListener use the registry:
export const removeListener: NativeEventBridge['removeListener'] = (moduleName, eventName, handler) => { - const emitter = emitters[moduleName]; - if (emitter) { - (emitter as any).removeListener?.(eventName, handler); - } + const sub = subscriptions.get(moduleName)?.get(eventName)?.get(handler); + sub?.remove(); + subscriptions.get(moduleName)?.get(eventName)?.delete(handler); };packages/mobile-sdk-alpha/src/client.ts (1)
119-145: Make proof timeout cancellable and avoid emitting ‘error’ after cancel/successThe current implementation uses a non-cancellable timer and unconditionally emits a timeout error, which can fire even after the operation is cancelled or completed. Refactor
generateProofto use an internalAbortControllerthat you bridge toopts.signal, abort the sleep on cancel, and only emit the error if the timer wasn’t aborted. Verified that:
ClockAdapter.sleep(ms, signal?)accepts anAbortSignaland will resolve early on abort.SDKEventMapincludeserror: Error, sonew Error('timeout')is valid.Affected location:
- packages/mobile-sdk-alpha/src/client.ts, function
generateProof(approx. lines 119–145)Apply this diff:
async function generateProof( _req: ProofRequest, opts: { signal?: AbortSignal; onProgress?: (p: Progress) => void; timeoutMs?: number; } = {}, ): Promise<ProofHandle> { if (!adapters.network) throw notImplemented('network'); if (!adapters.crypto) throw notImplemented('crypto'); const timeoutMs = opts.timeoutMs ?? cfg.timeouts?.proofMs ?? defaultConfig.timeouts.proofMs; - void _adapters.clock.sleep(timeoutMs!, opts.signal).then(() => emit('error', new Error('timeout'))); + const timeoutCtrl = new AbortController(); + // Bridge external cancellation to the internal timer + if (opts.signal) { + if (opts.signal.aborted) timeoutCtrl.abort(); + else opts.signal.addEventListener('abort', () => timeoutCtrl.abort(), { once: true }); + } + void _adapters.clock.sleep(timeoutMs!, timeoutCtrl.signal).then(() => { + if (!timeoutCtrl.signal.aborted) { + emit('error', new Error('timeout')); + } + }); return { id: 'stub', status: 'pending', result: async () => ({ ok: false, reason: 'SELF_ERR_PROOF_STUB' }), - cancel: () => {}, + cancel: () => timeoutCtrl.abort(), }; }
🧹 Nitpick comments (2)
packages/mobile-sdk-alpha/src/errors.ts (1)
28-37: Preserve native error cause chaining by passing options to superModern runtimes support Error(message, { cause }). Currently options are ignored by super(), which can drop native cause chaining and stack context.
Apply:
- constructor(message: string, code: string, category: SdkErrorCategory, retryable = false, options?: ErrorOptions) { - super(message); + constructor(message: string, code: string, category: SdkErrorCategory, retryable = false, options?: ErrorOptions) { + // Passing options is safe: older engines ignore the 2nd arg; modern engines set `cause`. + super(message, options as any); this.name = 'SdkError'; this.code = code; this.category = category; this.retryable = retryable; - if (options?.cause) { - this.cause = options.cause as Error; - } + // Fallback assignment maintains `cause` visibility even if the engine ignores the options. + if (options?.cause) { + this.cause = options.cause as Error; + } }Additionally consider loosening the cause type for flexibility:
- declare cause?: Error; + declare cause?: unknown;packages/mobile-sdk-alpha/src/types/public.ts (1)
14-16: Clarify and constrain TLS pin format to reduce misconfiguration riskOpen-ended pins: string[] risks format drift (SHA-256 raw vs base64/SPKI/etc.). Consider a structured type to enforce algorithm and encoding, and optional enforce flag.
Example:
- tlsPinning?: { enabled: boolean; pins?: string[] }; + tlsPinning?: { + enabled: boolean; + enforce?: boolean; + pins?: Array<{ alg: 'sha256'; encoding: 'base64' | 'hex'; value: string }>; + };At minimum, document the expected pin format explicitly in the JSDoc to avoid runtime pin validation failures.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
packages/mobile-sdk-alpha/src/adapters/index.ts(1 hunks)packages/mobile-sdk-alpha/src/bridge/nativeEvents.native.ts(2 hunks)packages/mobile-sdk-alpha/src/bridge/nativeEvents.ts(1 hunks)packages/mobile-sdk-alpha/src/browser.ts(1 hunks)packages/mobile-sdk-alpha/src/client.ts(3 hunks)packages/mobile-sdk-alpha/src/config/defaults.ts(1 hunks)packages/mobile-sdk-alpha/src/config/merge.ts(1 hunks)packages/mobile-sdk-alpha/src/errors.ts(2 hunks)packages/mobile-sdk-alpha/src/index.ts(1 hunks)packages/mobile-sdk-alpha/src/types/public.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/mobile-sdk-alpha/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit Configuration File
packages/mobile-sdk-alpha/**/*.{ts,tsx,js,jsx}: Review alpha mobile SDK code for:
- API consistency with core SDK
- Platform-neutral abstractions
- Performance considerations
- Clear experimental notes or TODOs
Files:
packages/mobile-sdk-alpha/src/index.tspackages/mobile-sdk-alpha/src/browser.tspackages/mobile-sdk-alpha/src/config/defaults.tspackages/mobile-sdk-alpha/src/errors.tspackages/mobile-sdk-alpha/src/bridge/nativeEvents.native.tspackages/mobile-sdk-alpha/src/adapters/index.tspackages/mobile-sdk-alpha/src/config/merge.tspackages/mobile-sdk-alpha/src/bridge/nativeEvents.tspackages/mobile-sdk-alpha/src/client.tspackages/mobile-sdk-alpha/src/types/public.ts
🧠 Learnings (1)
📚 Learning: 2025-08-01T17:11:39.232Z
Learnt from: transphorm
PR: selfxyz/self#823
File: common/package.json:21-60
Timestamp: 2025-08-01T17:11:39.232Z
Learning: Explicit export mappings in package.json exports field provide better tree shaking than wildcard patterns because they enable easier static analysis by bundlers, allowing for more precise dependency graph understanding and better dead code elimination.
Applied to files:
packages/mobile-sdk-alpha/src/browser.ts
🔇 Additional comments (8)
packages/mobile-sdk-alpha/src/browser.ts (2)
1-5: Conditional Exports for Browser ConfirmedThe
package.jsoninpackages/mobile-sdk-alphacorrectly routes web builds to your browser entry point:
- Root export (
".") includes"browser": "./dist/esm/browser.js".- There’s also an explicit
"./browser"export pointing to./dist/esm/browser.js(and its CJS counterpart).No further changes needed here.
46-49: ✅ createSelfClient web-safe re-export
No direct react-native or native-module imports were found in packages/mobile-sdk-alpha/src/client.ts. Re-exporting createSelfClient from the browser entry is safe.packages/mobile-sdk-alpha/src/index.ts (1)
1-4: Conditional exports correctly configuredYour package.json already defines explicit
react-nativeandbrowserconditions under"exports": { ".": { … } }, pointing to separate entry-points (./dist/esm/index.jsvs./dist/esm/browser.js). No further changes needed here.packages/mobile-sdk-alpha/src/adapters/index.ts (1)
1-5: Docs add helpful context; adapter boundaries look cleanClear guidance for implementers. No runtime changes. LGTM.
packages/mobile-sdk-alpha/src/config/defaults.ts (1)
3-12: LGTM — clear baseline config docsThe defaults read clean and the added docs make intended override behavior obvious.
packages/mobile-sdk-alpha/src/bridge/nativeEvents.native.ts (1)
32-39: LGTM — subscription API returns a solid unsubscribe closureReturning
() => sub.remove()is the safest path across RN versions.packages/mobile-sdk-alpha/src/bridge/nativeEvents.ts (1)
3-6: LGTM — clean platform-neutral bridge surface for eventsThe EventHandler alias, interface, and no-op defaults for non-native environments look consistent and safe.
Also applies to: 12-26, 28-33, 34-37
packages/mobile-sdk-alpha/src/client.ts (1)
57-69: LGTM — internal event hub and initial stubs are safe and side-effect contained
- Using Set per event and Array.from during emit avoids mutation hazards.
- Errors in listeners are contained and logged via the logger adapter.
- Scan/validate/registration stubs are clearly delineated and side-effect free.
Also applies to: 71-87, 89-97, 99-107, 109-117
|
going to merge the updated error classes first before this pull request - #916 |
transphorm
force-pushed
the
codex/add-jsdoc-comments-to-/src-files
branch
from
76c836c to
2eaa746
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
transphorm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/mobile-sdk-alpha/src/bridge/nativeEvents.native.ts (1)
44-49: Replace deprecatedremoveListenerwith subscription-based removalReact Native ≥0.65 (including 0.73+) no longer supports
NativeEventEmitter.removeListener(event, handler). Relying on it is effectively a no-op, leading to leaked listeners and unexpected callbacks. Instead, remove the exactEmitterSubscriptionreturned byaddListener.Locations to update:
- packages/mobile-sdk-alpha/src/bridge/nativeEvents.native.ts (around lines 44–49)
Proposed change:
export const removeListener: NativeEventBridge['removeListener'] = (moduleName, eventName, handler) => { - const emitter = emitters[moduleName]; - if (emitter) { - (emitter as any).removeListener?.(eventName, handler); - } + // Prefer removing the exact subscription we stored in addListener + const sub = subscriptions[moduleName]?.[eventName]?.get(handler as any); + if (sub) { + sub.remove(); + subscriptions[moduleName]![eventName]!.delete(handler as any); + return; + } + // Fallback for legacy RN versions + const emitter = emitters[moduleName]; + if (emitter) { + (emitter as any).removeListener?.(eventName, handler); + } };packages/mobile-sdk-alpha/tests/client.test.ts (1)
62-79: Avoid monkey-patching Map.prototype in tests; use the public emit APIPatching built-ins makes tests brittle and order-dependent. Leverage client.emit to drive the event flow.
Apply:
- it('emits and unsubscribes events', () => { - const client = createSelfClient({ config: {}, adapters: { scanner, network, crypto } }); - const cb = vi.fn(); - const originalSet = Map.prototype.set; - let eventSet: Set<(p: any) => void> | undefined; - Map.prototype.set = function (key: any, value: any) { - if (key === 'progress') eventSet = value; - return originalSet.call(this, key, value); - }; - const unsub = client.on('progress', cb); - Map.prototype.set = originalSet; - - eventSet?.forEach(fn => fn({ step: 'one' })); - expect(cb).toHaveBeenCalledWith({ step: 'one' }); - unsub(); - eventSet?.forEach(fn => fn({ step: 'two' })); - expect(cb).toHaveBeenCalledTimes(1); - }); + it('emits and unsubscribes events', () => { + const client = createSelfClient({ config: {}, adapters: { scanner, network, crypto } }); + const cb = vi.fn(); + const unsub = client.on('progress', cb); + client.emit('progress', { step: 'one' }); + expect(cb).toHaveBeenCalledWith({ step: 'one' }); + unsub(); + client.emit('progress', { step: 'two' }); + expect(cb).toHaveBeenCalledTimes(1); + });packages/mobile-sdk-alpha/src/client.ts (2)
28-33: Default clock.sleep ignores AbortSignal; cancellations won’t workThe optionalDefaults implementation drops the signal, so timeouts/cancels in generateProof cannot abort the sleep. Honor AbortSignal to prevent spurious timeout emissions and to support cooperative cancellation.
Apply:
clock: { now: () => Date.now(), - sleep: async (ms: number) => { - await new Promise(r => setTimeout(r, ms)); - }, + sleep: async (ms: number, signal?: AbortSignal) => { + await new Promise<void>((resolve, reject) => { + const t = setTimeout(resolve, ms); + const onAbort = () => { + clearTimeout(t); + const err = new Error('aborted'); + (err as any).name = 'AbortError'; + reject(err); + }; + if (signal) { + if (signal.aborted) return onAbort(); + signal.addEventListener('abort', onAbort, { once: true }); + } + }); + }, },
140-151: Timeout emission is not cancelable; wire it to a controllable signal and swallow abort rejectionsAs written, the timeout will always emit an error after proofMs even if the operation is canceled/finishes early; and aborting sleep may cause an unhandled rejection. Make the timeout abortable and safe.
Apply:
): Promise<ProofHandle> { if (!adapters.network) throw notImplemented('network'); if (!adapters.crypto) throw notImplemented('crypto'); const timeoutMs = opts.timeoutMs ?? cfg.timeouts?.proofMs ?? defaultConfig.timeouts.proofMs; - void _adapters.clock - .sleep(timeoutMs!, opts.signal) - .then(() => emit('error', sdkError('timeout', 'SELF_ERR_PROOF_TIMEOUT', 'proof', true))); + const timeoutCtrl = new AbortController(); + // If caller provided a signal, cascade its abort into our timeout controller + if (opts.signal) { + if (opts.signal.aborted) timeoutCtrl.abort(); + else opts.signal.addEventListener('abort', () => timeoutCtrl.abort(), { once: true }); + } + const _timeout = _adapters.clock + .sleep(timeoutMs!, timeoutCtrl.signal) + .then(() => emit('error', sdkError('timeout', 'SELF_ERR_PROOF_TIMEOUT', 'proof', true))) + .catch(() => { + // Swallow abort rejection from sleep + }); return { id: 'stub', status: 'pending', result: async () => ({ ok: false, reason: 'SELF_ERR_PROOF_STUB' }), - cancel: () => {}, + cancel: () => { + timeoutCtrl.abort(); + }, }; }
♻️ Duplicate comments (3)
packages/mobile-sdk-alpha/src/types/public.ts (2)
6-19: Good move: platform-neutral HTTP surface and adapter signatureReplacing DOM Request/Response with portable RequestLike/RequestInitLike/HttpResponseLike and updating HttpAdapter.fetch accordingly de-couples the API from browser types and simplifies RN/Node portability.
Also applies to: 45-47
150-154: Event error payload typed as SdkError — thumbs upTyping SDKEventMap.error as SdkError preserves code/category/retryable for consumers.
packages/mobile-sdk-alpha/src/client.ts (1)
39-53: Accepting Partial matches merge semantics and improves DXThe function signature and JSDoc now reflect Partial, aligning with how defaults are merged.
🧹 Nitpick comments (2)
packages/mobile-sdk-alpha/src/types/public.ts (1)
244-249: Tighten WsConn callback types; avoid anyInbound message/error types should be concrete to help consumers and avoid unsound any usage.
Apply:
export interface WsConn { send: (data: string | ArrayBufferView | ArrayBuffer) => void; close: () => void; - onMessage: (cb: (data: any) => void) => void; - onError: (cb: (e: any) => void) => void; - onClose: (cb: () => void) => void; + onMessage: (cb: (data: string | ArrayBuffer | ArrayBufferView) => void) => void; + onError: (cb: (e: unknown) => void) => void; + onClose: (cb: (code?: number, reason?: string) => void) => void; }packages/mobile-sdk-alpha/tests/client.test.ts (1)
60-60: Add timeout error emission test for generateProofThe
generateProofimplementation uses your injectedclock.sleepto trigger a timeout and then callsemit('error', sdkError('timeout', 'SELF_ERR_PROOF_TIMEOUT', ...)). To cover this behavior, add a test immediately after the existing stub-proof assertion:it('returns stub proof handle when adapters provided', async () => { // …existing setup and assertions… }); + + it('emits a timeout error on generateProof', async () => { + const errors: any[] = []; + const network = { http: { fetch: vi.fn() }, ws: { connect: vi.fn() } } as any; + const crypto = { hash: vi.fn(), sign: vi.fn() } as any; + const scanner = { scan: vi.fn() } as any; + const client = createSelfClient({ + config: { timeouts: { proofMs: 0 } }, + adapters: { + network, + crypto, + scanner, + clock: { now: () => Date.now(), sleep: async () => {} }, // immediate + }, + }); + client.on('error', e => errors.push(e)); + await client.generateProof({ type: 'register', payload: {} }, { timeoutMs: 0 }); + await Promise.resolve(); // allow the timeout promise to fire + expect(errors).toHaveLength(1); + expect((errors[0] as any).message).toContain('SELF_ERR_PROOF_TIMEOUT'); + });This ensures your timeout path is exercised without waiting on real timers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
packages/mobile-sdk-alpha/src/adapters/index.ts(1 hunks)packages/mobile-sdk-alpha/src/bridge/nativeEvents.native.ts(2 hunks)packages/mobile-sdk-alpha/src/bridge/nativeEvents.ts(1 hunks)packages/mobile-sdk-alpha/src/browser.ts(1 hunks)packages/mobile-sdk-alpha/src/client.ts(5 hunks)packages/mobile-sdk-alpha/src/config/defaults.ts(1 hunks)packages/mobile-sdk-alpha/src/config/merge.ts(1 hunks)packages/mobile-sdk-alpha/src/index.ts(1 hunks)packages/mobile-sdk-alpha/src/types/public.ts(7 hunks)packages/mobile-sdk-alpha/tests/client.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/mobile-sdk-alpha/src/adapters/index.ts
- packages/mobile-sdk-alpha/src/config/defaults.ts
- packages/mobile-sdk-alpha/src/config/merge.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/mobile-sdk-alpha/src/browser.ts
- packages/mobile-sdk-alpha/src/index.ts
- packages/mobile-sdk-alpha/src/bridge/nativeEvents.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{test,spec}.{ts,js,tsx,jsx}
⚙️ CodeRabbit Configuration File
**/*.{test,spec}.{ts,js,tsx,jsx}: Review test files for:
- Test coverage completeness
- Test case quality and edge cases
- Mock usage appropriateness
- Test readability and maintainability
Files:
packages/mobile-sdk-alpha/tests/client.test.ts
packages/mobile-sdk-alpha/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit Configuration File
packages/mobile-sdk-alpha/**/*.{ts,tsx,js,jsx}: Review alpha mobile SDK code for:
- API consistency with core SDK
- Platform-neutral abstractions
- Performance considerations
- Clear experimental notes or TODOs
Files:
packages/mobile-sdk-alpha/tests/client.test.tspackages/mobile-sdk-alpha/src/bridge/nativeEvents.native.tspackages/mobile-sdk-alpha/src/client.tspackages/mobile-sdk-alpha/src/types/public.ts
🧬 Code Graph Analysis (2)
packages/mobile-sdk-alpha/src/bridge/nativeEvents.native.ts (2)
packages/mobile-sdk-alpha/src/bridge/nativeEvents.ts (2)
addListener(31-32)NativeEventBridge(12-26)app/ios/MRZScannerModule.swift (1)
moduleName(16-18)
packages/mobile-sdk-alpha/src/client.ts (1)
packages/mobile-sdk-alpha/src/types/public.ts (12)
Config(24-34)Adapters(96-103)SelfClient(194-208)SDKEvent(158-158)SDKEventMap(150-154)Unsubscribe(212-212)ScanOpts(167-169)ScanResult(173-184)ValidationInput(224-226)ValidationResult(230-233)RegistrationInput(136-139)RegistrationStatus(143-146)
🔇 Additional comments (4)
packages/mobile-sdk-alpha/src/bridge/nativeEvents.native.ts (2)
8-10: Docs: helpful summary for emitter retrievalThe JSDoc is clear and useful for discoverability.
41-43: Docs: clear and consistent with API surfaceThe removal semantics are documented. See adjacent comment for a robustness improvement to the implementation.
packages/mobile-sdk-alpha/src/types/public.ts (1)
6-12: Confirmed TS DOM lib for URL/AbortSignal
Thepackages/mobile-sdk-alpha/tsconfig.jsonexplicitly includes"DOM"(and ES2020), soURLandAbortSignalresolve at compile-time. Dependent configs also pull indomvia the root React Native tsconfig. Builds for both RN and Node should succeed without unexpected lib.dom gaps.packages/mobile-sdk-alpha/tests/client.test.ts (1)
87-95: Test mock correctly matches new HttpResponseLikeThe fetch mock aligns with the new shape (status/headers/text/json/arrayBuffer) and keeps tests independent of DOM Response.
| export const addListener: NativeEventBridge['addListener'] = (moduleName, eventName, handler) => { | ||
| const emitter = getEmitter(moduleName); | ||
| const sub: NativeEventSubscription = emitter.addListener(eventName, handler); | ||
| return () => sub.remove(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure listener removal works reliably across RN versions (track subscriptions).
On newer React Native versions, NativeEventEmitter may not expose a stable emitter.removeListener(eventName, handler). Relying solely on subscription.remove() is fine for the returned unsubscribe, but it leaves NativeEventBridge.removeListener(moduleName, eventName, handler) potentially ineffective and can cause memory leaks if clients use that API.
Store the EmitterSubscription keyed by moduleName/eventName/handler when adding, so removeListener can always detach the exact subscription.
Apply this diff within this function:
export const addListener: NativeEventBridge['addListener'] = (moduleName, eventName, handler) => {
const emitter = getEmitter(moduleName);
- const sub: NativeEventSubscription = emitter.addListener(eventName, handler);
- return () => sub.remove();
+ const sub: NativeEventSubscription = emitter.addListener(eventName, handler);
+ // Track subscription by handler so removeListener can reliably detach across RN versions
+ const byModule = (subscriptions[moduleName] ||= {});
+ const byEvent = (byModule[eventName] ||= new Map());
+ byEvent.set(handler as any, sub);
+ return () => {
+ sub.remove();
+ byEvent.delete(handler as any);
+ };
};Additionally, add the following outside the selected range:
// Import the handler type for stronger typing
import type { NativeEventBridge, EventHandler } from './nativeEvents';
// Registry of subscriptions by module -> event -> handler
const subscriptions: Record<string, Record<string, Map<EventHandler, NativeEventSubscription>>> = {};🤖 Prompt for AI Agents
In packages/mobile-sdk-alpha/src/bridge/nativeEvents.native.ts around lines 35
to 39, add a subscriptions registry and import the handler types, then change
addListener to store the created NativeEventSubscription in
subscriptions[moduleName][eventName].set(handler, sub) and return an unsubscribe
that calls sub.remove() and also deletes the map entry (clearing empty
maps/keys); also implement/modify NativeEventBridge.removeListener(moduleName,
eventName, handler) to look up the subscription in that registry and call
sub.remove() and clean up the registry so removal works reliably across RN
versions.
| export type { PassportValidationCallbacks } from '../validation/document'; | ||
|
|
||
| // Platform-neutral HTTP shapes | ||
| export type RequestLike = string | URL; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid referencing global URL type; replace with a URL-like shape to prevent DOM lib coupling
Using URL in exported types can force consumers to include lib.dom or rely on Node’s globals, reducing portability in some RN/Node setups. Use a minimal URL-like shape instead.
Apply:
- export type RequestLike = string | URL;
+ export type URLLike = { toString(): string } | { href: string };
+ export type RequestLike = string | URLLike;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type RequestLike = string | URL; | |
| export type URLLike = { toString(): string } | { href: string }; | |
| export type RequestLike = string | URLLike; |
🤖 Prompt for AI Agents
In packages/mobile-sdk-alpha/src/types/public.ts around line 7, the exported
type RequestLike currently references the global URL which can force consumers
to include lib.dom; replace that with a minimal URL-like shape (e.g. an object
type with a string href property and/or a toString() method) so consumers can
pass either a string or any lightweight URL-like object without requiring DOM
types; update the export to use string | { href: string; toString?: () => string
} (or similar minimal shape) and update any usages/tests to accept the new type.
| export interface RequestInitLike { | ||
| method?: string; | ||
| headers?: Record<string, string>; | ||
| body?: string | ArrayBuffer | ArrayBufferView | Uint8Array; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broaden body type without pulling in DOM types (Blob/FormData) to support file uploads in RN
Current body excludes common mobile payloads (images/files). Add platform-neutral BlobLike/FormDataLike aliases instead of DOM Blob/FormData to keep portability.
Apply:
export interface RequestInitLike {
method?: string;
headers?: Record<string, string>;
- body?: string | ArrayBuffer | ArrayBufferView | Uint8Array;
+ body?: string | ArrayBuffer | ArrayBufferView | Uint8Array | BlobLike | FormDataLike;
}Add near the HTTP types:
// Minimal, portable shapes for binary/form bodies
export interface BlobLike {
readonly size: number;
readonly type: string;
arrayBuffer(): Promise<ArrayBuffer>;
text(): Promise<string>;
}
export type FormDataEntryLike = string | BlobLike | ArrayBuffer | Uint8Array;
export type FormDataLike = Record<string, FormDataEntryLike | FormDataEntryLike[]>;🤖 Prompt for AI Agents
In packages/mobile-sdk-alpha/src/types/public.ts around lines 8 to 12,
RequestInitLike currently omits mobile-friendly binary/form payloads; add
lightweight, platform-neutral types and expand the body union to include them:
introduce a BlobLike interface (size, type, arrayBuffer(), text()), a
FormDataEntryLike union (string | BlobLike | ArrayBuffer | Uint8Array) and a
FormDataLike type (Record<string, FormDataEntryLike | FormDataEntryLike[]>),
then update RequestInitLike.body to also accept BlobLike | FormDataLike so
file/image uploads work in RN without pulling in DOM types.
|
need to update paths based on new |
|
closing due to new proposed work and architecture |
Summary
Testing
yarn workspaces foreach -p -v --topological-dev --since=HEAD run nice --if-presentyarn lintyarn buildyarn workspace @selfxyz/contracts build(fails: Invalid account config)yarn typesyarn test(fails: workspace build errors)yarn workspace @selfxyz/mobile-sdk-alpha testhttps://chatgpt.com/codex/tasks/task_b_68a47bf92098832d90948a3680a84b31
Summary by CodeRabbit