Deduplicate bridge-layer fallback adapters onto engine-owned browser implementations (SC-01)#1841
Merged
transphorm merged 6 commits intodevfrom Mar 11, 2026
Merged
Deduplicate bridge-layer fallback adapters onto engine-owned browser implementations (SC-01)#1841transphorm merged 6 commits intodevfrom
transphorm merged 6 commits intodevfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralizes browser adapter implementations into the mobile SDK, adds a new Changes
Sequence DiagramsequenceDiagram
participant App as App
participant WebBridge as WebViewBridge
participant MobileSDK as MobileSDK\n(Browser Adapters)
participant BrowserAPI as Browser APIs
App->>WebBridge: request adapter (documents / crypto / analytics / haptic)
WebBridge->>MobileSDK: call create*Adapter() wrapper
MobileSDK->>BrowserAPI: use IndexedDB / WebCrypto / fetch / other APIs
BrowserAPI-->>MobileSDK: result
MobileSDK-->>WebBridge: adapter instance / result
WebBridge-->>App: return adapter / result
App->>WebBridge: adapter.method(args)
WebBridge->>MobileSDK: delegate method call
MobileSDK->>BrowserAPI: perform operation (cloneForStorage, hash, persist, etc.)
BrowserAPI-->>MobileSDK: operation result
MobileSDK-->>WebBridge: result
WebBridge-->>App: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
specs/projects/sdk/workstreams/sdk-core/plans/SC-01-fallback-adapter-dedup.md (1)
3-4: Status updated correctly; consider filling in Branch/PR fields.The status and Definition of Done updates accurately reflect the completed consolidation work. Minor nit: Lines 9-10 still show "TBD" for Branch and PR while Status is "Done". Consider updating these to reflect the actual PR number (1841) and branch (
justin/kmp-sc-01) for traceability.Also applies to: 9-10
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/projects/sdk/workstreams/sdk-core/plans/SC-01-fallback-adapter-dedup.md` around lines 3 - 4, Update the metadata fields in the SC-01-fallback-adapter-dedup.md header: replace "Branch: TBD" with "Branch: justin/kmp-sc-01" and "PR: TBD" with "PR: 1841" so the document shows the actual branch and PR used for the completed work.packages/webview-bridge/src/adapters/index.ts (1)
60-67: Consider clarifying thetype as nevercast.The
noOpHapticAdapterwrapper works correctly, but thetype as nevercast on line 65 is unconventional. The SDK'screateNoOpHapticAdapterreturns a function typed as(type: HapticFeedbackTypes) => void, so casting toneversilences TypeScript when the bridge passes arbitrary strings.This is functionally correct for a no-op, but an inline comment explaining the intent would aid maintainability.
🔧 Optional: Add clarifying comment
export function noOpHapticAdapter(): BridgeHapticAdapter { const trigger = createNoOpHapticAdapter(); return { trigger(type: string): void { + // Cast to `never` since the no-op ignores the type anyway trigger(type as never); }, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webview-bridge/src/adapters/index.ts` around lines 60 - 67, The `type as never` cast in noOpHapticAdapter's returned trigger is unclear; update the noOpHapticAdapter (and its returned trigger function) to keep the existing harmless cast but add a brief inline comment explaining that createNoOpHapticAdapter returns (type: HapticFeedbackTypes) => void and we intentionally cast arbitrary bridge strings to never because this is a true no-op and we intentionally ignore runtime values; reference the functions createNoOpHapticAdapter and noOpHapticAdapter and the trigger method in your comment so future maintainers understand the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/webview-bridge/src/adapters/index.ts`:
- Around line 60-67: The `type as never` cast in noOpHapticAdapter's returned
trigger is unclear; update the noOpHapticAdapter (and its returned trigger
function) to keep the existing harmless cast but add a brief inline comment
explaining that createNoOpHapticAdapter returns (type: HapticFeedbackTypes) =>
void and we intentionally cast arbitrary bridge strings to never because this is
a true no-op and we intentionally ignore runtime values; reference the functions
createNoOpHapticAdapter and noOpHapticAdapter and the trigger method in your
comment so future maintainers understand the rationale.
In
`@specs/projects/sdk/workstreams/sdk-core/plans/SC-01-fallback-adapter-dedup.md`:
- Around line 3-4: Update the metadata fields in the
SC-01-fallback-adapter-dedup.md header: replace "Branch: TBD" with "Branch:
justin/kmp-sc-01" and "PR: TBD" with "PR: 1841" so the document shows the actual
branch and PR used for the completed work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b9f1d94-0339-4827-8d11-833ed7dd409a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
packages/mobile-sdk-alpha/package.jsonpackages/mobile-sdk-alpha/src/adapters/browser/documents.tspackages/mobile-sdk-alpha/tests/adapters/browser/documents.test.tspackages/mobile-sdk-alpha/tsup.config.tspackages/webview-bridge/package.jsonpackages/webview-bridge/src/__tests__/adapters.test.tspackages/webview-bridge/src/__tests__/analytics-web.test.tspackages/webview-bridge/src/__tests__/documents-web.test.tspackages/webview-bridge/src/adapters/analytics-web.tspackages/webview-bridge/src/adapters/crypto.tspackages/webview-bridge/src/adapters/documents-web.tspackages/webview-bridge/src/adapters/haptic.tspackages/webview-bridge/src/adapters/index.tsspecs/projects/sdk/workstreams/sdk-core/SPEC.mdspecs/projects/sdk/workstreams/sdk-core/plans/SC-01-fallback-adapter-dedup.md
💤 Files with no reviewable changes (3)
- packages/webview-bridge/src/adapters/haptic.ts
- packages/webview-bridge/src/adapters/analytics-web.ts
- packages/webview-bridge/src/adapters/documents-web.ts
transphorm
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/webview-bridge-ci.yml (1)
44-52:⚠️ Potential issue | 🟠 MajorPotential inconsistency:
testjob may also need the SDK build step.The
buildandtypesjobs now buildmobile-sdk-alphafirst, but thetestjob does not. Ifwebview-bridgetests import from@selfxyz/mobile-sdk-alpha(e.g., the newadapters/browserexports), tests could fail without the SDK being built.Consider adding the same build step for consistency:
test: runs-on: ubuntu-latest timeout-minutes: 15 steps: - uses: actions/checkout@v6 - name: Install dependencies uses: ./.github/actions/yarn-install + - name: Build mobile-sdk-alpha + run: yarn workspace `@selfxyz/mobile-sdk-alpha` build - name: Test run: yarn workspace `@selfxyz/webview-bridge` test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/webview-bridge-ci.yml around lines 44 - 52, The test job currently runs yarn workspace `@selfxyz/webview-bridge` test without building the SDK, which can cause imports from `@selfxyz/mobile-sdk-alpha` to fail; add a step in the test job's steps to build the SDK first (e.g., run yarn workspace `@selfxyz/mobile-sdk-alpha` build or the same build command used in the build/types jobs) before running yarn workspace `@selfxyz/webview-bridge` test so tests have the latest SDK artifacts available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/webview-bridge-ci.yml:
- Around line 44-52: The test job currently runs yarn workspace
`@selfxyz/webview-bridge` test without building the SDK, which can cause imports
from `@selfxyz/mobile-sdk-alpha` to fail; add a step in the test job's steps to
build the SDK first (e.g., run yarn workspace `@selfxyz/mobile-sdk-alpha` build or
the same build command used in the build/types jobs) before running yarn
workspace `@selfxyz/webview-bridge` test so tests have the latest SDK artifacts
available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 926e79a8-8843-4351-afc3-190789a18035
📒 Files selected for processing (3)
.github/workflows/webview-app-ci.yml.github/workflows/webview-bridge-ci.ymlpackages/mobile-sdk-alpha/src/data/country-document-types.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/webview-app-ci.yml:
- Around line 29-32: The workflow currently runs steps "Build common" and "Build
mobile-sdk-alpha" but the workflow's path filters (pull_request.paths and
push.paths) still exclude the corresponding directories, so changes in common/
or packages/mobile-sdk-alpha/ won't trigger the CI; update the path filters in
.github/workflows/webview-app-ci.yml to include the new upstream dependency
paths (e.g., common/** and packages/mobile-sdk-alpha/**) for both
pull_request.paths and push.paths (and any other path include/exclude blocks
referenced in the file) so that changes to those directories will run the
workflow that executes the "Build common" and "Build mobile-sdk-alpha" steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd9ca5d3-1970-4158-abdf-ea3564abe59e
📒 Files selected for processing (4)
.github/workflows/webview-app-ci.yml.github/workflows/webview-bridge-ci.ymlpackages/mobile-sdk-alpha/tsup.config.tsscripts/check-license-headers.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/webview-bridge-ci.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mobile-sdk-alpha/adapters/browser
Test plan
Native Consolidation Checklist
cd app && yarn jest:run/yarn workspace @selfxyz/rn-sdk-test-app test)Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Other