docs: add architecture guide and reorganize docs #890
Conversation
WalkthroughAdds architecture documentation under packages/mobile-sdk-alpha/docs/, updates the package README to reference these docs and relocate the Migration Checklist link. No code or public API changes. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the "Integrations" page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
packages/mobile-sdk-alpha/docs/ARCHITECTURE.md (6)
7-7: Clarify Metro tree-shaking limitations to set correct expectationsMetro’s tree-shaking isn’t equivalent to Webpack/Rollup. Suggest clarifying to avoid over-promising bundle size wins.
- The alpha SDK follows an adapter-first, React Native–oriented design. Tree-shakable named exports and `"sideEffects": false` keep bundles lean. While React Native is the only supported platform today, the structure below leaves room to add web, Capacitor, or Cordova targets without major refactors. + The alpha SDK follows an adapter-first, React Native–oriented design. Named exports and `"sideEffects": false` help minimize bundles (noting that Metro’s tree-shaking differs from Webpack/Rollup). While React Native is the only supported platform today, the structure below leaves room to add web, Capacitor, or Cordova targets without major refactors.
12-12: Event bridge: add guidance on listener lifecycle and leaksCall out unsubscribe and single-subscription patterns to avoid leaks and duplicate handlers.
- 2. **Bridge layer for native events** – wrap `NativeModules`/`NativeEventEmitter` so features register listeners through a shared adapter. + 2. **Bridge layer for native events** – wrap `NativeModules`/`NativeEventEmitter` so features register listeners through a shared adapter. Ensure `addListener` returns an unsubscribe, consolidate per-event native subscriptions within the bridge, and guard against duplicate registrations.
13-13: Typed errors: include codes and avoid PIIRecommend standard error codes, toJSON, and strict sanitization to prevent PII/secret leakage.
- 3. **Exception classes** – add typed errors (`InitError`, `LivenessError`, etc.) and surface them instead of generic `Error`. + 3. **Exception classes** – add typed errors (`InitError`, `LivenessError`, etc.) with stable `code` fields and `toJSON()`. Sanitize messages (no PII/secrets) and map native error codes to these classes.
14-14: SDK lifecycle: specify idempotency and cancellation semanticsDocument idempotent initialize/deinitialize and cancellation (AbortController) to prevent race conditions and orphaned tasks.
- 4. **SDK lifecycle management** – evolve `createSelfClient` into an SDK class with `initialize`/`deinitialize` and stored config. + 4. **SDK lifecycle management** – evolve `createSelfClient` into an SDK class with `initialize`/`deinitialize` and stored config. Make lifecycle methods idempotent, enforce single initialization, and support cancellation (e.g., `AbortController`) for in-flight work.
15-15: Packaging targets: note conditional exports and types per targetCall out
exportsmap and per-target type declarations to keep DX solid across RN/web.- 5. **Package targets** – keep React Native build first, but scaffold entry points for web and future environments (Capacitor/Cordova). + 5. **Package targets** – keep React Native build first, but scaffold entry points for web and future environments (Capacitor/Cordova) using conditional `exports` (`react-native`, `browser`, `default`) and per-target `types`.
25-27: Add security and privacy guidanceGiven the SDK’s handling of identity data, make security/PII posture explicit.
- Favor small, sensical abstractions over premature generality. - Maintain tree-shakability by avoiding side effects in new modules. - Document new APIs and flows under `docs/` as they solidify. + - Security & privacy: + - Never log MRZ/NFC payloads or derived keys; redact sensitive fields in errors/logs. + - Require explicit user consent before scanning; surface platform prompts where applicable. + - Store secrets via platform keystore/Keychain; avoid plain AsyncStorage or debug logs.packages/mobile-sdk-alpha/README.md (1)
3-3: Standardize terminology: “adapter-first” (singular) to match ARCHITECTURE.mdKeep terminology consistent across docs.
-Alpha SDK for registering and proving. Adapters-first, React Native-first with web shims. Minimal surface for scan → validate → generate proof → attestation verification. +Alpha SDK for registering and proving. Adapter-first, React Native–first with web shims. Minimal surface for scan → validate → generate proof → attestation verification.packages/mobile-sdk-alpha/docs/ARCHITECTURE_PROMPTS.md (6)
11-18: Pre-flight: add safe monorepo-wide alternatives and note known Yarn constraintA single workspace flow is fine; also offer foreach variants and warn about the
--since+-Aconflict noted in the PR description.```bash yarn workspace @selfxyz/mobile-sdk-alpha nice yarn workspace @selfxyz/mobile-sdk-alpha build yarn workspace @selfxyz/mobile-sdk-alpha types yarn workspace @selfxyz/mobile-sdk-alpha test yarn lint yarn build + # Optional: run across all workspaces (avoids --since/-A conflict) + yarn workspaces foreach -A -p --topological-dev run build --if-present + yarn workspaces foreach -A -p --topological-dev run types --if-present + # Note: avoid combining --since with -A (Yarn constraint).--- `35-37`: **Bridge prompts: encourage typed event maps and centralized subscriptions** Add guidance for a typed EventMap, one native subscription per event, and disposers to prevent leaks. ```diff 1. Add `src/bridge/nativeEvents.ts` wrapping `NativeModules` and `NativeEventEmitter`. -2. Expose `addListener` and `removeListener` helpers so modules can register without touching React Native directly. +2. Expose typed `addListener<Event extends keyof EventMap>()` / `removeListener` helpers with a single native subscription per event and per-listener disposers to prevent leaks.
55-57: Lifecycle prompts: specify idempotency and concurrency guaranteesStating these behaviors upfront reduces edge cases in app integrations.
1. Convert `createSelfClient` into a class exposing `initialize()` and `deinitialize()`. -2. Store configuration and adapters on the instance to avoid global state. +2. Store configuration and adapters on the instance to avoid global state. Make `initialize()`/`deinitialize()` idempotent, queue/ignore concurrent calls, and support cancellation of in-flight operations.
65-67: Packaging prompts: include conditional exports, types, and sideEffectsThis keeps RN/web consumers correct and tree-shakable.
1. Add build outputs for web, Capacitor, and Cordova under `dist/`. -2. Configure `package.json` `exports` to point to the new bundles. +2. Configure `package.json` `exports` (`react-native`, `browser`, `default`) and per-target `types`, and set `"sideEffects": false`.
75-77: Dogfooding: provide a concrete workspace install exampleAdd the workspace protocol example to minimize version drift.
1. Add `@selfxyz/mobile-sdk-alpha` to `app/package.json` and wire flows to use the SDK. 2. Validate builds and tests in the `app` workspace. + ```bash + yarn workspace app add @selfxyz/mobile-sdk-alpha@workspace:* + ```
85-86: Android sample: add security note about secrets and test dataAvoid accidental leakage in a public sample.
1. Under `samples/android/`, scaffold a basic React Native project showing MRZ → NFC → proof generation. -2. Document setup steps in `samples/android/README.md`. +2. Document setup steps in `samples/android/README.md` (no real credentials or PII; use mock/test data and environment variables where needed).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/mobile-sdk-alpha/README.md(1 hunks)packages/mobile-sdk-alpha/docs/ARCHITECTURE.md(1 hunks)packages/mobile-sdk-alpha/docs/ARCHITECTURE_PROMPTS.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/mobile-sdk-alpha/docs/ARCHITECTURE.md
[grammar] ~11-~11: There might be a mistake here.
Context: ...lders and re-export from src/index.ts. 2. Bridge layer for native events – wrap ...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...ster listeners through a shared adapter. 3. Exception classes – add typed errors (...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...surface them instead of generic Error. 4. SDK lifecycle management – evolve `cre...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...alize/deinitialize` and stored config. 5. Package targets – keep React Native bu...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...future environments (Capacitor/Cordova). 6. Dogfood in /app – integrate the SDK ...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ... app workspace to validate real flows. 7. Android demo app – ship a minimal Reac...
(QB_NEW_EN)
🔇 Additional comments (3)
packages/mobile-sdk-alpha/README.md (3)
52-56: Docs links LGTMMigration and Architecture links resolve under ./docs/ as expected.
28-38: Confirmed — README imports are valid: referenced symbols are exportedVerified that the package entrypoint re-exports the symbols used in the README; no broken examples.
- packages/mobile-sdk-alpha/src/index.ts — re-exports:
- createSelfClient (from ./client)
- webScannerShim (from ./adapters/web/shims)
- extractMRZInfo, formatDateToYYMMDD (from ./processing/mrz)
- parseNFCResponse (from ./processing/nfc)
- Definitions live in:
- packages/mobile-sdk-alpha/src/client.ts (createSelfClient)
- packages/mobile-sdk-alpha/src/adapters/web/shims.ts (webScannerShim)
- packages/mobile-sdk-alpha/src/processing/mrz.ts (extractMRZInfo, formatDateToYYMMDD)
- packages/mobile-sdk-alpha/src/processing/nfc.ts (parseNFCResponse)
- browser build also re-exports MRZ helpers and the web shim via packages/mobile-sdk-alpha/src/browser.ts.
No fixes required for the README imports.
5-8: exports and sideEffects verified for packages/mobile-sdk-alpha/package.jsonpackages/mobile-sdk-alpha/package.json includes the required export conditions and is tree-shaking friendly:
- File: packages/mobile-sdk-alpha/package.json
- exports "." keys: types, react-native, browser, import, require, default
- exports "./browser" present
- sideEffects: false
- type: "module"
No changes required.
|
@RamsesAupart see that you are using an ai pr reviewer (https://github.com/RamsesAupart/ai-pr-reviewer) so changed our repo setting to allow only members to review |
Summary
Testing
yarn workspaces foreach -A -p -v --topological-dev --since=HEAD run nice --if-present(fails: Invalid option schema: property "all" forbids using property "since" )yarn lintyarn buildyarn workspace @selfxyz/contracts build(fails: Invalid account in Hardhat config)yarn typesyarn workspace @selfxyz/mobile-sdk-alpha testyarn workspace @selfxyz/mobile-sdk-alpha niceyarn workspace @selfxyz/mobile-sdk-alpha buildhttps://chatgpt.com/codex/tasks/task_b_689cce692430832db753e9d9f1c0fe08
Summary by CodeRabbit