feat(webview): route document storage through the documents bridge domain - #2264
feat(webview): route document storage through the documents bridge domain#2264seshanthS wants to merge 2 commits into
Conversation
…main
webview-app previously tunneled document JSON through secureStorage
(self_document_catalog / self_doc_* keys), bypassing rn-sdk's dedicated
DocumentsHandler and creating a parallel store that the WIA cutover would
have had to migrate. Documents now go over the documents domain, so hosts
control storage: the Self app's WIA host already implements the documents
prop against its production keychain store (documentCatalog /
document-<hash>), which means WebView document I/O lands directly in the
app's real store — no document migration at cutover (B2, documents half).
- createBridgeDocumentsSdkAdapter: typed SDK-facing wrapper over the raw
documents-domain adapter; normalizes null/malformed catalogs to
{ documents: [] } (SDK callers dereference catalog.documents unguarded;
a never-written host store legitimately returns null).
- sdk-adapter-map + SelfClientProvider use it; the second per-provider
adapter instantiation is gone; createKeychainDocumentsAdapter deleted.
- KeychainDebugScreen dumps the catalog via the documents adapter instead
of the removed secureStorage key.
- Test harness gains documents-domain handlers mirroring DocumentsHandler
semantics; passport happy-path now asserts the captured document lands
in the documents store (isRegistered: false); catalog fixture test pins
the app's DocumentCatalog shape through normalization.
No migration of orphaned self_doc_* secureStorage entries (dev-only data).
Spec: specs/projects/sdk/workstreams/webview-in-app/plans/WIA-documents-bridge-domain.md
Validation: webview-bridge build + 72 tests, webview-app build + 245 tests green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe webview now obtains document storage from the typed bridge documents adapter. Keychain-backed document storage was removed. Passport onboarding stores normalized data through ChangesDocuments adapter migration
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6063e67d7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| network: createWebNetworkAdapter(), | ||
| auth, | ||
| documents: createKeychainDocumentsAdapter(bridge), | ||
| documents: createBridgeDocumentsSdkAdapter(bridge), |
There was a problem hiding this comment.
When the WebView is hosted by the Self app, this now writes the catalog directly into the app's production document store, but the passport and EU-ID capture routes still create IDs such as passport-<passportNumber> and id_card-<documentNumber>. The shared DocumentMetadata contract defines IDs as content hashes, and the native storeDocumentWithDeduplication path searches by a freshly calculated content hash, so capturing the same document later through the native path misses the WebView entry and creates a duplicate. Route capture through the existing mobile-sdk deduplication utility, or otherwise calculate the canonical content hash before exposing these records to the host catalog.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| ) { | ||
| return raw as DocumentCatalog; | ||
| } | ||
| return EMPTY_CATALOG; |
There was a problem hiding this comment.
When a host returns null or malformed catalog data, this returns the module-level EMPTY_CATALOG object, but capture callers mutate the returned documents array in place before saving it. If saveDocumentCatalog fails, or a subsequent host read is still unset/corrupt, later loads return the previously mutated fallback and expose stale document metadata instead of failing closed to an empty catalog; this can select a document body that was never successfully persisted. Construct a new { documents: [] } for each normalization failure rather than sharing a mutable singleton.
AGENTS.md reference: AGENTS.md:L36-L36
Useful? React with 👍 / 👎.
| network: createWebNetworkAdapter(), | ||
| auth, | ||
| documents: createKeychainDocumentsAdapter(bridge), | ||
| documents: createBridgeDocumentsSdkAdapter(bridge), |
There was a problem hiding this comment.
When this WebView bundle runs in either standalone native shell, every document request now fails: packages/native-shell-android/.../SelfVerificationActivity.kt registers only secure-storage, crypto, and lifecycle handlers, and packages/native-shell-ios/.../SelfSdk.swift does the same, while scripts/build-webview-bundle.sh copies this WebView into both shells. Consequently the first documents.save during NFC capture receives DOMAIN_NOT_FOUND/UNKNOWN_DOMAIN and the successfully scanned user is routed to the NFC error screen. Register a compatible documents handler in those hosts before making the domain mandatory, or retain a compatible storage path for them.
Useful? React with 👍 / 👎.
| Array.isArray((raw as { documents?: unknown }).documents) | ||
| ) { | ||
| return raw as DocumentCatalog; |
There was a problem hiding this comment.
A host response such as { documents: [null] }, or an array containing entries without a string id, passes this check and is cast to DocumentCatalog. SDK consumers then dereference fields such as doc.id and doc.isRegistered, causing runtime failures or persisting an invalid selectedDocumentId instead of using the intended empty fallback. Since this adapter explicitly treats malformed host catalogs as a boundary failure, validate the entry shapes and optional selected ID before returning the value.
AGENTS.md reference: AGENTS.md:L36-L36
Useful? React with 👍 / 👎.
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e34b0280-612c-41b2-814a-6afed0fdb3b3
📒 Files selected for processing (9)
packages/webview-app/src/providers/SelfClientProvider.tsxpackages/webview-app/src/screens/debug/KeychainDebugScreen.tsxpackages/webview-app/tests/flows/onboarding.passport.test.tsxpackages/webview-app/tests/utils/renderWithBridge.tsxpackages/webview-bridge/src/__tests__/adapters.test.tspackages/webview-bridge/src/adapters/documents.tspackages/webview-bridge/src/adapters/index.tspackages/webview-bridge/src/adapters/keychain-documents.tspackages/webview-bridge/src/adapters/sdk-adapter-map.ts
💤 Files with no reviewable changes (1)
- packages/webview-bridge/src/adapters/keychain-documents.ts
| const EMPTY_CATALOG: DocumentCatalog = { documents: [] }; | ||
|
|
||
| // The documents host store legitimately returns null for a never-written | ||
| // catalog (DocumentsHandler contract), but SDK callers dereference | ||
| // catalog.documents unguarded — normalize anything malformed to an empty | ||
| // catalog at the boundary. | ||
| function normalizeCatalog(raw: unknown): DocumentCatalog { | ||
| if ( | ||
| raw && | ||
| typeof raw === 'object' && | ||
| Array.isArray((raw as { documents?: unknown }).documents) | ||
| ) { | ||
| return raw as DocumentCatalog; | ||
| } | ||
| return EMPTY_CATALOG; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return a new empty catalog for each read.
EMPTY_CATALOG is mutable. Each null or malformed host response returns the same object. A caller can mutate catalog.documents, and later reads can return that process-local mutation even when the host store remains empty or a save failed. Return a new { documents: [] } fallback from normalizeCatalog.
Proposed fix
-const EMPTY_CATALOG: DocumentCatalog = { documents: [] };
-
function normalizeCatalog(raw: unknown): DocumentCatalog {
if (
raw &&
typeof raw === 'object' &&
Array.isArray((raw as { documents?: unknown }).documents)
) {
return raw as DocumentCatalog;
}
- return EMPTY_CATALOG;
+ return { documents: [] };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const EMPTY_CATALOG: DocumentCatalog = { documents: [] }; | |
| // The documents host store legitimately returns null for a never-written | |
| // catalog (DocumentsHandler contract), but SDK callers dereference | |
| // catalog.documents unguarded — normalize anything malformed to an empty | |
| // catalog at the boundary. | |
| function normalizeCatalog(raw: unknown): DocumentCatalog { | |
| if ( | |
| raw && | |
| typeof raw === 'object' && | |
| Array.isArray((raw as { documents?: unknown }).documents) | |
| ) { | |
| return raw as DocumentCatalog; | |
| } | |
| return EMPTY_CATALOG; | |
| // The documents host store legitimately returns null for a never-written | |
| // catalog (DocumentsHandler contract), but SDK callers dereference | |
| // catalog.documents unguarded — normalize anything malformed to an empty | |
| // catalog at the boundary. | |
| function normalizeCatalog(raw: unknown): DocumentCatalog { | |
| if ( | |
| raw && | |
| typeof raw === 'object' && | |
| Array.isArray((raw as { documents?: unknown }).documents) | |
| ) { | |
| return raw as DocumentCatalog; | |
| } | |
| return { documents: [] }; | |
| } |
🧰 Tools
🪛 GitHub Check: lint
[warning] 48-48:
Replace ⏎····raw·&&⏎····typeof·raw·===·'object'·&&⏎····Array.isArray((raw·as·{·documents?:·unknown·}).documents)⏎·· with raw·&&·typeof·raw·===·'object'·&&·Array.isArray((raw·as·{·documents?:·unknown·}).documents)
Capture routes keyed documents by document number (passport-<number> / id_card-<number>), which is PII: keychain service names derived from the id appear verbatim in native logs (RNKeychainManager logs missing services), and number-based ids collide across document types. Replace the hand-rolled save + catalog upsert in NfcRoute and NfcInstructionsRoute with the SDK's storePassportData, which computes calculateContentHash ids (matching the app's native document-<hash> scheme), dedups re-captures, and writes richer catalog metadata (data: mrz for name display). Flow test pins that the id is a content hash, never a document number. Validation: webview-app build + 245 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Stacked on #2263 (→ #2262 → #2245). PR 2 of 2 for the documents-domain swap — spec:
specs/projects/sdk/workstreams/webview-in-app/plans/WIA-documents-bridge-domain.md.What
webview-app previously tunneled document JSON through the
secureStoragebridge domain (self_document_catalog/self_doc_*), bypassing rn-sdk's dedicatedDocumentsHandlerand creating a parallel document store. Documents now go over thedocumentsdomain, so the host controls storage:WebViewHostScreenalready implements thedocumentsprop against the app's production keychain store (documentCatalog/document-<contentHash>viapassportDataProvider) — WebView document I/O now lands directly in the app's real store, eliminating the documents half of cutover blocker B2 (no document migration; existing users' documents just appear).How
createBridgeDocumentsSdkAdapter(bridge): DocumentsAdapter— typed wrapper over the rawbridgeDocumentsAdapter; normalizesnull/malformed catalogs to{ documents: [] }(a never-written host store legitimately returnsnullper theDocumentsHandlercontract, but SDK callers dereferencecatalog.documentsunguarded).sdk-adapter-mapandSelfClientProviderboth use it; the provider's second per-instance adapter is gone (one construction site);createKeychainDocumentsAdapterdeleted.KeychainDebugScreendumps the catalog via the documents adapter;self_document_catalogremoved from its raw key list.renderWithBridge) gainsdocuments-domain handlers mirroringDocumentsHandlersemantics, with seeding + exposure on the result.Also in this PR
passport-<number>), which is PII — keychain service names derived from the id appear verbatim in native logs (RNKeychainManager logs missing services). Both routes now use the SDK'sstorePassportData(contentHash ids matching the app's nativedocument-<hash>scheme, dedup, richer catalog metadata). Pinned by a flow test asserting the id never contains a document number.Decisions / risks
self_doc_*secureStorage entries — dev-only data, nothing production shipped on that path.useKmpBridge(prototype) reroutes onlysecureStorage; documents traffic always hits the TS handler. KMP documents parity is a noted follow-up.Validation
DocumentCatalogfixture pin — the B2 schema-compatibility check — and domain routing).isRegistered: false.createKeychainDocumentsAdapter/self_document_catalog/self_doc_references.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes