revert pr 1786 lottie json conversion#1848
Merged
transphorm merged 7 commits intodevfrom Mar 12, 2026
Merged
Conversation
This reverts commit 9406bac.
![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: c2e834245f
ℹ️ 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".
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:
📝 WalkthroughWalkthroughMigrates animation handling from dotLottie/.lottie to JSON + lottie-react-native, adds a DelayedLottieView wrapper, removes the legacy LottieAnimation/DotLottie exports and converter, replaces many .lottie requires with JSON imports, and updates build/config/test tooling and multiple app/sdk screens. Changes
Sequence Diagram(s)sequenceDiagram
participant App as rgba(33,150,243,0.5) App Screen
participant SDK as rgba(156,39,176,0.5) DelayedLottieView
participant Native as rgba(76,175,80,0.5) LottieNativeModule
App->>SDK: render <DelayedLottieView source=animation autoPlay=true />
SDK->>SDK: props.autoPlay -> false (disable native autoplay)
SDK->>Native: mount LottieView with source (no autoplay)
SDK->>SDK: setTimeout(100ms) to defer start
SDK->>Native: call play() after delay
Native-->>SDK: emit onComplete / onFinish
SDK-->>App: propagate onAnimationFinish
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx (1)
304-320:⚠️ Potential issue | 🔴 CriticalCritical: Object literal in useCallback dependency array causes re-renders.
The dependency array contains an inline object
{ canNumber: props.canNumber, useCan: props.useCan, ... }which creates a new reference on every render, defeating memoization and potentially causing performance issues or infinite re-render loops.🐛 Fix: List props individually in the dependency array
}, [ baseContext, isNfcEnabled, isNfcSupported, - { - canNumber: props.canNumber, - useCan: props.useCan, - skipPACE: props.skipPACE, - skipCA: props.skipCA, - extendedMode: props.extendedMode, - }, + props.canNumber, + props.useCan, + props.skipPACE, + props.skipCA, + props.extendedMode, passportNumber, dateOfBirth, dateOfExpiry, isPacePolling, trackEvent, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx` around lines 304 - 320, The inline object in the useCallback dependency array inside document-nfc-screen.tsx (the callback that uses baseContext, isNfcEnabled, isNfcSupported, passportNumber, dateOfBirth, dateOfExpiry, isPacePolling, trackEvent) is causing a new reference each render; replace the object literal with the individual props instead—use props.canNumber, props.useCan, props.skipPACE, props.skipCA, and props.extendedMode directly in the dependency array of that useCallback so the callback can be properly memoized.app/src/screens/app/SplashScreen.tsx (1)
127-136:⚠️ Potential issue | 🟠 MajorAdd safety timeout to prevent splash screen hang if animation callback fails.
If
onAnimationFinishnever fires (animation loading failure, native bridge timing issues, etc.), the app will remain stuck on the splash screen indefinitely, blocking navigation even after data has loaded successfully. Currently there is no fallback mechanism.Add a safety timeout to ensure the app always progresses:
🛡️ Suggested safety timeout
+ // Safety timeout in case animation fails to complete + useEffect(() => { + const timeout = setTimeout(() => { + if (!isAnimationFinished) { + console.warn('Splash animation timeout - forcing completion'); + handleAnimationFinish(); + } + }, 10000); // 10 second safety net + return () => clearTimeout(timeout); + }, [isAnimationFinished, handleAnimationFinish]); + return ( <DelayedLottieView🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/screens/app/SplashScreen.tsx` around lines 127 - 136, The splash can hang if DelayedLottieView.onAnimationFinish never fires; add a safety timeout in the SplashScreen that calls the same completion logic as handleAnimationFinish after a short delay (e.g., 5s) as a fallback. Implement this by starting a timer (store id in a ref) when the component mounts or when the Lottie starts, clear the timer inside handleAnimationFinish and in useEffect cleanup/unmount, and ensure the timeout handler invokes the same navigation/data-ready flow as handleAnimationFinish; update references to DelayedLottieView and handleAnimationFinish accordingly.
🧹 Nitpick comments (3)
app/package.json (1)
133-136: Hash library duplication strategy: Consider consolidating around@noble/hashesfor future maintenance.The four SHA libraries (
hash.js,js-sha1,js-sha256,js-sha512) are actively used across the codebase, primarily incommon/src/utils/hash.tsfor React Native compatibility (crypto is unavailable in RN environments). However,@noble/hashesis already established as the modern polyfill standard for web builds (common/src/polyfills/crypto.ts).This dual-library approach is intentional but creates maintenance complexity and bundle overhead. A long-term refactoring could migrate the RN-specific utilities to use
@noble/hashesas well—it's widely compatible and would consolidate around a single, audited cryptographic library. This is not blocking the current implementation but worth considering for future optimization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/package.json` around lines 133 - 136, The repo currently depends on four separate SHA libs (hash.js, js-sha1, js-sha256, js-sha512) which duplicate functionality already provided by `@noble/hashes` used in common/src/polyfills/crypto.ts; to reduce maintenance and bundle overhead, remove those four packages from package.json and replace their usage in common/src/utils/hash.ts by importing the appropriate `@noble/hashes` exports (sha1/sha256/sha512) and adapting any RN-specific shims to call `@noble/hashes` APIs; update the dependency list to include `@noble/hashes` if missing, run and adjust unit/metro bundle tests to verify React Native compatibility, and ensure any import names changed in common/src/utils/hash.ts match the rest of the codebase.app/src/assets/animations/loader.ts (1)
5-8: Remove unused animation loader functions or fix module default export handling.
loadMiscAnimationandloadPassportAnimationhave no callers in the codebase—they're either dead code or pre-emptive API. If removed, no changes needed. If kept for future/external use, both functions must unwrap the module default export returned byimport(), otherwise consumers will receive{ default: <json> }instead of the JSON object itself.💡 Fix if keeping for public API
-export const loadMiscAnimation = () => - import('@selfxyz/mobile-sdk-alpha/animations/loading/misc.json'); -export const loadPassportAnimation = () => - import('@/assets/animations/passport_verify.json'); +export const loadMiscAnimation = async () => { + const mod = await import('@selfxyz/mobile-sdk-alpha/animations/loading/misc.json'); + return mod.default ?? mod; +}; + +export const loadPassportAnimation = async () => { + const mod = await import('@/assets/animations/passport_verify.json'); + return mod.default ?? mod; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/assets/animations/loader.ts` around lines 5 - 8, The two exported loaders loadMiscAnimation and loadPassportAnimation are unused; either delete them to remove dead code, or if you intend to keep them as a public API then change each to unwrap the dynamic import's default export so consumers get the raw JSON (e.g., have loadMiscAnimation and loadPassportAnimation return the promise that resolves to module.default or module itself if default is missing). Update the functions by referencing loadMiscAnimation and loadPassportAnimation so they return the unwrapped JSON object from the import promise.packages/mobile-sdk-alpha/package.json (1)
214-214: Consider relaxing the peer dependency version constraint.Pinning
lottie-react-nativeto an exact version (7.2.2) in peerDependencies is quite restrictive. Consumers on7.2.3or7.3.xwould see npm/yarn warnings or errors. A semver range like^7.2.0would be more flexible while still ensuring compatibility.♻️ Suggested change
- "lottie-react-native": "7.2.2", + "lottie-react-native": "^7.2.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mobile-sdk-alpha/package.json` at line 214, Update the peerDependencies entry for lottie-react-native which is currently pinned to the exact version "7.2.2": change it to a semver range (e.g. ^7.2.0) so consumers on 7.2.x/7.3.x don’t get install warnings; locate the "lottie-react-native" key in package.json's peerDependencies and replace the exact version string with the chosen caret range.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/screens/onboarding/AccountVerifiedSuccessScreen.tsx`:
- Line 25: The component AccountVerifiedSuccessScreen currently declares unused
destructured props with the empty object pattern "({})"; remove the unnecessary
destructuring and change the function signature to a plain no-props React.FC
(e.g., AccountVerifiedSuccessScreen: React.FC = () => ...) so the component
doesn't accept or destructure props it doesn't use—update the declaration in
AccountVerifiedSuccessScreen accordingly.
In `@app/src/screens/verification/ProofRequestStatusScreen.tsx`:
- Line 26: Rename the misspelled import identifier succesAnimation to
successAnimation in the import statement and update every usage of
succesAnimation in ProofRequestStatusScreen (e.g., the component prop or
variable where the animation is passed) to the new successAnimation identifier
so imports and references match; ensure the import path and filename remain
unchanged and run a quick typecheck to catch any remaining references.
In `@packages/mobile-sdk-alpha/src/animations/passport_scan.json`:
- Line 1: The animation JSON embeds a huge base64 blob under asset id "0"
(payload starting with "data:image/png;base64,...") which bloats parsing and
memory; remove that inline data and replace it with a lightweight reference
(e.g. an asset URL or a local filename) pointed from the same asset object's "p"
value so layers that reference refId "0" keep working; ensure the image is added
to your app bundle or hosted and loaded by your asset loader before playing the
animation (update any loader that expects embedded data to fetch the external
asset).
In `@packages/mobile-sdk-alpha/src/components/DelayedLottieView.tsx`:
- Around line 24-48: The early return when LottieView is undefined causes hooks
in DelayedLottieView (useRef, useEffect) to be called conditionally; move the
hook calls to always run before any conditional return: always create
internalRef and compute ref from forwardedRef, and always run the useEffect (it
can be a no-op when LottieView is undefined by checking LottieView inside the
effect), then keep the conditional return for rendering null when LottieView is
undefined; also compute modifiedProps (based on props.autoPlay) after hooks so
the component's hook order remains stable.
---
Outside diff comments:
In `@app/src/screens/app/SplashScreen.tsx`:
- Around line 127-136: The splash can hang if
DelayedLottieView.onAnimationFinish never fires; add a safety timeout in the
SplashScreen that calls the same completion logic as handleAnimationFinish after
a short delay (e.g., 5s) as a fallback. Implement this by starting a timer
(store id in a ref) when the component mounts or when the Lottie starts, clear
the timer inside handleAnimationFinish and in useEffect cleanup/unmount, and
ensure the timeout handler invokes the same navigation/data-ready flow as
handleAnimationFinish; update references to DelayedLottieView and
handleAnimationFinish accordingly.
In `@packages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx`:
- Around line 304-320: The inline object in the useCallback dependency array
inside document-nfc-screen.tsx (the callback that uses baseContext,
isNfcEnabled, isNfcSupported, passportNumber, dateOfBirth, dateOfExpiry,
isPacePolling, trackEvent) is causing a new reference each render; replace the
object literal with the individual props instead—use props.canNumber,
props.useCan, props.skipPACE, props.skipCA, and props.extendedMode directly in
the dependency array of that useCallback so the callback can be properly
memoized.
---
Nitpick comments:
In `@app/package.json`:
- Around line 133-136: The repo currently depends on four separate SHA libs
(hash.js, js-sha1, js-sha256, js-sha512) which duplicate functionality already
provided by `@noble/hashes` used in common/src/polyfills/crypto.ts; to reduce
maintenance and bundle overhead, remove those four packages from package.json
and replace their usage in common/src/utils/hash.ts by importing the appropriate
`@noble/hashes` exports (sha1/sha256/sha512) and adapting any RN-specific shims to
call `@noble/hashes` APIs; update the dependency list to include `@noble/hashes` if
missing, run and adjust unit/metro bundle tests to verify React Native
compatibility, and ensure any import names changed in common/src/utils/hash.ts
match the rest of the codebase.
In `@app/src/assets/animations/loader.ts`:
- Around line 5-8: The two exported loaders loadMiscAnimation and
loadPassportAnimation are unused; either delete them to remove dead code, or if
you intend to keep them as a public API then change each to unwrap the dynamic
import's default export so consumers get the raw JSON (e.g., have
loadMiscAnimation and loadPassportAnimation return the promise that resolves to
module.default or module itself if default is missing). Update the functions by
referencing loadMiscAnimation and loadPassportAnimation so they return the
unwrapped JSON object from the import promise.
In `@packages/mobile-sdk-alpha/package.json`:
- Line 214: Update the peerDependencies entry for lottie-react-native which is
currently pinned to the exact version "7.2.2": change it to a semver range (e.g.
^7.2.0) so consumers on 7.2.x/7.3.x don’t get install warnings; locate the
"lottie-react-native" key in package.json's peerDependencies and replace the
exact version string with the chosen caret range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0caa48f7-dbad-4e33-85e6-62f8a3429206
⛔ Files ignored due to path filters (3)
app/Gemfile.lockis excluded by!**/*.lockapp/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (69)
app/android/app/build.gradleapp/android/build.gradleapp/ios/Podfileapp/ios/Self.xcodeproj/project.pbxprojapp/jest.config.cjsapp/metro.config.cjsapp/package.jsonapp/scripts/convert-to-dotlottie.mjsapp/src/assets/animations/launch_onboarding.jsonapp/src/assets/animations/loader.tsapp/src/assets/animations/passport_onboarding.jsonapp/src/assets/animations/passport_onboarding.lottieapp/src/assets/animations/passport_scan.jsonapp/src/assets/animations/passport_scan.lottieapp/src/assets/animations/passport_verify.jsonapp/src/assets/animations/passport_verify.lottieapp/src/assets/animations/proof_failed.jsonapp/src/assets/animations/proof_failed.lottieapp/src/assets/animations/proof_success.jsonapp/src/assets/animations/proof_success.lottieapp/src/assets/animations/qr_scan.jsonapp/src/assets/animations/qr_scan.lottieapp/src/assets/animations/splash.jsonapp/src/assets/animations/splash.lottieapp/src/assets/animations/warning.jsonapp/src/assets/animations/warning.lottieapp/src/components/LoadingUI.tsxapp/src/screens/app/GratificationScreen.tsxapp/src/screens/app/LoadingScreen.tsxapp/src/screens/app/SplashScreen.tsxapp/src/screens/dev/DevLoadingScreen.tsxapp/src/screens/documents/scanning/DocumentCameraScreen.tsxapp/src/screens/documents/scanning/DocumentNFCScanScreen.tsxapp/src/screens/documents/selection/DocumentOnboardingScreen.tsxapp/src/screens/kyc/KycSuccessScreen.tsxapp/src/screens/onboarding/AccountVerifiedSuccessScreen.tsxapp/src/screens/onboarding/DisclaimerScreen.tsxapp/src/screens/verification/ProofRequestStatusScreen.tsxapp/src/screens/verification/QRCodeViewFinderScreen.tsxapp/tests/src/screens/GratificationScreen.test.tsxapp/tests/src/screens/kyc/KycSuccessScreen.test.tsxapp/vite.config.tspackages/mobile-sdk-alpha/package.jsonpackages/mobile-sdk-alpha/src/animations/loading/fail.jsonpackages/mobile-sdk-alpha/src/animations/loading/fail.lottiepackages/mobile-sdk-alpha/src/animations/loading/misc.jsonpackages/mobile-sdk-alpha/src/animations/loading/misc.lottiepackages/mobile-sdk-alpha/src/animations/loading/prove.jsonpackages/mobile-sdk-alpha/src/animations/loading/prove.lottiepackages/mobile-sdk-alpha/src/animations/loading/success.jsonpackages/mobile-sdk-alpha/src/animations/loading/success.lottiepackages/mobile-sdk-alpha/src/animations/loading/youWin.jsonpackages/mobile-sdk-alpha/src/animations/loading/youWin.lottiepackages/mobile-sdk-alpha/src/animations/passport_scan.jsonpackages/mobile-sdk-alpha/src/animations/passport_scan.lottiepackages/mobile-sdk-alpha/src/animations/passport_verify.jsonpackages/mobile-sdk-alpha/src/animations/passport_verify.lottiepackages/mobile-sdk-alpha/src/components/DelayedLottieView.tsxpackages/mobile-sdk-alpha/src/components/DelayedLottieView.web.tsxpackages/mobile-sdk-alpha/src/components/LottieAnimation.tsxpackages/mobile-sdk-alpha/src/flows/onboarding/confirm-identification.tsxpackages/mobile-sdk-alpha/src/flows/onboarding/document-camera-screen.tsxpackages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsxpackages/mobile-sdk-alpha/src/index.tspackages/mobile-sdk-alpha/tests/LottieAnimation.test.tsxpackages/mobile-sdk-alpha/tests/setup.tspackages/mobile-sdk-alpha/tsup.config.tspatches/@lottiefiles+dotlottie-react-native+0.5.0.patchpurple/frontend/styling.md
💤 Files with no reviewable changes (5)
- app/android/build.gradle
- packages/mobile-sdk-alpha/src/components/LottieAnimation.tsx
- packages/mobile-sdk-alpha/tests/LottieAnimation.test.tsx
- app/scripts/convert-to-dotlottie.mjs
- patches/@LottieFiles+dotlottie-react-native+0.5.0.patch
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx (1)
308-324:⚠️ Potential issue | 🟠 MajorObject literal in dependency array causes memoization failure.
The object literal at lines 312-318 creates a new reference on every render, defeating
useCallback's memoization. This will causeonVerifyPressto be recreated on every render cycle, potentially triggering unnecessary re-renders of child components that depend on this callback.🐛 Proposed fix: destructure props directly in dependencies
}, [ baseContext, isNfcEnabled, isNfcSupported, - { - canNumber: props.canNumber, - useCan: props.useCan, - skipPACE: props.skipPACE, - skipCA: props.skipCA, - extendedMode: props.extendedMode, - }, + props.canNumber, + props.useCan, + props.skipPACE, + props.skipCA, + props.extendedMode, passportNumber, dateOfBirth, dateOfExpiry, isPacePolling, trackEvent, + handleNFCError, + selfClient, + logNFCEvent, + trackNfcEvent, ]);Note: The callback also references
handleNFCError,selfClient,logNFCEvent, andtrackNfcEventwhich should be included in the dependency array for exhaustive deps compliance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx` around lines 308 - 324, The dependency array for the useCallback that defines onVerifyPress must stop using the inline object literal (which creates a new reference each render) — replace the object literal with the individual prop fields (props.canNumber, props.useCan, props.skipPACE, props.skipCA, props.extendedMode) and include the missing references handleNFCError, selfClient, logNFCEvent, and trackNfcEvent so onVerifyPress's dependencies are stable and exhaustive; update the dependency array for onVerifyPress accordingly to list baseContext, isNfcEnabled, isNfcSupported, props.canNumber, props.useCan, props.skipPACE, props.skipCA, props.extendedMode, passportNumber, dateOfBirth, dateOfExpiry, isPacePolling, trackEvent, handleNFCError, selfClient, logNFCEvent, and trackNfcEvent.app/src/screens/verification/ProofRequestStatusScreen.tsx (1)
217-225:⚠️ Potential issue | 🟠 MajorRemove the fixed
progress={0}from DelayedLottieView.The
DelayedLottieViewcomponent forwards all props directly toLottieView, includingprogress={0}. In lottie-react-native,progressis a controlled prop that specifies the exact playback frame position (0..1 range). When passed alongsideautoPlayor imperativeplay()calls, it conflicts with the animation lifecycle. The component's internalplay()call on line 37 of DelayedLottieView.tsx will interact unpredictably with the fixed zero progress value. Remove this prop to let the animation play naturally from the beginning.Suggested fix
<DelayedLottieView autoPlay loop={animationSource === loadingAnimation} source={animationSource} style={styles.animation} cacheComposition={false} renderMode="HARDWARE" speed={1} - progress={0} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/screens/verification/ProofRequestStatusScreen.tsx` around lines 217 - 225, Remove the hard-coded progress={0} so Lottie can control playback: delete the progress prop from the DelayedLottieView usage in ProofRequestStatusScreen (the JSX shown) and also remove any default/forwarded progress prop inside the DelayedLottieView implementation so it doesn't forward a fixed progress to LottieView; keep the component's internal play() call (line ~37 in DelayedLottieView.tsx) intact so autoPlay/play() can drive the animation from the start.
🧹 Nitpick comments (1)
packages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx (1)
70-76: Consider usingautoPlay={true}to simplify animation initialization.This manual
useEffectreplicates the 100ms delayed play thatDelayedLottieViewalready provides whenautoPlay={true}. Since the component setsautoPlay={false}at line 395, you're duplicating logic.If you switch to
autoPlay={true},DelayedLottieViewwill handle the initial 100ms delay internally, and your ref will still be available for theonAnimationFinishreplay logic.♻️ Proposed simplification
Remove the manual useEffect:
- useEffect(() => { - const timer = setTimeout(() => { - animationRef.current?.play(); - }, 100); - - return () => clearTimeout(timer); - }, []);And update the component to use autoPlay:
<DelayedLottieView ref={animationRef} - autoPlay={false} + autoPlay={true} loop={false}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx` around lines 70 - 76, Remove the manual useEffect that sets a 100ms timeout to call animationRef.current?.play() and instead enable the DelayedLottieView's built-in startup behavior by changing its autoPlay prop from false to true; keep animationRef and the existing onAnimationFinish replay logic intact so the ref is still available for replay, and delete the timeout/clearTimeout useEffect to avoid duplicating the delay.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/scripts/bundle-analyze-ci.cjs`:
- Around line 17-22: Revert the temporary bump to the iOS threshold in
BUNDLE_THRESHOLDS_MB (the ios key) back to the original 46 MB unless you can
attach measured evidence; if you intend to keep 46.25 MB, run the post-fix
baseline measurement with `yarn workspace `@selfxyz/mobile-app` test`, record the
resulting bundle size, and add that measurement and the test command output as a
comment or PR description to justify the change; otherwise restore ios: 46 and
remove the "TODO: fix temporary bundle bump" or replace it with a note linking
to the benchmark evidence.
---
Outside diff comments:
In `@app/src/screens/verification/ProofRequestStatusScreen.tsx`:
- Around line 217-225: Remove the hard-coded progress={0} so Lottie can control
playback: delete the progress prop from the DelayedLottieView usage in
ProofRequestStatusScreen (the JSX shown) and also remove any default/forwarded
progress prop inside the DelayedLottieView implementation so it doesn't forward
a fixed progress to LottieView; keep the component's internal play() call (line
~37 in DelayedLottieView.tsx) intact so autoPlay/play() can drive the animation
from the start.
In `@packages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx`:
- Around line 308-324: The dependency array for the useCallback that defines
onVerifyPress must stop using the inline object literal (which creates a new
reference each render) — replace the object literal with the individual prop
fields (props.canNumber, props.useCan, props.skipPACE, props.skipCA,
props.extendedMode) and include the missing references handleNFCError,
selfClient, logNFCEvent, and trackNfcEvent so onVerifyPress's dependencies are
stable and exhaustive; update the dependency array for onVerifyPress accordingly
to list baseContext, isNfcEnabled, isNfcSupported, props.canNumber,
props.useCan, props.skipPACE, props.skipCA, props.extendedMode, passportNumber,
dateOfBirth, dateOfExpiry, isPacePolling, trackEvent, handleNFCError,
selfClient, logNFCEvent, and trackNfcEvent.
---
Nitpick comments:
In `@packages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx`:
- Around line 70-76: Remove the manual useEffect that sets a 100ms timeout to
call animationRef.current?.play() and instead enable the DelayedLottieView's
built-in startup behavior by changing its autoPlay prop from false to true; keep
animationRef and the existing onAnimationFinish replay logic intact so the ref
is still available for replay, and delete the timeout/clearTimeout useEffect to
avoid duplicating the delay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc4acf65-0769-41c3-aa63-fdd22858884c
⛔ Files ignored due to path filters (1)
app/ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
app/ios/Podfileapp/scripts/bundle-analyze-ci.cjsapp/src/screens/app/SplashScreen.tsxapp/src/screens/documents/scanning/DocumentNFCScanScreen.tsxapp/src/screens/verification/ProofRequestStatusScreen.tsxapp/src/screens/verification/QRCodeViewFinderScreen.tsxpackages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx
💤 Files with no reviewable changes (1)
- app/ios/Podfile
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/screens/documents/scanning/DocumentNFCScanScreen.tsx
- app/src/screens/app/SplashScreen.tsx
- app/src/screens/verification/QRCodeViewFinderScreen.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx (1)
391-404:⚠️ Potential issue | 🟠 MajorMissing cleanup for the replay timeout creates potential memory leak.
The
setTimeoutinsideonAnimationFinish(5-second delay before replay) has no cleanup mechanism. If the component unmounts during that window, the timer will still fire and attempt to callplay()on a potentially stale ref.While
animationRef.current?.play()won't crash due to optional chaining, this pattern can still leak memory and trigger React warnings about updates on unmounted components.🛡️ Proposed fix: Track and cleanup the replay timeout
+ const replayTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); + + // Cleanup replay timeout on unmount + useEffect(() => { + return () => { + if (replayTimeoutRef.current) { + clearTimeout(replayTimeoutRef.current); + } + }; + }, []); + // ... existing code ... <DelayedLottieView ref={animationRef} autoPlay={false} loop={false} onAnimationFinish={() => { - setTimeout(() => { + replayTimeoutRef.current = setTimeout(() => { animationRef.current?.play(); }, 5000); // Pause 5 seconds before playing again }} source={passportVerifyAnimation} style={styles.animation} cacheComposition={true} renderMode="HARDWARE" />As per coding guidelines: "Implement memory leak prevention in native modules with proper cleanup in useEffect and component unmount."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx` around lines 391 - 404, The onAnimationFinish handler in DelayedLottieView schedules a setTimeout to call animationRef.current?.play() but never clears it, risking a leak if the component unmounts; update the component to store the timeout id (from setTimeout) in a ref (e.g., replayTimeoutRef), clear that timeout on unmount (or before scheduling a new one) inside a useEffect cleanup, and ensure any scheduled timeout is cancelled before calling play or when animationRef changes so the replay timeout is properly cleaned up.app/src/screens/verification/ProofRequestStatusScreen.tsx (1)
216-225:⚠️ Potential issue | 🟠 MajorRemove the fixed
progress={0}prop.
DelayedLottieViewpasses all props through tolottie-react-native, includingprogress. Settingprogress={0}pins the animation to the first frame, which prevents the loading/success/failure animations from playing even thoughautoPlayis enabled. This will display a static image instead of animated feedback, degrading the user experience on the proof-status screen.Suggested fix
<DelayedLottieView autoPlay loop={animationSource === loadingAnimation} source={animationSource} style={styles.animation} cacheComposition={false} renderMode="HARDWARE" speed={1} - progress={0} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/screens/verification/ProofRequestStatusScreen.tsx` around lines 216 - 225, Remove the fixed progress prop passed into DelayedLottieView so the underlying lottie-react-native instance can control playback; specifically, delete the explicit progress={0} prop in the ProofRequestStatusScreen component (the DelayedLottieView usage) so autoPlay/loop/speed behave normally and animations play.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/mobile-sdk-alpha/src/components/DelayedLottieView.web.tsx`:
- Around line 11-16: The DelayedLottieView web component currently discards all
props except onAnimationFinish which breaks the drop-in contract; update the
DelayedLottieView function to forward the incoming props onto the rendered
element (at minimum spread style, testID/testId/data-testid, and any other core
props like autoPlay, loop, source, cacheComposition, renderMode) and return a
DOM element (e.g., <div>) that applies the forwarded style and test id; keep the
existing useEffect call to props.onAnimationFinish but accept a props object
type that allows these keys and then spread {...rest} onto the returned element
so callers’ layout and behavior props are honored.
---
Outside diff comments:
In `@app/src/screens/verification/ProofRequestStatusScreen.tsx`:
- Around line 216-225: Remove the fixed progress prop passed into
DelayedLottieView so the underlying lottie-react-native instance can control
playback; specifically, delete the explicit progress={0} prop in the
ProofRequestStatusScreen component (the DelayedLottieView usage) so
autoPlay/loop/speed behave normally and animations play.
In `@packages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx`:
- Around line 391-404: The onAnimationFinish handler in DelayedLottieView
schedules a setTimeout to call animationRef.current?.play() but never clears it,
risking a leak if the component unmounts; update the component to store the
timeout id (from setTimeout) in a ref (e.g., replayTimeoutRef), clear that
timeout on unmount (or before scheduling a new one) inside a useEffect cleanup,
and ensure any scheduled timeout is cancelled before calling play or when
animationRef changes so the replay timeout is properly cleaned up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc11577b-f93a-4183-9a92-1412eb2ea524
📒 Files selected for processing (3)
app/src/screens/verification/ProofRequestStatusScreen.tsxpackages/mobile-sdk-alpha/src/components/DelayedLottieView.web.tsxpackages/mobile-sdk-alpha/src/flows/onboarding/document-nfc-screen.tsx
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 `@packages/mobile-sdk-alpha/src/components/DelayedLottieView.web.tsx`:
- Around line 23-25: The effect in DelayedLottieView.web.tsx calls
onAnimationFinish unguarded and can fire twice under React StrictMode; add a
one-shot guard using a ref (e.g., called hasCalledRef) inside the useEffect that
checks if hasCalledRef.current is true before invoking onAnimationFinish and
sets it to true after the first call so onAnimationFinish (the prop) is only
invoked once even if the effect re-runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df494d47-e668-4ecc-9528-58ca361edeed
📒 Files selected for processing (2)
app/scripts/bundle-analyze-ci.cjspackages/mobile-sdk-alpha/src/components/DelayedLottieView.web.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/scripts/bundle-analyze-ci.cjs
Comment on lines
+23
to
+25
| useEffect(() => { | ||
| onAnimationFinish?.(); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== StrictMode usage ==\n'
rg -n -C2 'StrictMode' --glob '*.{ts,tsx,js,jsx}'
printf '\n== DelayedLottieView / onAnimationFinish call sites ==\n'
rg -n -C3 'DelayedLottieView|onAnimationFinish' --glob '*.{ts,tsx,js,jsx}'Repository: selfxyz/self
Length of output: 25647
Guard onAnimationFinish against React StrictMode double-invocation.
In React 18 dev/test runs, mount effects intentionally re-fire under StrictMode. This causes onAnimationFinish to fire twice, triggering duplicate navigation/state transitions and making tests flaky. Add a one-shot guard using useRef to prevent multiple invocations.
Suggested fix
-import { useEffect } from 'react';
+import { useEffect, useRef } from 'react';
@@
export const DelayedLottieView = (props: DelayedLottieViewWebProps) => {
- const { onAnimationFinish, style, testID, ...rest } = props;
+ const { onAnimationFinish, style, testID } = props;
+ const hasFiredRef = useRef(false);
useEffect(() => {
+ if (hasFiredRef.current) return;
+ hasFiredRef.current = true;
onAnimationFinish?.();
- }, []);
+ }, [onAnimationFinish]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/mobile-sdk-alpha/src/components/DelayedLottieView.web.tsx` around
lines 23 - 25, The effect in DelayedLottieView.web.tsx calls onAnimationFinish
unguarded and can fire twice under React StrictMode; add a one-shot guard using
a ref (e.g., called hasCalledRef) inside the useEffect that checks if
hasCalledRef.current is true before invoking onAnimationFinish and sets it to
true after the first call so onAnimationFinish (the prop) is only invoked once
even if the effect re-runs.
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
Test plan
Native Consolidation Checklist
cd app && yarn jest:run/yarn workspace @selfxyz/rn-sdk-test-app test)Summary by CodeRabbit