Skip to content

Conversation

@transphorm
Copy link
Member

@transphorm transphorm commented Jan 29, 2026

Description

A brief description of the changes, what and how is being changed.

Tested

Explain how the change has been tested (for example by manual testing, unit tests etc) or why it's not necessary (for example version bump).

How to QA

How can the change be tested in a repeatable manner?

Summary by CodeRabbit

  • New Features

    • Bundled a complete country-to-document-type mapping for offline use.
  • Performance Improvements

    • Switched country data to the local bundled resource to reduce latency and improve reliability.
  • Behavioral Changes

    • Country data is now served from the bundled resource; loading is deterministic and no longer depends on runtime fetching.
  • Tests

    • Added an integration test to ensure the bundled mapping stays synchronized with the upstream source.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 29, 2026

📝 Walkthrough

Walkthrough

Bundles a new static country-document-types.json, refactors useCountries to consume the bundled data (removing async fetch and loading state), and adds an integration test that compares the bundled data against the staging API.

Changes

Cohort / File(s) Summary
Country Data Bundling
packages/mobile-sdk-alpha/src/data/country-document-types.json
Added comprehensive JSON mapping of ISO country codes to document type arrays (e.g., "p", "i").
Hook Refactor
packages/mobile-sdk-alpha/src/documents/useCountries.tsx
Replaced API-based country loading with a static import from the new JSON. Removed useEffect/useState and async fetching; loading now always false.
Integration Test
packages/mobile-sdk-alpha/tests/data/country-data-sync.integration.test.ts
New Vitest integration test comparing bundled JSON to staging API response with timeout, network-error skips, and selective handling of 4xx/5xx responses.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • MOVING COUNTRIES:  #1229: Modifies the same useCountries implementation and is directly related to replacing/factoring API-driven country logic with bundled data.

Suggested labels

codex

Suggested reviewers

  • shazarre

Poem

📦 Countries packed, no calls to send,
Local maps now guide each document friend,
A test keeps watch across the network sea,
Static, swift, and tidy — small victory! 🌍

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description contains only an empty template with no actual content describing the changes, testing approach, or QA steps. Fill in all template sections: explain what changed (static JSON asset migration), how it was tested (integration test details), and QA steps for validation.
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding country document information as a static JSON asset instead of fetching dynamically.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch justin/add-countries-json-file-to-app

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 021754cd0e

ℹ️ 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".

"CYM": ["p", "i"],
"CYP": ["p", "i"],
"CZE": ["p", "i"],
"D<<": ["p", "i"],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use ISO-3 key for Germany in country data

The new static dataset uses the MRZ code D<< for Germany, but useCountries derives userCountryCode via alpha2ToAlpha3 (e.g., DEDEU) and then checks countryData[userCountryCode] to decide whether to show and filter the suggested country. Because there is no DEU entry in the dataset, German users won’t get a suggestion and Germany won’t be filtered out of the list. If the intent is to keep MRZ codes, the user country should be normalized to D<<; otherwise, add a DEU entry so userCountryCode can match.

Useful? React with 👍 / 👎.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are just matching the existing json data 1:1 and adding as an asset

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@packages/mobile-sdk-alpha/src/data/country-document-types.json`:
- Around line 61-75: This JSON mapping contains non‑ISO 3166‑1 alpha‑3 keys
(e.g. "D<<", "EUE", "UNO", "XCE", "XOM", "XPO") which break consumers expecting
ISO codes; remove those keys from the main country-document-types mapping or
move them into a clearly named separate mapping (e.g.
nonIsoCountryDocumentTypes) with explicit handling, and where possible replace
them with the correct ISO alpha‑3 codes (normalize entries) so consumers that
use the primary mapping can safely assume all keys are valid ISO codes.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@packages/mobile-sdk-alpha/tests/data/country-data-sync.test.ts`:
- Around line 16-28: The test performs a real network fetch to
'https://api.staging.self.xyz/id-picker' using controller and timeoutId which
makes CI flaky; replace the live call with a deterministic approach (preferably
mock the global fetch or inject a mocked client used in this test) or
alternatively mark the spec as an integration test / add retry or graceful skip
when network is unavailable, and remove the redundant clearTimeout(timeoutId)
inside the catch block (keep the single clearTimeout in the finally block) to
avoid duplicate cleanup; locate the fetch call, controller, and timeoutId in the
test to apply these changes.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@packages/mobile-sdk-alpha/tests/data/country-data-sync.integration.test.ts`:
- Around line 1-3: Prettier formatting is failing for the test file
packages/mobile-sdk-alpha/tests/data/country-data-sync.integration.test.ts; run
the project's formatter (e.g., yarn prettier --write
packages/mobile-sdk-alpha/tests/data/country-data-sync.integration.test.ts or
npx prettier --write <that file>) to reformat the file, verify the file is
updated and linted, then stage and commit the reformatted file so CI passes (no
code changes required in test logic such as in any test functions within
country-data-sync.integration.test.ts).
- Around line 16-19: Replace the relative JSON import with the src alias: change
the import that defines countryDocumentTypesData to use
"@/data/country-document-types.json" instead of the relative path, ensuring the
module specifier uses the project’s "@/..." resolver (and keep the vitest
imports as-is or use `@tests/` alias for internal test helpers if applicable).

Comment on lines +1 to +3
// SPDX-FileCopyrightText: 2025 Social Connect Labs, Inc.
// SPDX-License-Identifier: BUSL-1.1
// NOTE: Converts to Apache-2.0 on 2029-06-11 per LICENSE.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Prettier check is failing for this file.
CI reports a Prettier failure here; please format it before merge (e.g., yarn prettier --write packages/mobile-sdk-alpha/tests/data/country-data-sync.integration.test.ts).

🧰 Tools
🪛 GitHub Actions: Mobile SDK CI

[warning] 1-1: Code style issues found in tests/data/country-data-sync.integration.test.ts. Run Prettier with --write to fix.


[error] 1-1: Prettier formatting check failed for the workspace. Run 'yarn prettier --write' to fix code style issues.

🤖 Prompt for AI Agents
In `@packages/mobile-sdk-alpha/tests/data/country-data-sync.integration.test.ts`
around lines 1 - 3, Prettier formatting is failing for the test file
packages/mobile-sdk-alpha/tests/data/country-data-sync.integration.test.ts; run
the project's formatter (e.g., yarn prettier --write
packages/mobile-sdk-alpha/tests/data/country-data-sync.integration.test.ts or
npx prettier --write <that file>) to reformat the file, verify the file is
updated and linted, then stage and commit the reformatted file so CI passes (no
code changes required in test logic such as in any test functions within
country-data-sync.integration.test.ts).

Comment on lines +16 to +19
import { describe, expect, it } from 'vitest';

import countryDocumentTypesData from '../../src/data/country-document-types.json';

Copy link
Contributor

@coderabbitai coderabbitai bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use the @/ alias for the bundled data import.
Switch the JSON import to the src alias for consistency and path stability.

♻️ Suggested change
-import countryDocumentTypesData from '../../src/data/country-document-types.json';
+import countryDocumentTypesData from '@/data/country-document-types.json';

As per coding guidelines: Use @/ alias for src imports and @tests/ alias for test imports in TypeScript/JavaScript files.

🤖 Prompt for AI Agents
In `@packages/mobile-sdk-alpha/tests/data/country-data-sync.integration.test.ts`
around lines 16 - 19, Replace the relative JSON import with the src alias:
change the import that defines countryDocumentTypesData to use
"@/data/country-document-types.json" instead of the relative path, ensuring the
module specifier uses the project’s "@/..." resolver (and keep the vitest
imports as-is or use `@tests/` alias for internal test helpers if applicable).

Copy link
Member Author

@transphorm transphorm Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this applies to the /app path only and not the mobile sdk

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@transphorm, you're absolutely right—I apologize for the confusion. The @/ alias convention applies to the /app directory only, not to the packages/mobile-sdk-alpha package. The relative import you're using is correct for the mobile SDK structure.


✏️ Learnings added
Learnt from: transphorm
Repo: selfxyz/self PR: 1670
File: packages/mobile-sdk-alpha/tests/data/country-data-sync.integration.test.ts:16-19
Timestamp: 2026-01-30T18:25:54.862Z
Learning: The `@/` alias for src imports and `tests/` alias for test imports applies only to app/**/*.{ts,tsx,js,jsx} files, not to packages/mobile-sdk-alpha. The mobile SDK uses relative imports.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.test.{ts,tsx} : Test `isPassportDataValid()` with realistic, synthetic passport data (never use real user data)

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.test.{ts,tsx} : Ensure `parseNFCResponse()` works with representative, synthetic NFC data

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.test.{ts,tsx} : Write integration tests that exercise real validation logic from the SDK

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.test.{ts,tsx} : Avoid dynamic `require()` calls in `beforeEach`/`afterEach` hooks to prevent out-of-memory errors

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.test.{ts,tsx} : Verify `extractMRZInfo()` using published sample MRZ strings (e.g., ICAO examples)

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.test.{ts,tsx} : Avoid mocking the entire package in Jest setup or replacing real functions with mock implementations

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.test.{ts,tsx} : Never use real user PII (personally identifiable information) in tests - use only synthetic, anonymized, or approved test vectors

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.test.{ts,tsx} : Use Vitest for testing, not Jest, with setup configuration in `tests/setup.ts`

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.test.{ts,tsx} : React Native is mocked in `tests/setup.ts` using `vi.mock()` - use imports in test files instead of require()

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.test.{ts,tsx} : Use real imports from `selfxyz/mobile-sdk-alpha` in tests instead of mocking the entire package

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.test.{ts,tsx} : Use ES6 `import` statements instead of `require()` when possible

Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `@/` alias for src imports and `tests/` alias for test imports in TypeScript/JavaScript files.

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.test.{ts,tsx} : Prefer top-level imports over nested requires in test files

Learnt from: aaronmgdr
Repo: selfxyz/self PR: 951
File: app/src/providers/authProvider.web.tsx:17-18
Timestamp: 2025-08-25T14:25:57.586Z
Learning: The selfxyz/mobile-sdk-alpha/constants/analytics import path is properly configured with SDK exports, Metro aliases, and TypeScript resolution. Import changes from `@/consts/analytics` to this path are part of valid analytics migration, not TypeScript resolution issues.

Learnt from: CR
Repo: selfxyz/self PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T19:18:43.224Z
Learning: Applies to {app,packages/mobile-sdk-alpha}/**/*.{test,spec}.{js,ts,jsx,tsx} : Never create nested require('react-native') calls in tests as this causes out-of-memory errors in CI/CD pipelines; use ES6 import statements instead and avoid dynamic require() calls in beforeEach/afterEach hooks

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.{ts,tsx} : Ensure no breaking changes to public API or document them properly

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/index.{ts,tsx} : Run `yarn validate:exports` to verify that exports are properly configured

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.{ts,tsx} : Verify cross-platform compatibility for both React Native and Web environments

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Verify integration with main app by testing in `selfxyz/mobile-app` when SDK API changes

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.{ts,tsx} : Use platform detection `Platform.OS === 'web'` when adding platform-specific code

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/package.json : Run `yarn validate:pkg` to verify that package conditions are valid

Learnt from: CR
Repo: selfxyz/self PR: 0
File: packages/mobile-sdk-alpha/AGENTS.md:0-0
Timestamp: 2025-12-25T19:19:35.354Z
Learning: Applies to packages/mobile-sdk-alpha/**/*.{ts,tsx} : Run `yarn build` to build the package for distribution

Learnt from: CR
Repo: selfxyz/self PR: 0
File: .cursorrules:0-0
Timestamp: 2025-11-25T14:06:55.970Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use platform-specific initial routes: web uses 'Home', mobile uses 'Splash' in React Navigation.

@transphorm transphorm merged commit a96777d into dev Jan 30, 2026
31 checks passed
@transphorm transphorm deleted the justin/add-countries-json-file-to-app branch January 30, 2026 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants