feat: upgrade SelfClient foundation and expose MRZ parsing #925
Conversation
WalkthroughAdds a React SelfClient context/provider and high-level SelfMobileSdk, extends SelfClient API (extractMRZInfo, stub registerDocument), reorganizes MRZ/NFC/QR exports, introduces UI screens/hooks/types, multiple tests, and updates package metadata and test environment. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App
participant Provider as SelfClientProvider
participant Factory as createSelfClient
participant Ctx as SelfClientContext
participant Child as Consumer
App->>Provider: Render({config, adapters})
Provider->>Factory: createSelfClient({config, adapters})
Factory-->>Provider: SelfClient
Provider->>Ctx: Provide SelfClient
Child->>Ctx: useSelfClient()
Ctx-->>Child: SelfClient instance
sequenceDiagram
autonumber
participant HLC as SelfMobileSdk
participant Provider as SelfClientProvider
participant Content as SelfMobileSdkContent
participant External as external
participant Client as SelfClient
HLC->>Provider: Wrap content
Content->>External: getAllDocuments()
External-->>Content: documents
alt any registered document
Content->>External: show Disclosure (QRCodeScreen)
External-->>Content: onDisclosureSuccess/Failure
else no registered documents
Content->>Client: registerDocument({mode:'mrz', ...})
Client-->>Content: RegistrationStatus
alt registered
Content->>External: onOnboardingSuccess()
else
Content->>External: onOnboardingFailure()
end
end
sequenceDiagram
autonumber
participant Caller as Consumer/Test
participant Client as SelfClient
participant MRZ as processing/mrz
Caller->>Client: extractMRZInfo(mrz)
Client->>MRZ: parseMRZInfo(mrz)
MRZ-->>Client: MRZInfo
Client-->>Caller: MRZInfo
Caller->>Client: registerDocument(input)
Client-->>Caller: { registered: false, reason: 'SELF_REG_STATUS_STUB' }
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/mobile-sdk-alpha/src/types/public.ts (1)
131-131: Confirm synchronous API addition and document thrown errorThis is the only synchronous method on SelfClient; confirm that’s intentional for API consistency. Also, callers need to know it can throw (e.g., MrzParseError). Add JSDoc to clarify behavior and error type.
Apply this diff to document the method:
): Promise<ProofHandle>; - extractMRZInfo(mrz: string): MRZInfo; + /** + * Parses a TD3 MRZ string into structured info. + * Throws MrzParseError on invalid format or checksum failure. + * + * Security note: MRZ contains PII. Avoid logging raw values. + */ + extractMRZInfo(mrz: string): MRZInfo;packages/mobile-sdk-alpha/src/browser.ts (1)
44-44: Browser barrel also drops extractMRZInfo — ensure migration guidanceSame breaking implication as index.ts for browser consumers. Recommend either adding a deprecated re-export for one cycle or documenting migration to client.extractMRZInfo or './mrz'.
Optional transitional alias:
export { defaultConfig } from './config/defaults'; -export { formatDateToYYMMDD, scanMRZ } from './mrz'; +export { formatDateToYYMMDD, scanMRZ } from './mrz'; +/** @deprecated Use createSelfClient().extractMRZInfo or import from './mrz' */ +export { extractMRZInfo } from './mrz';packages/mobile-sdk-alpha/tests/client.test.ts (1)
81-87: Add negative-path coverage: invalid MRZ should throw via clientGreat to see the happy-path test. Please also assert that client.extractMRZInfo propagates parser errors (e.g., MrzParseError) for malformed input.
You can add a test like:
import { MrzParseError } from '../src/index'; // at top near other imports it('throws MrzParseError on malformed MRZ via client', () => { const client = createSelfClient({ config: {}, adapters: { scanner, network, crypto } }); expect(() => client.extractMRZInfo('P<UTOERIKSSON<<ANNA<MARIA')).toThrowError(MrzParseError); });packages/mobile-sdk-alpha/src/client.ts (1)
101-109: API extension looks good; consider PII-safe error handling wrapper for MRZ parsingExposing MRZ parsing through the client is a good move for API cohesion. One concern: MRZ strings contain PII. If the underlying parser throws with messages that include the raw MRZ, consumers might inadvertently log that. To be safe, wrap the call to sanitize thrown errors (preserve code/cause, omit MRZ text).
Apply this diff in the return object to wrap and sanitize errors:
return { scanDocument, validateDocument, checkRegistration, generateProof, - extractMRZInfo: parseMRZInfo, + extractMRZInfo: (mrz: string) => { + try { + return parseMRZInfo(mrz); + } catch (err) { + const e = err as any; + // Avoid leaking MRZ/PII via error messages + const sanitized = new Error('MRZ parse failed'); + // Preserve machine-readable metadata if present + if (e?.code) (sanitized as any).code = e.code; + if ('cause' in Error.prototype) { + // TS/ES2022 optional; safe no-op if unsupported at runtime + (sanitized as any).cause = e; + } + throw sanitized; + } + }, on, emit, };
- Optional follow-up: add a brief JSDoc on
extractMRZInfostating that callers should not log MRZ input or error messages verbatim to avoid PII leakage.
📜 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 (6)
packages/mobile-sdk-alpha/src/browser.ts(1 hunks)packages/mobile-sdk-alpha/src/client.ts(2 hunks)packages/mobile-sdk-alpha/src/index.ts(1 hunks)packages/mobile-sdk-alpha/src/types/public.ts(1 hunks)packages/mobile-sdk-alpha/tests/client.test.ts(1 hunks)packages/mobile-sdk-alpha/tests/processing/mrz.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/types/public.tspackages/mobile-sdk-alpha/tests/client.test.tspackages/mobile-sdk-alpha/tests/processing/mrz.test.tspackages/mobile-sdk-alpha/src/browser.tspackages/mobile-sdk-alpha/src/index.tspackages/mobile-sdk-alpha/src/client.ts
**/*.{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.tspackages/mobile-sdk-alpha/tests/processing/mrz.test.ts
🧬 Code Graph Analysis (2)
packages/mobile-sdk-alpha/src/types/public.ts (2)
packages/mobile-sdk-alpha/src/browser.ts (1)
MRZInfo(12-12)packages/mobile-sdk-alpha/src/index.ts (1)
MRZInfo(10-10)
packages/mobile-sdk-alpha/tests/client.test.ts (1)
packages/mobile-sdk-alpha/src/client.ts (1)
createSelfClient(40-110)
🔇 Additional comments (2)
packages/mobile-sdk-alpha/tests/processing/mrz.test.ts (1)
3-4: LGTM: import path updated to dedicated MRZ moduleThe test now targets the dedicated MRZ module and remains consistent with the new public surface.
packages/mobile-sdk-alpha/src/client.ts (1)
4-4: MRZ parser import is lightweight
Themrz.tsmodule only pulls in a local error class and some type definitions—no heavy OCR, vision, or WASM libraries were detected. Keeping the static import inpackages/mobile-sdk-alpha/src/client.tswon’t meaningfully bloat mobile bundles. No refactoring needed here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/mobile-sdk-alpha/package.json (1)
57-59: Peer range on React may be too narrow for RN apps—consider widening.Using "^18.3.1" can cause peer resolution conflicts for apps pinned to 18.2.x (still common in RN projects). Consider widening to avoid unnecessary peer warnings.
Proposed change:
- "peerDependencies": { - "react": "^18.3.1" - }, + "peerDependencies": { + "react": ">=18.2 <19" + },packages/mobile-sdk-alpha/src/index.ts (1)
61-61: Direct export of extractMRZInfo contradicts PR objective—decide on API and keep it consistent.PR description says “remove direct export” and expose via client, but Line 61 still re-exports extractMRZInfo. Please confirm the desired public API and align both entry points (index and browser) accordingly.
If the goal is to remove the direct export now:
-export { extractMRZInfo, formatDateToYYMMDD, scanMRZ } from './mrz'; +export { formatDateToYYMMDD, scanMRZ } from './mrz';If you prefer a transitional deprecation:
- export { extractMRZInfo, formatDateToYYMMDD, scanMRZ } from './mrz'; + /** @deprecated Use createSelfClient().extractMRZInfo or import directly from './mrz' */ + export { extractMRZInfo, formatDateToYYMMDD, scanMRZ } from './mrz';packages/mobile-sdk-alpha/tests/provider.test.tsx (1)
33-44: MRZ happy-path covered; consider adding a negative/edge case to harden the contract.A failing MRZ (bad check digits, wrong line lengths) would catch regressions in validation flags. Not blocking, but worthwhile for medium-priority correctness in parsing logic.
I can add a complementary test with an invalid MRZ to assert info.validation.overall === false. Want me to draft it?
packages/mobile-sdk-alpha/src/browser.ts (1)
45-45: Keep browser entry aligned with root barrel regarding extractMRZInfo.Line 45 still re-exports extractMRZInfo. If you choose to remove or deprecate it in src/index.ts, mirror the same here to avoid entry-point divergence.
Removal option:
-export { extractMRZInfo, formatDateToYYMMDD, scanMRZ } from './mrz'; +export { formatDateToYYMMDD, scanMRZ } from './mrz';Transitional deprecation option:
- export { extractMRZInfo, formatDateToYYMMDD, scanMRZ } from './mrz'; + /** @deprecated Use createSelfClient().extractMRZInfo or import directly from './mrz' */ + export { extractMRZInfo, formatDateToYYMMDD, scanMRZ } from './mrz';packages/mobile-sdk-alpha/src/context.tsx (1)
16-16: Guard against churn: re-instantiation on object identity changes can drop client state
useMemouses reference equality. If callers pass inline objects forconfigoradapters, you’ll recreate the client frequently, potentially dropping sessions, network state, or caches. Ensure upstream passes stable references, or document that these props must be memoized. If frequent changes are expected, consider a deep-compare memoization strategy or normalizing props (e.g.,useMemoat the callsite).
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/mobile-sdk-alpha/package.json(2 hunks)packages/mobile-sdk-alpha/src/browser.ts(1 hunks)packages/mobile-sdk-alpha/src/context.tsx(1 hunks)packages/mobile-sdk-alpha/src/index.ts(1 hunks)packages/mobile-sdk-alpha/tests/provider.test.tsx(1 hunks)packages/mobile-sdk-alpha/tsconfig.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/browser.tspackages/mobile-sdk-alpha/tests/provider.test.tsxpackages/mobile-sdk-alpha/src/context.tsxpackages/mobile-sdk-alpha/src/index.ts
**/*.{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/provider.test.tsx
🔇 Additional comments (6)
packages/mobile-sdk-alpha/tsconfig.json (1)
19-19: React type injection looks appropriate for new context API.Adding "react" to compilerOptions.types aligns with the new TSX context exports. Given React is now a peerDependency, this should not force bundling. No blocking issues.
packages/mobile-sdk-alpha/package.json (1)
61-63: Dev deps consistent with JSDOM tests and React context. Verify CI Node version supports Response.The added dev deps are appropriate. Note that your tests instantiate a global Response in provider.test.tsx; ensure CI runs on Node >= 18 (or polyfill ‘undici’) to prevent failures due to missing Response in JSDOM.
Also applies to: 72-72, 74-75
packages/mobile-sdk-alpha/src/index.ts (1)
56-56: Context exports from root barrel are fine; confirm tree-shaking and RN compatibility.Exporting SelfClientProvider/useSelfClient from the root barrel improves DX. Make sure the build remains side-effect-free so non-React consumers aren’t forced to include React. Given peer dep usage and tsup config, this looks okay.
packages/mobile-sdk-alpha/src/browser.ts (1)
42-42: Context re-exports on the browser entry look good.This keeps the API surface consistent across entries without introducing side-effects.
packages/mobile-sdk-alpha/src/context.tsx (2)
20-23: Solid runtime guard for misuse outside providerThrowing when the hook is used without a provider is the right DX and avoids silent
nullfailures.
15-18: SelfClientProvider cleanup isn’t needed—client is currently side-effect freecreateSelfClient today returns a pure, in-memory object (no open sockets, subscriptions or timers at module-level), and the SelfClient interface doesn’t expose any dispose/close API to invoke. The only timer is the stub timeout inside generateProof, which you’d only hit if you call generateProof—and it lives and dies within the ProofHandle’s own lifecycle.
No changes required to SelfClientProvider until createSelfClient gains real resources to clean up. If, in a future implementation, you introduce sockets, subscriptions, or other persistent handles, then:
• Add a
dispose()(or similar) method on SelfClient
• Invoke it in auseEffectcleanup when the client changes or the provider unmountsFor now, you can safely ignore the suggested cleanup.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (4)
packages/mobile-sdk-alpha/package.json(5 hunks)packages/mobile-sdk-alpha/src/react-native.ts(1 hunks)packages/mobile-sdk-alpha/tests/react-native.test.ts(1 hunks)packages/mobile-sdk-alpha/tsup.config.ts(2 hunks)
🧰 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/react-native.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/react-native.test.tspackages/mobile-sdk-alpha/tsup.config.tspackages/mobile-sdk-alpha/src/react-native.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/react-native.tspackages/mobile-sdk-alpha/package.json
🧬 Code Graph Analysis (1)
packages/mobile-sdk-alpha/tests/react-native.test.ts (1)
packages/mobile-sdk-alpha/src/react-native.ts (2)
createSelfClient(43-43)extractMRZInfo(45-45)
🔇 Additional comments (5)
packages/mobile-sdk-alpha/package.json (2)
62-64: Peer dependency on React may be too restrictive for RN consumers.RN apps in the wild still ship with React 18.2.x; requiring ^18.3.1 can cause peer resolution warnings or force upgrades. Consider widening to a range that includes 18.2.x (and optionally 19 when ready).
Would you like me to propose a compatibility matrix and a version range that covers current Expo/RN baselines?
15-22: Guard against type/runtime divergence for the default RN entryThe package-root “.” export uses the React-Native build at runtime (
react-native.js) while its TypeScript types still point atindex.d.ts. Ifsrc/index.tsandsrc/react-native.tsever diverge in their public exports, you’ll get no compile errors but runtime failures or undefined symbols.• Verify that
src/index.ts(or its true entry file) andsrc/react-native.tsre-export the exact same API surface.
• If they differ, either:
- Align both modules to re-export identical symbols, or
- Change the
"types"entry for “.” inpackage.jsonto a declaration file generated from your RN build (so types match runtime).Also applies to the analogous mapping at lines 28–32.
packages/mobile-sdk-alpha/tests/react-native.test.ts (1)
10-15: Tests are correct given current exports
Thereact-native.tsentry still directly exportsextractMRZInfo, so the existing test importing it is valid. No changes are needed here. If and when we remove that top-level export in a future breaking release, we can revisit and update tests accordingly.Likely an incorrect or invalid review comment.
packages/mobile-sdk-alpha/tsup.config.ts (1)
8-9: Build entries for RN look correct.Adding the react-native entry for both ESM and CJS aligns with the new export surface and should generate matching .d.ts in dist/esm. No issues spotted.
Also applies to: 23-24
packages/mobile-sdk-alpha/src/react-native.ts (1)
39-39: No action needed –SdkErrorCategoryis a pure type aliasVerified in packages/mobile-sdk-alpha/src/errors/SdkError.ts that
export type SdkErrorCategory = | 'scanner' | 'network' | …;and the index re-exports it with
export type { SdkErrorCategory }. There is no runtime enum or value to export, so the currentexport typeform is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/mobile-sdk-alpha/src/react-native.ts (2)
46-46: Direct MRZ export bypasses the “client-only” MRZ API—remove to keep API consistent.Re-exporting extractMRZInfo here conflicts with the goal to expose MRZ parsing via SelfClient. It introduces two public paths and risks API drift across entries.
Apply this diff:
-export { extractMRZInfo, formatDateToYYMMDD, scanMRZ } from './mrz'; +export { formatDateToYYMMDD, scanMRZ } from './mrz';To find any direct imports you may need to adjust in tests or docs:
#!/bin/bash # Find direct imports of extractMRZInfo from the RN entry or root rg -n -C2 -P "import\s+\{[^}]*extractMRZInfo[^}]*\}\s+from\s+['\"](\.\./src/react-native|../src|@selfxyz/mobile-sdk-alpha)['\"]"
57-57: Remove web-only shim from RN entry to prevent potential runtime crashes.Re-exporting webScannerShim from the RN bundle can cause Metro to evaluate browser-specific code (window/document), breaking React Native at runtime or bloating bundles.
Apply this diff:
-export { webScannerShim } from './adapters/web/shims';
🧹 Nitpick comments (2)
packages/mobile-sdk-alpha/tests/client.test.ts (1)
81-95: Solid coverage for MRZ parsing and registration stub via the client.
- Confirms extractMRZInfo correctness including validation.overall.
- Verifies registerDocument stub contract.
Consider adding one negative MRZ case (bad checksum/date) to lock in error semantics, but current coverage is acceptable for this PR’s scope.
packages/mobile-sdk-alpha/src/entry/index.tsx (1)
12-16: Guard against inadvertent client re-creation due to unstable adapters/config identities.SelfClientProvider memoizes the client on [config, adapters]. If parents pass new object literals each render (e.g., adapters={{ ... }}), the client will be rebuilt, potentially dropping listeners/state and impacting performance.
- Document that adapters/config should be stable across renders, or
- Change SelfClientProvider to derive a stable memoization key (e.g., shallow hash of adapter function refs) to avoid unnecessary re-instantiation.
I can propose a targeted provider-side change if you want to pursue this now.
📜 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 (9)
packages/mobile-sdk-alpha/src/client.ts(3 hunks)packages/mobile-sdk-alpha/src/context.tsx(1 hunks)packages/mobile-sdk-alpha/src/entry/index.tsx(1 hunks)packages/mobile-sdk-alpha/src/index.ts(1 hunks)packages/mobile-sdk-alpha/src/react-native.ts(1 hunks)packages/mobile-sdk-alpha/src/types/public.ts(2 hunks)packages/mobile-sdk-alpha/tests/client.test.ts(1 hunks)packages/mobile-sdk-alpha/tests/entry.test.tsx(1 hunks)packages/mobile-sdk-alpha/tests/provider.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/mobile-sdk-alpha/src/types/public.ts
- packages/mobile-sdk-alpha/src/context.tsx
- packages/mobile-sdk-alpha/tests/provider.test.tsx
- packages/mobile-sdk-alpha/src/client.ts
🧰 Additional context used
📓 Path-based instructions (2)
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/entry/index.tsxpackages/mobile-sdk-alpha/tests/entry.test.tsxpackages/mobile-sdk-alpha/src/react-native.tspackages/mobile-sdk-alpha/tests/client.test.tspackages/mobile-sdk-alpha/src/index.ts
**/*.{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/entry.test.tsxpackages/mobile-sdk-alpha/tests/client.test.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/react-native.ts
🧬 Code Graph Analysis (3)
packages/mobile-sdk-alpha/src/entry/index.tsx (2)
packages/mobile-sdk-alpha/src/types/public.ts (2)
Config(3-13)Adapters(55-62)packages/mobile-sdk-alpha/src/context.tsx (1)
SelfClientProvider(15-18)
packages/mobile-sdk-alpha/tests/entry.test.tsx (3)
packages/mobile-sdk-alpha/src/context.tsx (1)
useSelfClient(20-24)packages/mobile-sdk-alpha/src/types/public.ts (3)
ScannerAdapter(116-118)NetworkAdapter(78-81)CryptoAdapter(14-17)packages/mobile-sdk-alpha/src/entry/index.tsx (1)
SelfMobileSdk(12-16)
packages/mobile-sdk-alpha/tests/client.test.ts (1)
packages/mobile-sdk-alpha/src/client.ts (1)
createSelfClient(40-115)
🔇 Additional comments (2)
packages/mobile-sdk-alpha/src/index.ts (1)
56-57: Re-exporting context and entry from the root is good; API surface looks coherent.Surfaces SelfClientContext/Provider/hook and the SelfMobileSdk entry from a single place, which improves DX and keeps parity with other entries.
packages/mobile-sdk-alpha/tests/entry.test.tsx (1)
41-50: Test covers the new client-provisioning path and MRZ parse—LGTM.The test validates context wiring and exercises extractMRZInfo via the client. This is a meaningful integration check for the new API surface.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/mobile-sdk-alpha/package.json (1)
56-59: Potential RN resolution break: missingreact-nativeexport conditionYou removed the
react-nativecondition from the “.” export. Metro (and some RN tooling) prefer thereact-nativecondition overbrowser/import. Without it, RN projects may resolve to the generic ESM entry, increasing the risk of pulling web-only modules (e.g., DOM-based components) into native bundles.Add a
react-nativecondition to the “.” export and keep web-only components restricted to the browser entry to prevent accidental inclusion in RN apps.Example exports snippet (outside the changed lines) to illustrate:
{ "exports": { ".": { "types": "./dist/esm/index.d.ts", "browser": "./dist/esm/browser.js", "react-native": "./dist/esm/index.js", "import": "./dist/esm/index.js", "require": "./dist/cjs/index.cjs", "default": "./dist/esm/index.js" }, "./browser": { "types": "./dist/esm/browser.d.ts", "import": "./dist/esm/browser.js", "require": "./dist/cjs/browser.cjs" } } }Action items:
- Verify whether any RN demo/app in this monorepo can still consume this package without custom resolver tweaks.
- Consider exposing the DOM-heavy “high-level” component only via
./browser(and not from the main index) so it’s invisible to RN consumers by default.packages/mobile-sdk-alpha/tests/SelfMobileSdk.test.tsx (1)
13-30: Test does not validate runtime rendering or provider wiringAsserting that a props object can be constructed doesn’t exercise the component, nor does it detect cross-platform regressions (e.g., DOM usage in RN). Add a smoke render test (jsdom) and, if possible, a bundling-level test that imports from the RN condition to ensure no web-only modules leak into RN builds.
I can help scaffold a minimal render test that mounts
<SelfMobileSdk config={...} external={...} adapters={...} />with mock adapters and asserts it doesn’t throw and displays the expected initial state.packages/mobile-sdk-alpha/tests/client.test.tsx (1)
23-31: Fix misleading test: it doesn’t exercise “different adapter configurations”This test uses the exact same
mockAdaptersas other tests and doesn’t validate any adapter-driven behavior. As written, it’s redundant and can mislead about coverage.Minimal fix (rename to reflect reality):
- it('accepts different adapter configurations', () => { + it('instantiates with provided adapters', () => { const clientWithAllAdapters = createSelfClient({ config: {}, adapters: mockAdapters, }); expect(clientWithAllAdapters).toBeDefined(); expect(typeof clientWithAllAdapters.extractMRZInfo).toBe('function'); });Alternative (actually vary adapters and assert behavior): replace
network.http.fetchorcryptowith a different stub and call a method that uses it (e.g., whenregisterDocumentis implemented). Until then, renaming is safer to avoid false confidence.If you want, I can add an adapter-driven test that asserts behavior once
registerDocumentis wired tonetworkandcrypto.
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
packages/mobile-sdk-alpha/package.json(3 hunks)packages/mobile-sdk-alpha/src/components/SelfMobileSdk.tsx(1 hunks)packages/mobile-sdk-alpha/src/context.tsx(1 hunks)packages/mobile-sdk-alpha/src/index.ts(1 hunks)packages/mobile-sdk-alpha/tests/SelfMobileSdk.test.tsx(1 hunks)packages/mobile-sdk-alpha/tests/client.test.tsx(1 hunks)packages/mobile-sdk-alpha/tests/entry.test.tsx(1 hunks)packages/mobile-sdk-alpha/tests/provider.test.tsx(1 hunks)packages/mobile-sdk-alpha/tests/utils/testHelpers.ts(1 hunks)packages/mobile-sdk-alpha/vitest.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/mobile-sdk-alpha/tests/entry.test.tsx
- packages/mobile-sdk-alpha/tests/provider.test.tsx
- packages/mobile-sdk-alpha/src/index.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.tsxpackages/mobile-sdk-alpha/tests/SelfMobileSdk.test.tsx
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.tsxpackages/mobile-sdk-alpha/vitest.config.tspackages/mobile-sdk-alpha/tests/SelfMobileSdk.test.tsxpackages/mobile-sdk-alpha/tests/utils/testHelpers.tspackages/mobile-sdk-alpha/src/components/SelfMobileSdk.tsxpackages/mobile-sdk-alpha/src/context.tsx
🧠 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/package.json
🧬 Code Graph Analysis (4)
packages/mobile-sdk-alpha/tests/client.test.tsx (1)
packages/mobile-sdk-alpha/tests/utils/testHelpers.ts (3)
mockAdapters(30-34)sampleMRZ(5-5)expectedMRZResult(37-40)
packages/mobile-sdk-alpha/tests/SelfMobileSdk.test.tsx (2)
packages/mobile-sdk-alpha/src/index.ts (2)
SelfMobileSdk(57-57)SelfMobileSdk(58-58)packages/mobile-sdk-alpha/src/components/SelfMobileSdk.tsx (1)
SelfMobileSdk(182-188)
packages/mobile-sdk-alpha/src/components/SelfMobileSdk.tsx (2)
app/src/providers/passportDataProvider.tsx (2)
DocumentMetadata(84-91)getAllDocuments(353-369)packages/mobile-sdk-alpha/src/context.tsx (2)
useSelfClient(32-36)SelfClientProvider(15-30)
packages/mobile-sdk-alpha/src/context.tsx (2)
packages/mobile-sdk-alpha/src/types/public.ts (3)
SelfClient(119-135)Config(3-13)Adapters(55-62)packages/mobile-sdk-alpha/src/client.ts (1)
createSelfClient(40-115)
🔇 Additional comments (2)
packages/mobile-sdk-alpha/vitest.config.ts (1)
6-6: LGTM: jsdom environment is appropriate for React component testsSwitching to jsdom aligns with the new React components/hooks under test.
packages/mobile-sdk-alpha/src/context.tsx (1)
32-36: Hook guard is correct and aligns with best practicesThrowing when the hook is used outside the provider prevents silent misuse at runtime. No changes needed.
|
@coderabbitai full summary |
|
Tip For best results, initiate chat on the files or code changes. @transphorm Here's a comprehensive summary of PR #925: 🎯 Core ObjectiveThis PR exposes MRZ (Machine Readable Zone) parsing functionality through the SelfClient interface, moving from direct exports to a client-based approach while adding React context support and high-level components. 🏗️ Major Architectural Changes1. MRZ Parsing Integration
2. New Client Methods
3. React Context System
🧩 New High-Level Components4. Entry Components
5. Mobile Onboarding FlowThe high-level component provides:
📦 Package & Configuration Updates6. Version & Dependencies
7. Build & Test Configuration
🧪 Comprehensive Test Coverage8. New Test Suites
🔄 Migration Impact9. Breaking Changes
10. New Capabilities
🎯 Usage Examples// Old approach (no longer available)
import { extractMRZInfo } from '@selfxyz/mobile-sdk-alpha';
// New client-based approach
const client = createSelfClient({ config, adapters });
const mrzInfo = client.extractMRZInfo(mrzString);
// React context approach
<SelfClientProvider config={config} adapters={adapters}>
<YourComponent />
</SelfClientProvider>
// High-level component approach
<SelfMobileSdk config={config} external={externalHandlers} />This PR represents a significant evolution of the mobile SDK, transitioning from utility functions to a comprehensive client-based architecture with full React integration support. |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
packages/mobile-sdk-alpha/src/types/ui.ts (1)
37-39: Type the MRZ detection payload to prevent misuse and validation gaps
onMRZDetected: (mrzData: any) => voidweakens type-safety for a security-sensitive flow. Given MRZ parsing is now exposed on the client, define and use a concrete type (e.g.,MRZCapture,MRZTextPayload, or reuse a shared type if available) to avoid accidental logging/serialization bugs and ensure validation at boundaries.Example:
-export interface PassportCameraProps { - onMRZDetected: (mrzData: any) => void; -} +export type MRZTextPayload = { lines: [string, string, string] } | { text: string }; + +export interface PassportCameraProps { + onMRZDetected: (mrz: MRZTextPayload) => void; +}If your SelfClient API’s
extractMRZInfoexpects a specific shape (string vs. lines), align this type accordingly and reuse a shared type from@selfxyz/commonif one exists.packages/mobile-sdk-alpha/src/hooks/useDocumentManager.ts (1)
5-5: Ensureexternalis a stable reference to avoid redundant fetchesIf callers pass a new
externalobject per render, this will refetch documents repeatedly. Either constrain consumers to pass a stable adapter instance or depend only onexternal.getAllDocumentsif its identity is stable.Would you like me to add a short docstring to this hook clarifying the stability requirement, or to refactor the dependency to
external.getAllDocuments?packages/mobile-sdk-alpha/src/components/flows/OnboardingFlow.tsx (2)
42-42: Dependency list includes an unused prop
setDocumentis in the dependency array but not used inhandleMRZDetected. Either use it (e.g., persist on success) or remove it from the deps to avoid unnecessary callback re-creations.Apply this diff:
- [client, external, setDocument], + [client, external],
6-7: Web-only screen imports can cause RN bundling issuesImporting DOM-based screens directly in a cross-platform flow risks accidental RN breakage if defaults are rendered. With the component-type change above, consider moving the web defaults behind platform-specific files (OnboardingFlow.web.tsx and OnboardingFlow.native.tsx) or lazy-loading them for web only.
If you want, I can draft platform-specific file splits and adjust the barrel exports to keep the native bundle clean.
packages/mobile-sdk-alpha/src/index.ts (1)
59-74: Exporting web-only screens from the root barrel can leak DOM into RN appsExposing DOM-based components at the package root increases the chance of inadvertent imports in React Native, leading to runtime errors. Consider:
- Platform-specific barrels via the package.json exports field (mapping "react-native" to a native index without DOM screens).
- Separate entry points (e.g., ./browser for web screens, ./native without them) and documenting usage.
I can propose an exports map and reorganized barrels to keep native bundles clean if helpful.
📜 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 (14)
packages/mobile-sdk-alpha/src/browser.ts(1 hunks)packages/mobile-sdk-alpha/src/components/SelfMobileSdk.tsx(1 hunks)packages/mobile-sdk-alpha/src/components/flows/OnboardingFlow.tsx(1 hunks)packages/mobile-sdk-alpha/src/components/screens/NFCScannerScreen.tsx(1 hunks)packages/mobile-sdk-alpha/src/components/screens/PassportCameraScreen.tsx(1 hunks)packages/mobile-sdk-alpha/src/components/screens/QRCodeScreen.tsx(1 hunks)packages/mobile-sdk-alpha/src/context.tsx(1 hunks)packages/mobile-sdk-alpha/src/hooks/useDocumentManager.ts(1 hunks)packages/mobile-sdk-alpha/src/index.ts(3 hunks)packages/mobile-sdk-alpha/src/types/ui.ts(1 hunks)packages/mobile-sdk-alpha/tests/SelfMobileSdk.test.tsx(1 hunks)packages/mobile-sdk-alpha/tests/client.test.ts(2 hunks)packages/mobile-sdk-alpha/tests/client.test.tsx(1 hunks)packages/mobile-sdk-alpha/tests/utils/testHelpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/mobile-sdk-alpha/tests/utils/testHelpers.ts
- packages/mobile-sdk-alpha/src/components/SelfMobileSdk.tsx
- packages/mobile-sdk-alpha/tests/client.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
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/components/screens/QRCodeScreen.tsxpackages/mobile-sdk-alpha/src/components/screens/NFCScannerScreen.tsxpackages/mobile-sdk-alpha/src/components/screens/PassportCameraScreen.tsxpackages/mobile-sdk-alpha/src/types/ui.tspackages/mobile-sdk-alpha/tests/SelfMobileSdk.test.tsxpackages/mobile-sdk-alpha/src/components/flows/OnboardingFlow.tsxpackages/mobile-sdk-alpha/tests/client.test.tsxpackages/mobile-sdk-alpha/src/context.tsxpackages/mobile-sdk-alpha/src/browser.tspackages/mobile-sdk-alpha/src/index.tspackages/mobile-sdk-alpha/src/hooks/useDocumentManager.ts
**/*.{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/SelfMobileSdk.test.tsxpackages/mobile-sdk-alpha/tests/client.test.tsx
🧬 Code Graph Analysis (9)
packages/mobile-sdk-alpha/src/components/screens/QRCodeScreen.tsx (2)
packages/mobile-sdk-alpha/src/index.ts (2)
QRCodeScreen(67-67)ScreenProps(36-36)packages/mobile-sdk-alpha/src/types/ui.ts (1)
ScreenProps(32-35)
packages/mobile-sdk-alpha/src/components/screens/NFCScannerScreen.tsx (1)
packages/mobile-sdk-alpha/src/types/ui.ts (1)
ScreenProps(32-35)
packages/mobile-sdk-alpha/src/components/screens/PassportCameraScreen.tsx (1)
packages/mobile-sdk-alpha/src/types/ui.ts (1)
PassportCameraProps(37-39)
packages/mobile-sdk-alpha/src/types/ui.ts (1)
packages/mobile-sdk-alpha/src/index.ts (5)
DocumentMetadata(36-36)DocumentData(36-36)ExternalAdapter(36-36)ScreenProps(36-36)PassportCameraProps(36-36)
packages/mobile-sdk-alpha/tests/SelfMobileSdk.test.tsx (3)
packages/mobile-sdk-alpha/src/index.ts (1)
SelfMobileSdk(73-73)packages/mobile-sdk-alpha/src/browser.ts (1)
SelfMobileSdk(44-44)packages/mobile-sdk-alpha/src/components/SelfMobileSdk.tsx (1)
SelfMobileSdk(58-64)
packages/mobile-sdk-alpha/src/components/flows/OnboardingFlow.tsx (3)
packages/mobile-sdk-alpha/src/types/ui.ts (2)
ExternalAdapter(19-29)DocumentData(13-16)packages/mobile-sdk-alpha/src/components/screens/PassportCameraScreen.tsx (1)
PassportCameraScreen(4-15)packages/mobile-sdk-alpha/src/components/screens/NFCScannerScreen.tsx (1)
NFCScannerScreen(6-28)
packages/mobile-sdk-alpha/tests/client.test.tsx (1)
packages/mobile-sdk-alpha/tests/utils/testHelpers.ts (5)
mockAdapters(43-47)sampleMRZ(5-5)expectedMRZResult(50-53)invalidMRZ(7-7)badCheckDigitsMRZ(8-8)
packages/mobile-sdk-alpha/src/context.tsx (2)
packages/mobile-sdk-alpha/src/types/public.ts (3)
SelfClient(119-135)Config(3-13)Adapters(55-62)packages/mobile-sdk-alpha/src/client.ts (1)
createSelfClient(40-115)
packages/mobile-sdk-alpha/src/hooks/useDocumentManager.ts (1)
packages/mobile-sdk-alpha/src/types/ui.ts (2)
ExternalAdapter(19-29)DocumentData(13-16)
🔇 Additional comments (7)
packages/mobile-sdk-alpha/tests/SelfMobileSdk.test.tsx (1)
10-30: No additional MRZ parsing test needed – public API is already coveredExisting tests already validate
extractMRZInfothrough the public client interface:
- packages/mobile-sdk-alpha/tests/entry.test.tsx (lines 10–14): uses
useSelfClient()and callsclient.extractMRZInfo- packages/mobile-sdk-alpha/tests/provider.test.tsx (lines 18–21): renders
useSelfClient()and asserts onextractMRZInfo- packages/mobile-sdk-alpha/tests/client.test.ts(.tsx) (multiple cases): directly creates
SelfClientand verifies parsing, error throwing, and validation flags- packages/mobile-sdk-alpha/tests/processing/mrz.test.ts: covers the internal
extractMRZInfologicSince the MRZ extraction path is already exercised end-to-end via the client, no additional test is required here.
Likely an incorrect or invalid review comment.
packages/mobile-sdk-alpha/tests/client.test.tsx (1)
7-44: LGTM: solid coverage including unhappy pathsGood job covering:
- API surface shape
- Happy-path MRZ parse
- Malformed MRZ throwing MrzParseError
- Bad check digits returning validation.overall = false
This meaningfully hardens input handling for untrusted MRZ.
packages/mobile-sdk-alpha/src/context.tsx (1)
15-19: Provider is now platform-neutral and efficientCreating the client with useMemo and removing web DOM placeholders fixes React Native breakage and avoids unnecessary renders. This is the right shape for a cross-platform SDK provider.
packages/mobile-sdk-alpha/src/index.ts (1)
79-82: Backward compatibility preserved with deprecated re-exportReintroducing
extractMRZInfowith a deprecation notice prevents breakage while guiding consumers toclient.extractMRZInfoor./mrz. Good balance between stability and forward direction.packages/mobile-sdk-alpha/src/browser.ts (3)
42-45: Context and DOM component exports look appropriate for the browser surfaceExposing the React context/hook and the DOM-based high-level component only from the browser entry is aligned with platform separation. No side effects or SSR-leakage concerns apparent from this entrypoint.
46-46: createSelfClient export aligns with the API expansionThis keeps the client-centric API as the primary integration path. No issues noted here.
48-48: defaultConfig export is fineUseful for deterministic configuration in browser consumers. No issues.
packages/mobile-sdk-alpha/src/components/flows/OnboardingFlow.tsx
Outdated
Show resolved
Hide resolved
packages/mobile-sdk-alpha/src/components/flows/OnboardingFlow.tsx
Outdated
Show resolved
Hide resolved
packages/mobile-sdk-alpha/src/components/flows/OnboardingFlow.tsx
Outdated
Show resolved
Hide resolved
packages/mobile-sdk-alpha/src/components/screens/PassportCameraScreen.tsx
Outdated
Show resolved
Hide resolved
transphorm
force-pushed
the
codex/add-extractmrzinfo-method-and-tests
branch
from
cf98de1 to
fc850a3
Compare
|
@coderabbitai title |
|
Tip For best results, initiate chat on the files or code changes. @transphorm The current PR title "feat: expose MRZ parsing on client" is well-crafted and appropriate for several reasons: ✅ Follows conventional commits - Uses The title effectively communicates that existing users will need to migrate from direct Alternative titles could be considered if you wanted to emphasize different aspects:
But the current title strikes the right balance between accuracy and brevity. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
packages/mobile-sdk-alpha/src/browser.ts (1)
50-51: Remove direct export of extractMRZInfo; enforce SelfClient-only accessThis contradicts the PR objective and creates a duplicate API surface. Keep MRZ parsing behind SelfClient to avoid bypassing client-level logic (telemetry, gating, error handling).
Apply this diff:
-/** @deprecated Use createSelfClient().extractMRZInfo or import from './mrz' */ -export { extractMRZInfo, formatDateToYYMMDD, scanMRZ } from './mrz'; +// MRZ helpers (extractMRZInfo is available via SelfClient only) +export { formatDateToYYMMDD, scanMRZ } from './mrz';Verification script to ensure no remaining direct imports from the public barrel:
#!/bin/bash set -euo pipefail echo "Searching for direct named imports of extractMRZInfo from the package barrel..." rg -nP -C2 $'import\\s+\\{[^}]*\\bextractMRZInfo\\b[^}]*\\}\\s+from\\s+[\'"]@selfxyz/mobile-sdk-alpha[\'"]' -g '!**/node_modules/**' || true echo -e "\nSearching for barrel-based imports from ../src (local consumers/tests)..." rg -nP -C2 $'import\\s+\\{[^}]*\\bextractMRZInfo\\b[^}]*\\}\\s+from\\s+[\'"]\\.\\./src[\'"]' -g '!**/node_modules/**' || truepackages/mobile-sdk-alpha/src/hooks/useDocumentManager.ts (1)
11-22: Prevent setState on unmounted component; catch sync and async errorsWithout an unmount guard, late resolves can trigger setState after unmount (React warning/leak). Also, synchronous throws from getAllDocuments won’t be caught.
Apply this diff:
useEffect(() => { - external - .getAllDocuments() - .then(documents => { - setDocuments(documents); - setIsLoading(false); - }) - .catch(error => { - console.error('Failed to load documents:', error); - setIsLoading(false); - }); + let cancelled = false; + (async () => { + try { + const docs = await external.getAllDocuments(); + if (!cancelled) { + setDocuments(docs); + } + } catch (error) { + if (!cancelled) { + console.error('Failed to load documents:', error); + } + } finally { + if (!cancelled) { + setIsLoading(false); + } + } + })(); + return () => { + cancelled = true; + }; }, [external]);
🧹 Nitpick comments (6)
docs/templates/pr-action-items-template.md (3)
134-142: Expand the Testing Checklist for React Native/TypeScript reliabilityGiven this repo targets RN + TS, add concrete checks to prevent platform compatibility regressions and packaging surprises.
### Before Merge - [ ] {{SPECIFIC_TEST_TO_RUN}} - [ ] {{SPECIFIC_VALIDATION}} - [ ] {{SPECIFIC_BUILD_TEST}} + - [ ] Type-check: yarn workspace @selfxyz/mobile-sdk-alpha types (tsc --noEmit) + - [ ] Lint: yarn workspace @selfxyz/mobile-sdk-alpha lint + - [ ] React Native bundle (ios): npx react-native bundle --platform ios --dev false --entry-file index.ts --bundle-output /tmp/main.ios.jsbundle + - [ ] React Native bundle (android): npx react-native bundle --platform android --dev false --entry-file index.ts --bundle-output /tmp/index.android.bundle + - [ ] Verify no Node.js-only APIs imported in RN code (fs, path, crypto without polyfills) + - [ ] Package hygiene: ensure package.json "exports"/"react-native" fields are correct and "sideEffects" is set for tree-shaking + - [ ] Bundle size check for public entry points (no accidental test/mocks in bundle) ### Post-Merge - [ ] {{INTEGRATION_TEST}} - [ ] {{PERFORMANCE_TEST}} + - [ ] E2E smoke on emulator/device (Android + iOS) + - [ ] Validate published package excludes docs/pr-reports artifacts
31-35: Replace the non-standard “giga_read_pr” with GitHub CLI/RESTWe’ve confirmed GH CLI v2.74.2 is available, so updating this template will improve portability for all contributors.
• Location: docs/templates/pr-action-items-template.md (lines 31–35)
• Change the bullet forgiga_read_prto usegh pr view/gh api(with a curl+jq fallback):- - Use giga_read_pr to fetch PR data and CodeRabbit comments + - Use the GitHub CLI to fetch PR data and comments: + gh pr view {{PR_NUMBER}} --json comments,files,headRefName,author,title,createdAt --comments + gh api repos/:owner/:repo/pulls/{{PR_NUMBER}}/reviews + - If GitHub CLI is unavailable, fall back to the GitHub REST API via curl+jq.
36-37: Ensure publication safety for docs/pr-reportsThe root .gitignore already ignores
PR-*-ACTION*.mdglobally, so your newdocs/pr-reports/PR-{{NUMBER}}-ACTION-ITEMS.mdfiles won’t be checked in by mistake. However, if you ever publish from the repo root (or from a package that includes this folder), make sure they’re excluded from the published tarball:• Confirm
"private": truein the root package.json to block accidental publishes.
• If you do publish, adddocs/pr-reports/(ordocs/pr-reports/PR-*-ACTION*.md) to a.npmignoreor whitelist only your dist/src folders via the package.json"files"field.Suggested snippet update:
- - Create file as PR-{{NUMBER}}-ACTION-ITEMS.md in project root - - Follow gitignore pattern: PR-*-ACTION*.md + - Create file at docs/pr-reports/PR-{{NUMBER}}-ACTION-ITEMS.md + (root .gitignore already ignores `PR-*-ACTION*.md`) + - Ensure root package.json has `"private": true` + - If you ever publish from root or this folder’s parent package: + • Add `docs/pr-reports/` to `.npmignore` + • Or whitelist only your build artifacts in package.json `"files"`packages/mobile-sdk-alpha/tests/setup.ts (1)
30-45: Make window.matchMedia configurable to avoid hard-to-debug redefine errorsDefining the property without configurable: true can cause failures if tests or helpers attempt to redefine it later with Object.defineProperty or try to delete it for cleanup. Safer to make it configurable in test environments.
Apply this diff:
Object.defineProperty(window, 'matchMedia', { writable: true, + configurable: true, value: vi.fn().mockImplementation(query => ({ matches: false, media: query, onchange: null, addListener: vi.fn(), // deprecated removeListener: vi.fn(), // deprecated addEventListener: vi.fn(), removeEventListener: vi.fn(), dispatchEvent: vi.fn(), })), });packages/mobile-sdk-alpha/tests/SelfMobileSdk.test.tsx (1)
10-30: Make this a real type-level assertion using expectTypeOfThe current “props shape” test doesn’t validate types at compile-time; it only asserts that constructing an object doesn’t throw at runtime (TypeScript types are erased). Use vitest’s expectTypeOf to assert the component’s props contract.
Apply this diff:
-import { describe, expect, it } from 'vitest'; +import { describe, expect, it, expectTypeOf } from 'vitest'; @@ it('accepts the expected props interface', async () => { const { SelfMobileSdk } = await import('../src'); - // Test that the component accepts the expected props structure + // Test that the component accepts the expected props structure (type-level) const mockExternal = { getSecret: async () => 'test-secret', getAllDocuments: async () => ({}), setDocument: async () => true, onOnboardingSuccess: () => {}, onOnboardingFailure: () => {}, onDisclosureSuccess: () => {}, onDisclosureFailure: () => {}, }; expect(SelfMobileSdk).toBeDefined(); - // The component should accept these props without throwing - expect(() => { - // This is just a type check - we're not actually rendering - const _props = { config: {}, external: mockExternal }; - }).not.toThrow(); + type Props = Parameters<typeof SelfMobileSdk>[0]; + const _props = { config: {}, external: mockExternal }; + expectTypeOf(_props).toMatchTypeOf<Props>(); });packages/mobile-sdk-alpha/src/index.ts (1)
59-74: Top-level re-exports of RN components/context can bloat bundles; consider subpath exports.Exporting UI components and React context from the root barrel risks pulling RN/React code into consumers that only need the core client/types, harming tree-shaking and platform neutrality.
Recommendation:
- Create subpath entry points (e.g.,
@selfxyz/mobile-sdk-alpha/ui,@selfxyz/mobile-sdk-alpha/context,@selfxyz/mobile-sdk-alpha/components) and move these exports there.- Configure
exportsin package.json so the root entry remains platform-neutral and side-effect free.This keeps the core API lean and reduces accidental transitive coupling to React/React Native while still exposing the UI surface for apps that need it.
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
docs/gigamind/pr-analysis-workflow.md(1 hunks)docs/templates/pr-action-items-template.md(4 hunks)packages/mobile-sdk-alpha/package.json(3 hunks)packages/mobile-sdk-alpha/src/browser.ts(1 hunks)packages/mobile-sdk-alpha/src/client.ts(3 hunks)packages/mobile-sdk-alpha/src/components/SelfMobileSdk.tsx(1 hunks)packages/mobile-sdk-alpha/src/components/flows/OnboardingFlow.tsx(1 hunks)packages/mobile-sdk-alpha/src/components/screens/NFCScannerScreen.tsx(1 hunks)packages/mobile-sdk-alpha/src/components/screens/PassportCameraScreen.tsx(1 hunks)packages/mobile-sdk-alpha/src/components/screens/QRCodeScreen.tsx(1 hunks)packages/mobile-sdk-alpha/src/context.tsx(1 hunks)packages/mobile-sdk-alpha/src/entry/index.tsx(1 hunks)packages/mobile-sdk-alpha/src/hooks/useDocumentManager.ts(1 hunks)packages/mobile-sdk-alpha/src/index.ts(3 hunks)packages/mobile-sdk-alpha/src/types/public.ts(2 hunks)packages/mobile-sdk-alpha/src/types/ui.ts(1 hunks)packages/mobile-sdk-alpha/tests/SelfMobileSdk.test.tsx(1 hunks)packages/mobile-sdk-alpha/tests/client.test.ts(2 hunks)packages/mobile-sdk-alpha/tests/client.test.tsx(1 hunks)packages/mobile-sdk-alpha/tests/entry.test.tsx(1 hunks)packages/mobile-sdk-alpha/tests/processing/mrz.test.ts(1 hunks)packages/mobile-sdk-alpha/tests/provider.test.tsx(1 hunks)packages/mobile-sdk-alpha/tests/setup.ts(1 hunks)packages/mobile-sdk-alpha/tests/utils/testHelpers.ts(1 hunks)packages/mobile-sdk-alpha/tsconfig.json(1 hunks)packages/mobile-sdk-alpha/vitest.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/mobile-sdk-alpha/tests/entry.test.tsx
- packages/mobile-sdk-alpha/src/context.tsx
- packages/mobile-sdk-alpha/tests/client.test.tsx
- packages/mobile-sdk-alpha/src/components/screens/PassportCameraScreen.tsx
- packages/mobile-sdk-alpha/src/entry/index.tsx
- packages/mobile-sdk-alpha/src/components/screens/QRCodeScreen.tsx
- packages/mobile-sdk-alpha/tests/provider.test.tsx
- packages/mobile-sdk-alpha/vitest.config.ts
- packages/mobile-sdk-alpha/tests/processing/mrz.test.ts
- packages/mobile-sdk-alpha/src/client.ts
- packages/mobile-sdk-alpha/tests/client.test.ts
- packages/mobile-sdk-alpha/tsconfig.json
- packages/mobile-sdk-alpha/tests/utils/testHelpers.ts
- packages/mobile-sdk-alpha/src/types/public.ts
- packages/mobile-sdk-alpha/package.json
- packages/mobile-sdk-alpha/src/components/flows/OnboardingFlow.tsx
- packages/mobile-sdk-alpha/src/components/SelfMobileSdk.tsx
🧰 Additional context used
📓 Path-based instructions (2)
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/setup.tspackages/mobile-sdk-alpha/tests/SelfMobileSdk.test.tsxpackages/mobile-sdk-alpha/src/hooks/useDocumentManager.tspackages/mobile-sdk-alpha/src/types/ui.tspackages/mobile-sdk-alpha/src/components/screens/NFCScannerScreen.tsxpackages/mobile-sdk-alpha/src/index.tspackages/mobile-sdk-alpha/src/browser.ts
**/*.{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/SelfMobileSdk.test.tsx
🧬 Code Graph Analysis (3)
packages/mobile-sdk-alpha/tests/SelfMobileSdk.test.tsx (3)
packages/mobile-sdk-alpha/src/browser.ts (1)
SelfMobileSdk(44-44)packages/mobile-sdk-alpha/src/components/SelfMobileSdk.tsx (1)
SelfMobileSdk(58-64)packages/mobile-sdk-alpha/src/index.ts (1)
SelfMobileSdk(73-73)
packages/mobile-sdk-alpha/src/hooks/useDocumentManager.ts (1)
packages/mobile-sdk-alpha/src/types/ui.ts (2)
ExternalAdapter(24-34)DocumentData(18-21)
packages/mobile-sdk-alpha/src/components/screens/NFCScannerScreen.tsx (1)
packages/mobile-sdk-alpha/src/types/ui.ts (1)
ScreenProps(37-40)
🪛 LanguageTool
docs/templates/pr-action-items-template.md
[grammar] ~56-~56: There might be a mistake here.
Context: ... ({{TOTAL_COMMENTS}}/{{TOTAL_COMMENTS}}) All comments show ✅ "Addressed" status, ...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ..., indicating they were fixed in commits: - {{RESOLVED_CATEGORY_1}} - Fixed in commi...
(QB_NEW_EN)
[grammar] ~58-~58: There might be a mistake here.
Context: ..._1}} - Fixed in commits {{COMMIT_RANGE}} - {{RESOLVED_CATEGORY_2}} - Fixed in commi...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ...ed Comments 🔴 (0/{{TOTAL_COMMENTS}}) None - All comments have been addressed...
(QB_NEW_EN)
[grammar] ~62-~62: There might be a mistake here.
Context: ...ne** - All comments have been addressed. ## Conclusion **This PR is ready for merge.*...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...ents have been addressed. ## Conclusion This PR is ready for merge. All CodeRa...
(QB_NEW_EN)
[grammar] ~73-~73: There might be a mistake here.
Context: ...erity issues --> ### 1. {{ISSUE_TITLE}} Files: {{FILE_PATH}}:{{LINE_NUMBER}}...
(QB_NEW_EN)
[grammar] ~74-~74: There might be a mistake here.
Context: ...Files:** {{FILE_PATH}}:{{LINE_NUMBER}} CodeRabbit Comment: {{COMMENT_ID}} *...
(QB_NEW_EN)
[grammar] ~75-~75: There might be a mistake here.
Context: ...` CodeRabbit Comment: {{COMMENT_ID}} Problem: {{PROBLEM_DESCRIPTION}} **F...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ...D}} Problem: {{PROBLEM_DESCRIPTION}} Fix: {{SPECIFIC_FIX_OR_ACTION}} **C...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...DE_EXAMPLE}} ``` ### 2. {{ISSUE_TITLE}} Files: {{FILE_PATH}}:{{LINE_NUMBER}}...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ...Files:** {{FILE_PATH}}:{{LINE_NUMBER}} CodeRabbit Comment: {{COMMENT_ID}} *...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ...` CodeRabbit Comment: {{COMMENT_ID}} Problem: {{PROBLEM_DESCRIPTION}} **F...
(QB_NEW_EN)
[grammar] ~87-~87: There might be a mistake here.
Context: ...D}} Problem: {{PROBLEM_DESCRIPTION}} Fix: {{SPECIFIC_FIX_OR_ACTION}} ## ...
(QB_NEW_EN)
[grammar] ~92-~92: There might be a mistake here.
Context: ...ns ### Issue 1: {{GROUPED_ISSUE_TITLE}} Root Cause: {{ROOT_CAUSE_DESCRIPTION}}...
(QB_NEW_EN)
[grammar] ~93-~93: There might be a mistake here.
Context: ...Root Cause:** {{ROOT_CAUSE_DESCRIPTION}} Files Affected: - `{{FILE_PATH}}:{{L...
(QB_NEW_EN)
[grammar] ~113-~113: There might be a mistake here.
Context: ...R}}` - {{ISSUE_DESCRIPTION}} Actions: - [ ] {{SPECIFIC_ACTION_1}} - [ ] {{SPECIF...
(QB_NEW_EN)
[grammar] ~114-~114: There might be a mistake here.
Context: ...Actions: - [ ] {{SPECIFIC_ACTION_1}} - [ ] {{SPECIFIC_ACTION_2}} ## CodeRabbit...
(QB_NEW_EN)
[grammar] ~119-~119: There might be a mistake here.
Context: ...nalysis Summary ### Resolved Comments ✅ - {{RESOLVED_COMMENT_1}} - {{RESOLVED_COMM...
(QB_NEW_EN)
[grammar] ~120-~120: There might be a mistake here.
Context: ...lved Comments ✅ - {{RESOLVED_COMMENT_1}} - {{RESOLVED_COMMENT_2}} ### Unresolved C...
(QB_NEW_EN)
[grammar] ~124-~124: There might be a mistake here.
Context: ...MMENT_2}} ### Unresolved Comments 🔴 - {{UNRESOLVED_COMMENT_1}} - {{STATUS}} - {...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: ...{{UNRESOLVED_COMMENT_1}} - {{STATUS}} - {{UNRESOLVED_COMMENT_2}} - {{STATUS}} ##...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: ... - {{UNRESOLVED_COMMENT_2}} - {{STATUS}} ### Comment Categories - Critical: {{COUN...
(QB_NEW_EN)
[grammar] ~127-~127: There might be a mistake here.
Context: ...2}} - {{STATUS}} ### Comment Categories - Critical: {{COUNT}} comments (React Na...
(QB_NEW_EN)
[grammar] ~128-~128: There might be a mistake here.
Context: ...e compatibility, security, memory leaks) - Architecture: {{COUNT}} comments (API ...
(QB_NEW_EN)
[grammar] ~129-~129: There might be a mistake here.
Context: ...UNT}} comments (API design, type safety) - Code Quality: {{COUNT}} comments (test...
(QB_NEW_EN)
[grammar] ~178-~178: There might be a mistake here.
Context: ... BEFORE FINALIZING - VERIFY EACH ITEM: - [ ] ✅ Read ALL {{TOTAL_COMMENTS}} CodeRa...
(QB_NEW_EN)
[grammar] ~187-~187: There might be a mistake here.
Context: ...lly unresolved SEVERITY VERIFICATION: - [ ] ✅ CRITICAL: Security, memory leaks, ...
(QB_NEW_EN)
[grammar] ~188-~188: There might be a mistake here.
Context: ...tform compatibility ({{CRITICAL_COUNT}}) - [ ] ✅ HIGH: API inconsistencies, type sa...
(QB_NEW_EN)
[grammar] ~189-~189: There might be a mistake here.
Context: ...pe safety, architecture ({{HIGH_COUNT}}) - [ ] ✅ MEDIUM: Test coverage, performance...
(QB_NEW_EN)
[grammar] ~190-~190: There might be a mistake here.
Context: ...e, minor architecture ({{MEDIUM_COUNT}}) - [ ] ❌ LOW/NITPICK: Style, naming, docs (...
(QB_NEW_EN)
[grammar] ~195-~195: There might be a mistake here.
Context: ...OUNT}}) --- Last Updated: {{DATE}} CodeRabbit Comments Analyzed: {{TOTA...
(QB_NEW_EN)
[grammar] ~196-~196: There might be a mistake here.
Context: ... Comments Analyzed:** {{TOTAL_COMMENTS}} Unresolved Medium+ Issues: {{UNRESOL...
(QB_NEW_EN)
[grammar] ~197-~197: There might be a mistake here.
Context: ...d Medium+ Issues:** {{UNRESOLVED_COUNT}} Excluded Low/Nitpick Issues: {{EXCLU...
(QB_NEW_EN)
[grammar] ~198-~198: There might be a mistake here.
Context: ...Low/Nitpick Issues:** {{EXCLUDED_COUNT}}
(QB_NEW_EN)
docs/gigamind/pr-analysis-workflow.md
[grammar] ~3-~3: There might be a mistake here.
Context: # PR Analysis Workflow ## Objective Ensure consistent identification of only...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ...ep Process ### Phase 1: Data Collection 1. Use giga_read_pr to fetch PR data and ...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ... numbers ### Phase 2: Initial Filtering For each comment: 1. Check Status: S...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...e 2: Initial Filtering For each comment: 1. Check Status: Skip if contains "✅ Addr...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...ntains "✅ Addressed" 2. Check Type: Identify comment category (security, architectur...
(QB_NEW_EN)
[grammar] ~45-~45: There might be a mistake here.
Context: ... comments ### Phase 3: Content Analysis For remaining comments: 1. **Read Full C...
(QB_NEW_EN)
[grammar] ~46-~46: There might be a mistake here.
Context: ...Content Analysis For remaining comments: 1. Read Full Content: Understand the actu...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...e actual issue 2. Categorize Impact: - Blocks merge? → CRITICAL - Affects ar...
(QB_NEW_EN)
[grammar] ~52-~52: There might be a mistake here.
Context: ...DIUM - Cosmetic only? → LOW (exclude) 3. Group by Root Cause: Related issues to...
(QB_NEW_EN)
[grammar] ~55-~55: There might be a mistake here.
Context: ...r ### Phase 4: Verification (MANDATORY) 1. Double-check Status: Re-verify no "✅ A...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ...EDIUM+ impact 4. Template Selection: - If 0 unresolved → Use "All issues resolv...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ... with issues ### Phase 5: Documentation 1. Create Action Items: Use updated templ...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...ed Flags (Double-check if you see these) - High unresolved count (>5) - likely incl...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...d count (>5) - likely including nitpicks - All comments marked unresolved - likely ...
(QB_NEW_EN)
[style] ~73-~73: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...icks - All comments marked unresolved - likely missing status checks - Style/formattin...
(ADVERB_REPETITION_PREMIUM)
[grammar] ~73-~73: There might be a mistake here.
Context: ...nresolved - likely missing status checks - Style/formatting issues in critical sect...
(QB_NEW_EN)
[grammar] ~74-~74: There might be a mistake here.
Context: ... section - wrong severity classification - Vague descriptions - insufficient conten...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...lysis ### Green Flags (Good indicators) - Low unresolved count (0-3) - proper filt...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ...nresolved count (0-3) - proper filtering - Clear severity justification - good anal...
(QB_NEW_EN)
[grammar] ~79-~79: There might be a mistake here.
Context: ...r severity justification - good analysis - Specific file paths and line numbers - t...
(QB_NEW_EN)
[grammar] ~80-~80: There might be a mistake here.
Context: ...paths and line numbers - thorough review - Code examples for fixes - actionable out...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ... Mistakes to Avoid ### ❌ Don't Include: - Comments with "✅ Addressed" status - Sty...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ...de: - Comments with "✅ Addressed" status - Style/formatting suggestions - Documenta...
(QB_NEW_EN)
[grammar] ~87-~87: There might be a mistake here.
Context: ...d" status - Style/formatting suggestions - Documentation improvements (unless criti...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...mentation improvements (unless critical) - Naming conventions - Minor refactoring s...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ...s (unless critical) - Naming conventions - Minor refactoring suggestions - Import o...
(QB_NEW_EN)
[grammar] ~90-~90: There might be a mistake here.
Context: ...ventions - Minor refactoring suggestions - Import organization - Code comments/JSDo...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ...toring suggestions - Import organization - Code comments/JSDoc additions ### ✅ Do ...
(QB_NEW_EN)
[grammar] ~94-~94: There might be a mistake here.
Context: ...ments/JSDoc additions ### ✅ Do Include: - Security vulnerabilities - Memory leaks ...
(QB_NEW_EN)
[grammar] ~95-~95: There might be a mistake here.
Context: ...✅ Do Include: - Security vulnerabilities - Memory leaks - Platform compatibility is...
(QB_NEW_EN)
[grammar] ~96-~96: There might be a mistake here.
Context: ... Security vulnerabilities - Memory leaks - Platform compatibility issues (DOM in Re...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...mpatibility issues (DOM in React Native) - API design inconsistencies - Type safety...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ...act Native) - API design inconsistencies - Type safety problems - Performance bottl...
(QB_NEW_EN)
[grammar] ~99-~99: There might be a mistake here.
Context: ...n inconsistencies - Type safety problems - Performance bottlenecks - Test coverage ...
(QB_NEW_EN)
[grammar] ~100-~100: There might be a mistake here.
Context: ...afety problems - Performance bottlenecks - Test coverage gaps for critical paths - ...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ... - Test coverage gaps for critical paths - Breaking changes without migration ## T...
(QB_NEW_EN)
[grammar] ~128-~128: There might be a mistake here.
Context: ...rack these metrics for each PR analysis: - Total CodeRabbit comments analyzed - Com...
(QB_NEW_EN)
[grammar] ~129-~129: There might be a mistake here.
Context: ...is: - Total CodeRabbit comments analyzed - Comments with "✅ Addressed" status (excl...
(QB_NEW_EN)
[grammar] ~130-~130: There might be a mistake here.
Context: ...nts with "✅ Addressed" status (excluded) - Low/nitpick severity comments (excluded)...
(QB_NEW_EN)
[grammar] ~131-~131: There might be a mistake here.
Context: ...Low/nitpick severity comments (excluded) - Medium+ severity unresolved comments (in...
(QB_NEW_EN)
[grammar] ~132-~132: There might be a mistake here.
Context: ... severity unresolved comments (included) - Final unresolved count in action items ...
(QB_NEW_EN)
[grammar] ~137-~137: There might be a mistake here.
Context: ...ontinuous Improvement ### Weekly Review - Audit recent PR action items for false p...
(QB_NEW_EN)
[grammar] ~143-~143: There might be a mistake here.
Context: ... based on patterns ### Template Updates - Add new comment type mappings as CodeRab...
(QB_NEW_EN)
🔇 Additional comments (6)
packages/mobile-sdk-alpha/tests/setup.ts (1)
30-45: Vitest globals and types are correctly configured; explicitviimport is optionalThe Vitest setup in
packages/mobile-sdk-alpha/vitest.config.tsenablesglobals: trueand the project’stsconfig.jsonincludes"vitest/globals", soviis available and properly typed at both runtime and compile time. No change is required—importingviexplicitly is purely a stylistic choice.packages/mobile-sdk-alpha/src/browser.ts (1)
42-45: Exports correctly isolate the DOM-only component for browser buildsThe package’s
exportsinpackage.jsoninclude abrowsercondition pointing atdist/esm/browser.js(which includesSelfMobileSdkHighLevel), while React Native consumers will resolve the"import"(or default) entrydist/esm/index.js—which doesn’t exportSelfMobileSdkHighLevel. No further changes are needed.docs/gigamind/pr-analysis-workflow.md (1)
1-152: LGTM — no medium+ issuesProcess doc reads clearly, no security or platform risks introduced.
packages/mobile-sdk-alpha/src/components/screens/NFCScannerScreen.tsx (1)
2-2: Peer Dependency Check: Already ConfiguredI’ve verified that
tamagui, along withreactandreact-native, is already listed under peerDependencies inpackages/mobile-sdk-alpha/package.json. No further action is needed here.packages/mobile-sdk-alpha/src/types/ui.ts (1)
4-16: Good hardening: metadata now excludes plaintext PII (keep it that way).The JSDoc and the move to an opaque
encryptedBlobRefeliminate a common PII leakage vector. This aligns with least-privilege data handling for MRZ/ID data.packages/mobile-sdk-alpha/src/index.ts (1)
79-81: Transitional deprecated re-export looks good.Re-introducing
extractMRZInfowith a deprecation notice provides a safe migration path for consumers while moving usage toclient.extractMRZInfo. Good call.
transphorm
changed the title
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 (3)
packages/mobile-sdk-alpha/src/client.ts (3)
97-115: Fix: generateProof always emits a timeout error even if the proof succeeds; add cancellation/abort handling.The scheduled timeout emits an 'error' unconditionally after timeoutMs, with no way to cancel. This will produce spurious error events and potential noise/memory churn in long-lived apps.
Apply this diff to make timeout emission cancellable and abort-aware:
): 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'))); - return { + let cancelled = false; + // schedule timeout with cancellation/abort awareness + void _adapters.clock.sleep(timeoutMs!, opts.signal).then( + () => { + if (!cancelled) emit('error', new Error('timeout')); + }, + () => { + // aborted sleep: don't emit timeout + }, + ); + return { id: 'stub', status: 'pending', result: async () => ({ ok: false, reason: 'SELF_ERR_PROOF_STUB' }), - cancel: () => {}, + cancel: () => { + cancelled = true; + }, }; }
35-38: Propagate AbortSignal in clock.sleep to prevent dangling timers and improve cancellation semantics.Default sleep ignores AbortSignal, undermining consumer cancellations and contributing to needless timer wakeups.
Apply this diff:
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); + if (signal) { + if (signal.aborted) { + clearTimeout(t); + return resolve(); + } + const onAbort = () => { + clearTimeout(t); + signal.removeEventListener('abort', onAbort); + reject(Object.assign(new Error('Aborted'), { name: 'AbortError' })); + }; + signal.addEventListener('abort', onAbort, { once: true }); + } + }); + }, },
62-67: Prevent listener map leaks by deleting empty sets from the registry.Unsub only removes the callback from the set; the per-event Set remains in the Map forever once emptied. In a long-lived mobile process, this can accumulate stale keys.
Apply this diff:
function on<E extends SDKEvent>(event: E, cb: (payload: SDKEventMap[E]) => void): Unsubscribe { const set = listeners.get(event) ?? new Set(); set.add(cb as any); listeners.set(event, set); - return () => set.delete(cb as any); + return () => { + set.delete(cb as any); + if (set.size === 0) { + listeners.delete(event); + } + }; }
♻️ Duplicate comments (2)
docs/templates/pr-action-items-template.md (2)
4-11: Security rule added for PII/secret redaction — good upgradeThe explicit prohibition of secrets/PII (including MRZ) is a necessary safeguard given MRZ handling in this repo.
25-31: Final redaction pass added — aligns with least-exposure best practicesMaking redaction a mandatory last step lowers the risk of secret/PII leakage in artifacts.
🧹 Nitpick comments (6)
docs/templates/pr-action-items-template.md (3)
10-10: Broaden sensitive-data examples and define a placeholder formatExpand the examples to explicitly include biometric data and passport/document numbers, and standardize the redaction placeholder to avoid ambiguity.
- ✅ DO NOT include secrets, access tokens, API keys, private keys, MRZ data, or any PII. Redact sensitive values and replace with placeholders. + ✅ DO NOT include secrets, access tokens, API keys, private keys, MRZ data, biometric data (face images, fingerprints), passport/document numbers, or any PII. Redact sensitive values and replace with placeholders (e.g., <REDACTED>).
32-37: Add fallback if giga_read_pr is unavailableNot all environments will have giga_read_pr. Provide a supported fallback to fetch PR data/comments without exposing tokens.
5. EXECUTION: - - Use giga_read_pr to fetch PR data and CodeRabbit comments + - Use giga_read_pr to fetch PR data and CodeRabbit comments + - If giga_read_pr is unavailable in this repo/toolchain, fall back to the GitHub CLI (gh) or REST API to fetch PR comments. Never print or store access tokens in logs or artifacts.
180-188: Add an explicit checklist item for the final redaction checkReinforces the security gate at the point of sign-off.
**BEFORE FINALIZING - VERIFY EACH ITEM:** - [ ] ✅ Read ALL {{TOTAL_COMMENTS}} CodeRabbit comments thoroughly - [ ] ✅ Checked each comment for "✅ Addressed" status - [ ] ✅ Excluded all nitpick/style/documentation-only suggestions - [ ] ✅ Only included MEDIUM+ severity issues (security, architecture, functionality) - [ ] ✅ Verified unresolved count is accurate - [ ] ✅ If 0 unresolved issues, used "All issues resolved" template - [ ] ✅ Double-checked that each "unresolved" issue is actually unresolved + - [ ] ✅ Final redaction check complete (no PII/secrets/MRZ/credentials in code, logs, or screenshots)packages/mobile-sdk-alpha/src/components/screens/PassportCameraScreen.tsx (2)
12-31: Align MRZ flow with the client-based parsing API; avoid emitting pre-parsed MRZInfo from the UI layer.The PR’s core change is to expose MRZ parsing via SelfClient. Having this screen emit a fully parsed MRZInfo object hard-codes parsing at the UI layer and bypasses the client API, increasing drift risk and duplicating responsibilities.
Recommended:
- Prefer emitting raw MRZ text/lines from the camera layer and let consumers (or this component via the client context) call client.extractMRZInfo(raw). This keeps API boundaries clear and future parsing fixes centralized.
- At minimum, derive MRZInfo here by calling the client’s extractMRZInfo on a raw MRZ sample, not by constructing the object manually. Also confirm field names match whatever extractMRZInfo returns to prevent subtle type/shape divergence (e.g., passportNumber vs documentNumber).
Proposed refactor (Option B: parse via client inside the screen):
@@ -export const PassportCameraScreen = ({ onMRZDetected }: PassportCameraProps) => ( - <YStack space="$4" padding="$4"> +export const PassportCameraScreen = ({ onMRZDetected }: PassportCameraProps) => { + const client = useSelfClient(); + return ( + <YStack space="$4" padding="$4"> @@ - <Button - onPress={() => - onMRZDetected({ - passportNumber: 'L898902C3', - dateOfBirth: '740812', - dateOfExpiry: '120415', - surname: 'ERIKSSON', - givenNames: 'ANNA MARIA', - sex: 'F', - nationality: 'UTO', - issuingCountry: 'UTO', - documentType: 'P', - validation: { - format: true, - passportNumberChecksum: true, - dateOfBirthChecksum: true, - dateOfExpiryChecksum: true, - compositeChecksum: true, - overall: true, - }, - }) - } - > + <Button + onPress={() => { + // ICAO TD3 sample MRZ (2-line) + const mrz = 'P<UTOERIKSSON<<ANNA<MARIA<<<<<<<<<<<<<<<<<<<\nL898902C36UTO7408122F1204159ZE184226B<<<<<10'; + onMRZDetected(client.extractMRZInfo(mrz)); + }} + > @@ - </YStack> -); + </YStack> + ); +}Outside this hunk, import the hook (adjust the path to your actual context export):
import { useSelfClient } from '../../context/useSelfClient';Alternatively (Option A, often cleaner for UI purity): change PassportCameraProps to onMRZDetected: (rawMrz: string | { line1: string; line2: string; line3?: string }) => void and parse in the parent using client.extractMRZInfo.
Please confirm which direction you prefer so the public UI API is consistent with the new client-centric parsing.
5-7: Add PII guardrails to the placeholder; avoid shipping simulation UX to prod unchanged.MRZ contains sensitive PII. Even as a placeholder, make the constraints explicit and keep the simulator behind a dev-only guard to prevent accidental exposure or analytics capture downstream.
Suggested comment and guard:
-// Simple placeholder component - this would be replaced with actual camera UI +// Placeholder camera UI — replace with a real camera scanner. +// Security: MRZ contains sensitive PII. Do not log, persist, or send raw MRZ to analytics. +// Prefer gating the simulation behind a dev-only flag.If feasible, render the “Simulate MRZ Detection” button only in development or when an explicit prop is provided by the host app (e.g., enableSimulation). This helps prevent accidental usage in production builds.
packages/mobile-sdk-alpha/src/client.ts (1)
117-126: Reconsider exposingemiton the public clientThe
SelfClientinterface inpackages/mobile-sdk-alpha/src/types/public.ts(lines 133–135) declares bothon<E extends SDKEvent>(…): Unsubscribe; emit<E extends SDKEvent>(…): void;and the factory in
packages/mobile-sdk-alpha/src/client.ts(lines 117–126) returns both methods. Exposingemitlets consumers inject arbitrary events, risking state-machine integrity and noisy or spurious errors.• If external code doesn’t need to trigger SDK events, remove
emitfrom:
– The returned object insrc/client.ts
– The publicSelfClientinterface insrc/types/public.tsProposed diff in
src/client.ts:return { scanDocument, validateDocument, checkRegistration, registerDocument, generateProof, extractMRZInfo: parseMRZInfo, on, - emit, };
📜 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 (7)
docs/templates/pr-action-items-template.md(4 hunks)packages/mobile-sdk-alpha/src/client.ts(5 hunks)packages/mobile-sdk-alpha/src/components/flows/OnboardingFlow.tsx(1 hunks)packages/mobile-sdk-alpha/src/components/screens/PassportCameraScreen.tsx(1 hunks)packages/mobile-sdk-alpha/src/context.tsx(1 hunks)packages/mobile-sdk-alpha/src/types/ui.ts(1 hunks)packages/mobile-sdk-alpha/tests/provider.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/mobile-sdk-alpha/src/context.tsx
- packages/mobile-sdk-alpha/tests/provider.test.tsx
- packages/mobile-sdk-alpha/src/components/flows/OnboardingFlow.tsx
- packages/mobile-sdk-alpha/src/types/ui.ts
🧰 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/components/screens/PassportCameraScreen.tsxpackages/mobile-sdk-alpha/src/client.ts
🧬 Code Graph Analysis (2)
packages/mobile-sdk-alpha/src/components/screens/PassportCameraScreen.tsx (2)
packages/mobile-sdk-alpha/src/index.ts (2)
PassportCameraScreen(65-65)PassportCameraProps(36-36)packages/mobile-sdk-alpha/src/types/ui.ts (1)
PassportCameraProps(44-46)
packages/mobile-sdk-alpha/src/client.ts (1)
packages/mobile-sdk-alpha/src/types/public.ts (2)
RegistrationInput(83-86)RegistrationStatus(88-91)
🪛 LanguageTool
docs/templates/pr-action-items-template.md
[grammar] ~58-~58: There might be a mistake here.
Context: ... ({{TOTAL_COMMENTS}}/{{TOTAL_COMMENTS}}) All comments show ✅ "Addressed" status, ...
(QB_NEW_EN)
[grammar] ~59-~59: There might be a mistake here.
Context: ..., indicating they were fixed in commits: - {{RESOLVED_CATEGORY_1}} - Fixed in commi...
(QB_NEW_EN)
[grammar] ~60-~60: There might be a mistake here.
Context: ..._1}} - Fixed in commits {{COMMIT_RANGE}} - {{RESOLVED_CATEGORY_2}} - Fixed in commi...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...ed Comments 🔴 (0/{{TOTAL_COMMENTS}}) None - All comments have been addressed...
(QB_NEW_EN)
[grammar] ~64-~64: There might be a mistake here.
Context: ...ne** - All comments have been addressed. ## Conclusion **This PR is ready for merge.*...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ...ents have been addressed. ## Conclusion This PR is ready for merge. All CodeRa...
(QB_NEW_EN)
[grammar] ~75-~75: There might be a mistake here.
Context: ...erity issues --> ### 1. {{ISSUE_TITLE}} Files: {{FILE_PATH}}:{{LINE_NUMBER}}...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ...Files:** {{FILE_PATH}}:{{LINE_NUMBER}} CodeRabbit Comment: {{COMMENT_ID}} *...
(QB_NEW_EN)
[grammar] ~77-~77: There might be a mistake here.
Context: ...` CodeRabbit Comment: {{COMMENT_ID}} Problem: {{PROBLEM_DESCRIPTION}} **F...
(QB_NEW_EN)
[grammar] ~78-~78: There might be a mistake here.
Context: ...D}} Problem: {{PROBLEM_DESCRIPTION}} Fix: {{SPECIFIC_FIX_OR_ACTION}} **C...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ...DE_EXAMPLE}} ``` ### 2. {{ISSUE_TITLE}} Files: {{FILE_PATH}}:{{LINE_NUMBER}}...
(QB_NEW_EN)
[grammar] ~87-~87: There might be a mistake here.
Context: ...Files:** {{FILE_PATH}}:{{LINE_NUMBER}} CodeRabbit Comment: {{COMMENT_ID}} *...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...` CodeRabbit Comment: {{COMMENT_ID}} Problem: {{PROBLEM_DESCRIPTION}} **F...
(QB_NEW_EN)
[grammar] ~89-~89: There might be a mistake here.
Context: ...D}} Problem: {{PROBLEM_DESCRIPTION}} Fix: {{SPECIFIC_FIX_OR_ACTION}} ## ...
(QB_NEW_EN)
[grammar] ~94-~94: There might be a mistake here.
Context: ...ns ### Issue 1: {{GROUPED_ISSUE_TITLE}} Root Cause: {{ROOT_CAUSE_DESCRIPTION}}...
(QB_NEW_EN)
[grammar] ~95-~95: There might be a mistake here.
Context: ...Root Cause:** {{ROOT_CAUSE_DESCRIPTION}} Files Affected: - `{{FILE_PATH}}:{{L...
(QB_NEW_EN)
[grammar] ~115-~115: There might be a mistake here.
Context: ...R}}` - {{ISSUE_DESCRIPTION}} Actions: - [ ] {{SPECIFIC_ACTION_1}} - [ ] {{SPECIF...
(QB_NEW_EN)
[grammar] ~116-~116: There might be a mistake here.
Context: ...Actions: - [ ] {{SPECIFIC_ACTION_1}} - [ ] {{SPECIFIC_ACTION_2}} ## CodeRabbit...
(QB_NEW_EN)
[grammar] ~121-~121: There might be a mistake here.
Context: ...nalysis Summary ### Resolved Comments ✅ - {{RESOLVED_COMMENT_1}} - {{RESOLVED_COMM...
(QB_NEW_EN)
[grammar] ~122-~122: There might be a mistake here.
Context: ...lved Comments ✅ - {{RESOLVED_COMMENT_1}} - {{RESOLVED_COMMENT_2}} ### Unresolved C...
(QB_NEW_EN)
[grammar] ~126-~126: There might be a mistake here.
Context: ...MMENT_2}} ### Unresolved Comments 🔴 - {{UNRESOLVED_COMMENT_1}} - {{STATUS}} - {...
(QB_NEW_EN)
[grammar] ~127-~127: There might be a mistake here.
Context: ...{{UNRESOLVED_COMMENT_1}} - {{STATUS}} - {{UNRESOLVED_COMMENT_2}} - {{STATUS}} ##...
(QB_NEW_EN)
[grammar] ~127-~127: There might be a mistake here.
Context: ... - {{UNRESOLVED_COMMENT_2}} - {{STATUS}} ### Comment Categories - Critical: {{COUN...
(QB_NEW_EN)
[grammar] ~129-~129: There might be a mistake here.
Context: ...2}} - {{STATUS}} ### Comment Categories - Critical: {{COUNT}} comments (React Na...
(QB_NEW_EN)
[grammar] ~130-~130: There might be a mistake here.
Context: ...e compatibility, security, memory leaks) - Architecture: {{COUNT}} comments (API ...
(QB_NEW_EN)
[grammar] ~131-~131: There might be a mistake here.
Context: ...UNT}} comments (API design, type safety) - Code Quality: {{COUNT}} comments (test...
(QB_NEW_EN)
[grammar] ~180-~180: There might be a mistake here.
Context: ... BEFORE FINALIZING - VERIFY EACH ITEM: - [ ] ✅ Read ALL {{TOTAL_COMMENTS}} CodeRa...
(QB_NEW_EN)
[grammar] ~189-~189: There might be a mistake here.
Context: ...lly unresolved SEVERITY VERIFICATION: - [ ] ✅ CRITICAL: Security, memory leaks, ...
(QB_NEW_EN)
[grammar] ~190-~190: There might be a mistake here.
Context: ...tform compatibility ({{CRITICAL_COUNT}}) - [ ] ✅ HIGH: API inconsistencies, type sa...
(QB_NEW_EN)
[grammar] ~191-~191: There might be a mistake here.
Context: ...pe safety, architecture ({{HIGH_COUNT}}) - [ ] ✅ MEDIUM: Test coverage, performance...
(QB_NEW_EN)
[grammar] ~192-~192: There might be a mistake here.
Context: ...e, minor architecture ({{MEDIUM_COUNT}}) - [ ] ❌ LOW/NITPICK: Style, naming, docs (...
(QB_NEW_EN)
[grammar] ~197-~197: There might be a mistake here.
Context: ...OUNT}}) --- Last Updated: {{DATE}} CodeRabbit Comments Analyzed: {{TOTA...
(QB_NEW_EN)
[grammar] ~198-~198: There might be a mistake here.
Context: ... Comments Analyzed:** {{TOTAL_COMMENTS}} Unresolved Medium+ Issues: {{UNRESOL...
(QB_NEW_EN)
[grammar] ~199-~199: There might be a mistake here.
Context: ...d Medium+ Issues:** {{UNRESOLVED_COUNT}} Excluded Low/Nitpick Issues: {{EXCLU...
(QB_NEW_EN)
[grammar] ~200-~200: There might be a mistake here.
Context: ...Low/Nitpick Issues:** {{EXCLUDED_COUNT}}
(QB_NEW_EN)
🔇 Additional comments (2)
packages/mobile-sdk-alpha/src/components/screens/PassportCameraScreen.tsx (1)
1-3: Good move to cross‑platform UI primitives; resolves RN incompatibility.Switching to Tamagui components avoids web DOM elements and keeps this screen usable in React Native and web. This addresses the earlier platform issue.
packages/mobile-sdk-alpha/src/client.ts (1)
93-96: Tests assert stubbed reason as expectedThe existing test in packages/mobile-sdk-alpha/tests/client.test.ts (lines 91–94) already verifies that registerDocument resolves to:
{ registered: false, reason: 'SELF_REG_STATUS_STUB' }No further action required.
|
|
||
| export { SCANNER_ERROR_CODES, notImplemented, sdkError } from './errors'; | ||
| export { SelfClientContext, SelfClientProvider, useSelfClient } from './context'; | ||
| // Browser-only high-level component (DOM-based) |
There was a problem hiding this comment.
ah. this isnt true theres nothing about SelfMobileSdk that is browser or dom based. it supposed to be react native based
Summary
Testing
yarn workspace @selfxyz/mobile-sdk-alpha niceyarn workspace @selfxyz/mobile-sdk-alpha buildyarn workspace @selfxyz/mobile-sdk-alpha typesyarn workspace @selfxyz/mobile-sdk-alpha testhttps://chatgpt.com/codex/tasks/task_b_68a4c6fd1374832dbf15efdda66b2fad
Summary by CodeRabbit
New Features
Refactor
Tests
Chores