[Bug] Circular dependency #1
Conversation
|
|
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant PlatformImpl
participant NativeModule
App->>PlatformImpl: startVerification(request)
PlatformImpl->>NativeModule: startVerification(request)
NativeModule-->>PlatformImpl: response / error
PlatformImpl-->>App: response or throws ReclaimVerificationException
App->>PlatformImpl: setOverrides(overrides)
PlatformImpl->>NativeModule: setOverrides(overrides)
NativeModule-->>PlatformImpl: emits events (e.g., providerRequest)
PlatformImpl-->>App: invokes callbacks, manages listeners
App->>PlatformImpl: setVerificationOptions(options)
PlatformImpl->>NativeModule: setVerificationOptions(options)
NativeModule-->>PlatformImpl: emits attestorAuthRequest event
PlatformImpl-->>App: invokes attestorAuthRequest callback
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
lib/commonjs/index.js (1)
46-58: Same IIFE concern as ESM buildSee remark in
lib/module/index.js; adopting named re-exports keeps CJS and ESM outputs symmetrical and simplifies maintenance.🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: 'ReclaimVerification' is a class.
'ReclaimVerification' is defined here.
(lint/suspicious/noClassAssign)
🧹 Nitpick comments (6)
src/types.ts (2)
86-88: Use primitivestringinstead of boxedStringin theLogConsumer.onLogscallback
Stringcreates a boxed wrapper object and is discouraged. Using the primitivestringkeeps typings simpler and avoids subtle bugs withinstanceof,typeof, and JSON serialisation.- onLogs?: (logJsonString: String, cancel: () => void) => void; + onLogs?: (logJsonString: string, cancel: () => void) => void;🧰 Tools
🪛 Biome (1.9.4)
[error] 87-87: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
40-52: Type-safety ofasProofscould silently drop valid proofs
filter(isProof)narrows the resulting array but does not guarantee the caller supplied an array of heterogeneous objects. If the input already has the correct type you incur an unnecessary run-time pass; if it’s mis-typed you silently discard data. Consider:export const asProofs = (proofs: unknown[]): Proof[] => { return proofs.filter(isProof); };and document that non-proof objects are ignored, or throw on the first invalid entry to surface data issues early.
lib/module/index.js (1)
43-54: Re-export block executes at module load – avoid accidental early side-effectsWrapping property re-exports in an IIFE immediately executes at import time. Although harmless here, consider using explicit
export conststatements to avoid hidden evaluation order issues and make tree-shaking easier:export const { ReclaimResult, ExceptionType, ReclaimPlatformException, ReclaimVerificationException, Platform } = ReclaimVerificationTypes;🧰 Tools
🪛 Biome (1.9.4)
[error] 54-54: 'ReclaimVerification' is a class.
'ReclaimVerification' is defined here.
(lint/suspicious/noClassAssign)
src/platform.ts (2)
93-96: Avoid sendingundefinedover the RN bridgeReact-Native automatically serialises
undefinedasnull, but some native
codepaths treatnulland missing-field differently.
Consider normalising optional fields before crossing the bridge to reduce
platform-specific surprises:- url: provider?.url, - jsonString: provider?.jsonString, + url: provider?.url ?? null, + jsonString: provider?.jsonString ?? null,Same applies to the
logConsumerRequestobject.Also applies to: 118-122
53-55: Minor: unnecessaryawaitinping()
return await NativeReclaimInappModule.ping();adds an extra micro-task.
You can justreturn NativeReclaimInappModule.ping();for the same behaviour.src/index.ts (1)
54-94: Namespace re-exports collide on the value namespaceInside
export namespace ReclaimVerification { … }you expose both:export const ExceptionType = … export type ExceptionType = …When consumers use
import { ExceptionType } …they will bring in the value by default; the type alias is erased at runtime.
If that isn’t intentional, consider renaming the value (e.g.ExceptionTypeEnum) or usingimport typein downstream code to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
lib/commonjs/index.js.mapis excluded by!**/*.maplib/module/index.js.mapis excluded by!**/*.maplib/typescript/commonjs/src/index.d.ts.mapis excluded by!**/*.maplib/typescript/module/src/index.d.ts.mapis excluded by!**/*.mappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
lib/commonjs/index.js(2 hunks)lib/module/index.js(2 hunks)lib/typescript/commonjs/src/index.d.ts(2 hunks)lib/typescript/module/src/index.d.ts(2 hunks)package.json(0 hunks)src/index.ts(2 hunks)src/platform.ts(1 hunks)src/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- package.json
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/platform.ts (3)
src/index.ts (5)
ReclaimVerification(12-52)Request(57-57)Response(58-58)OverrideConfig(81-81)VerificationOptions(68-69)lib/module/index.js (1)
ReclaimVerification(14-41)src/types.ts (6)
Request(18-18)Response(23-25)error(185-217)error(219-223)OverrideConfig(103-110)VerificationOptions(70-75)
lib/module/index.js (3)
lib/commonjs/index.js (7)
ReclaimResult(49-49)isProof(51-51)asProofs(52-52)ExceptionType(54-54)ReclaimPlatformException(55-55)ReclaimVerificationException(56-56)Platform(57-57)src/index.ts (10)
isProof(65-65)asProofs(66-66)ExceptionType(82-82)ExceptionType(83-83)ReclaimPlatformException(84-85)ReclaimPlatformException(86-87)ReclaimVerificationException(88-89)ReclaimVerificationException(90-91)Platform(92-92)Platform(93-93)src/types.ts (4)
isProof(40-48)asProofs(50-52)ReclaimPlatformException(119-142)ReclaimVerificationException(144-224)
lib/typescript/module/src/index.d.ts (3)
src/index.ts (24)
Platform(92-92)Platform(93-93)Request(57-57)Response(58-58)OverrideConfig(81-81)VerificationOptions(68-69)ReclaimVerification(12-52)SessionInformation(56-56)Proof(60-60)ProviderClaimData(61-62)WitnessData(63-64)isProof(65-65)asProofs(66-66)ProviderInformation(71-72)FeatureOptions(73-74)LogConsumer(75-75)SessionManagement(76-77)ReclaimAppInfo(78-79)ExceptionType(82-82)ExceptionType(83-83)ReclaimPlatformException(84-85)ReclaimPlatformException(86-87)ReclaimVerificationException(88-89)ReclaimVerificationException(90-91)src/types.ts (17)
Request(18-18)Response(23-25)OverrideConfig(103-110)VerificationOptions(70-75)SessionInformation(12-13)Proof(28-38)ProviderClaimData(54-62)WitnessData(64-67)isProof(40-48)asProofs(50-52)ProviderInformation(78-84)FeatureOptions(85-85)LogConsumer(86-90)SessionManagement(91-99)ReclaimAppInfo(100-100)ReclaimPlatformException(119-142)ReclaimVerificationException(144-224)src/specs/NativeInappRnSdk.ts (10)
Request(33-109)Response(114-129)VerificationOptions(322-325)SessionInformation(5-27)Overrides(301-308)ProviderInformation(131-135)FeatureOptions(140-184)LogConsumer(186-203)SessionManagement(226-234)ReclaimAppInfo(208-224)
src/types.ts (3)
src/index.ts (16)
ReclaimVerification(12-52)Request(57-57)Response(58-58)Proof(60-60)isProof(65-65)asProofs(66-66)VerificationOptions(68-69)OverrideConfig(81-81)ExceptionType(82-82)ExceptionType(83-83)ReclaimPlatformException(84-85)ReclaimPlatformException(86-87)ReclaimVerificationException(88-89)ReclaimVerificationException(90-91)Platform(92-92)Platform(93-93)lib/module/index.js (8)
ReclaimVerification(14-41)ReclaimResult(45-45)isProof(47-47)asProofs(48-48)ExceptionType(50-50)ReclaimPlatformException(51-51)ReclaimVerificationException(52-52)Platform(53-53)lib/commonjs/index.js (7)
ReclaimResult(49-49)isProof(51-51)asProofs(52-52)ExceptionType(54-54)ReclaimPlatformException(55-55)ReclaimVerificationException(56-56)Platform(57-57)
🪛 Biome (1.9.4)
src/types.ts
[error] 87-87: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (2)
lib/typescript/commonjs/src/index.d.ts (1)
1-48: Types now delegated to a single source of truth – great movePointing every public alias to
./typeseliminates the circular-dependency nightmare and keeps generated typings minimal. I don’t see any broken references or visibility regressions; the d.ts stays API-compatible.lib/typescript/module/src/index.d.ts (1)
1-48: d.ts refactor consistent with CommonJS build – looks solidThe module build mirrors the commonjs typings and therefore keeps the two artefacts in sync. No issues spotted.
src/types.ts
Outdated
| constructor(message: string, innerError: Error) { | ||
| super(message); | ||
| this.innerError = innerError; | ||
| this.reason = innerError.message; | ||
| if ('userInfo' in innerError) { | ||
| const details: any = innerError.userInfo; | ||
| this.details = details; | ||
| if ('message' in details) { | ||
| this.reason = details.message || this.reason; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve prototype chain in ReclaimPlatformException
For subclasses of Error, instanceof can fail in transpiled ES5/ES2015 output unless the prototype chain is manually restored. Add the same Object.setPrototypeOf pattern used above.
constructor(message: string, innerError: Error) {
super(message);
+ Object.setPrototypeOf(this, new.target.prototype);
this.innerError = innerError;📝 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.
| constructor(message: string, innerError: Error) { | |
| super(message); | |
| this.innerError = innerError; | |
| this.reason = innerError.message; | |
| if ('userInfo' in innerError) { | |
| const details: any = innerError.userInfo; | |
| this.details = details; | |
| if ('message' in details) { | |
| this.reason = details.message || this.reason; | |
| } | |
| } | |
| } | |
| constructor(message: string, innerError: Error) { | |
| super(message); | |
| Object.setPrototypeOf(this, new.target.prototype); | |
| this.innerError = innerError; | |
| this.reason = innerError.message; | |
| if ('userInfo' in innerError) { | |
| const details: any = innerError.userInfo; | |
| this.details = details; | |
| if ('message' in details) { | |
| this.reason = details.message || this.reason; | |
| } | |
| } | |
| } |
src/types.ts
Outdated
| 'constructor'( | ||
| message: string, | ||
| innerError: Error, | ||
| type: ExceptionType, | ||
| sessionId: string, | ||
| didSubmitManualVerification: boolean, | ||
| reason: string | ||
| ) { | ||
| super(message); | ||
| this.innerError = innerError; | ||
| this.type = type; | ||
| this.sessionId = sessionId; | ||
| this.didSubmitManualVerification = didSubmitManualVerification; | ||
| this.reason = reason; | ||
| } |
There was a problem hiding this comment.
'constructor' is not a real constructor – the class will be instantiated with an empty default constructor
Because the method name is quoted, TypeScript treats it as a normal method, not the class constructor.
• None of the fields are initialised when new ReclaimVerificationException() is called, leading to undefined properties at runtime.
• Compilation passes only in loose-mode emit but the logic is broken.
Fix by declaring a proper constructor and, for reliable subclassing of Error, set the prototype explicitly:
- 'constructor'(
+ constructor(
message: string,
innerError: Error,
type: ExceptionType,
sessionId: string,
didSubmitManualVerification: boolean,
reason: string
) {
super(message);
+ // Fix prototype chain for `instanceof` checks in transpiled output
+ Object.setPrototypeOf(this, new.target.prototype);
this.innerError = innerError;
this.type = type;
this.sessionId = sessionId;
this.didSubmitManualVerification = didSubmitManualVerification;
this.reason = reason;
}
src/platform.ts
Outdated
| const onLogsListener = logConsumer?.onLogs; | ||
| let logConsumerRequest = !logConsumer | ||
| ? undefined | ||
| : { | ||
| enableLogHandler: !!onLogsListener, | ||
| canSdkCollectTelemetry: logConsumer?.canSdkCollectTelemetry, | ||
| canSdkPrintLogs: logConsumer?.canSdkPrintLogs, | ||
| }; | ||
| if (onLogsListener) { | ||
| this.disposeLogListener(); | ||
| const cancel = () => { | ||
| this.previousLogSubscription?.remove(); | ||
| this.previousLogSubscription = null; | ||
| }; | ||
| this.previousLogSubscription = NativeReclaimInappModule.onLogs((arg) => { | ||
| onLogsListener(arg, cancel); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log listener leak mirrors the provider leak
disposeLogListener() is only called when a new onLogs handler is supplied.
If the handler is removed in a later call we never unsubscribe.
Follow the same pattern as the provider fix – dispose first, then (maybe) add the new subscription.
src/platform.ts
Outdated
| let sessionManagementRequest = !sessionManagement | ||
| ? undefined | ||
| : { | ||
| // A handler is provided, so we don't let SDK manage sessions | ||
| enableSdkSessionManagement: false, | ||
| }; | ||
| if (sessionManagement) { | ||
| this.disposeSessionManagement(); | ||
| let sessionCreateSubscription = | ||
| NativeReclaimInappModule.onSessionCreateRequest(async (event) => { | ||
| const replyId = event.replyId; | ||
| try { | ||
| let result = await sessionManagement.onSessionCreateRequest(event); | ||
| NativeReclaimInappModule.reply(replyId, result); | ||
| } catch (error) { | ||
| console.error(error); | ||
| NativeReclaimInappModule.reply(replyId, false); | ||
| } | ||
| }); | ||
| let sessionUpdateSubscription = | ||
| NativeReclaimInappModule.onSessionUpdateRequest(async (event) => { | ||
| const replyId = event.replyId; | ||
| try { | ||
| let result = await sessionManagement.onSessionUpdateRequest(event); | ||
| NativeReclaimInappModule.reply(replyId, result); | ||
| } catch (error) { | ||
| console.error(error); | ||
| NativeReclaimInappModule.reply(replyId, false); | ||
| } | ||
| }); | ||
| let sessionLogsSubscription = NativeReclaimInappModule.onSessionLogs( | ||
| (event) => { | ||
| try { | ||
| sessionManagement.onLog(event); | ||
| } catch (error) { | ||
| console.error(error); | ||
| } | ||
| } | ||
| this.previousProviderRequestCancelCallback = null; | ||
| ); | ||
| const cancel = () => { | ||
| sessionCreateSubscription.remove(); | ||
| sessionUpdateSubscription.remove(); | ||
| sessionLogsSubscription.remove(); | ||
| }; | ||
| this.previousSessionManagementCancelCallback = cancel; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Session-management subscriptions also need unconditional disposal
For consistency and to avoid triple leaks (create/update/log), call disposeSessionManagement() before checking sessionManagement.
You can then remove the call inside the branch.
src/platform.ts
Outdated
| override async setVerificationOptions( | ||
| options?: ReclaimVerification.VerificationOptions | null | ||
| ): Promise<void> { | ||
| let args: NativeReclaimInappModuleTypes.VerificationOptions | null = null; | ||
| if (options) { | ||
| let canUseAttestorAuthenticationRequest = | ||
| options.fetchAttestorAuthenticationRequest != null; | ||
| args = { | ||
| canDeleteCookiesBeforeVerificationStarts: | ||
| options.canDeleteCookiesBeforeVerificationStarts, | ||
| canUseAttestorAuthenticationRequest: | ||
| canUseAttestorAuthenticationRequest, | ||
| }; | ||
| if (canUseAttestorAuthenticationRequest) { | ||
| this.disposeAttestorAuthRequestListener(); | ||
| let attestorAuthRequestSubscription = | ||
| NativeReclaimInappModule.onReclaimAttestorAuthRequest( | ||
| async (event) => { | ||
| let result = await options.fetchAttestorAuthenticationRequest( | ||
| event.reclaimHttpProviderJsonString | ||
| ); | ||
| NativeReclaimInappModule.replyWithString(event.replyId, result); | ||
| } | ||
| } | ||
| try { | ||
| return await NativeReclaimInappModule.setVerificationOptions({ | ||
| options: args | ||
| }); | ||
| } catch (error) { | ||
| throw new ReclaimVerification.ReclaimPlatformException("Failed to set verification options", error as Error); | ||
| } | ||
| ); | ||
| const cancel = () => { | ||
| attestorAuthRequestSubscription.remove(); | ||
| }; | ||
| this.previousAttestorAuthRequestCancelCallback = cancel; | ||
| } | ||
| } | ||
| try { |
There was a problem hiding this comment.
setVerificationOptions leaks attestor-auth listener when options are cleared
disposeAttestorAuthRequestListener() is executed only when a new
fetchAttestorAuthenticationRequest is provided.
Add an unconditional call at the top of the method so the listener is removed when the consumer passes null or omits the field.
- override async setVerificationOptions(
+ override async setVerificationOptions(
options?: ReclaimVerification.VerificationOptions | null
): Promise<void> {
+ // clean up any previous listener first
+ this.disposeAttestorAuthRequestListener();📝 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.
| override async setVerificationOptions( | |
| options?: ReclaimVerification.VerificationOptions | null | |
| ): Promise<void> { | |
| let args: NativeReclaimInappModuleTypes.VerificationOptions | null = null; | |
| if (options) { | |
| let canUseAttestorAuthenticationRequest = | |
| options.fetchAttestorAuthenticationRequest != null; | |
| args = { | |
| canDeleteCookiesBeforeVerificationStarts: | |
| options.canDeleteCookiesBeforeVerificationStarts, | |
| canUseAttestorAuthenticationRequest: | |
| canUseAttestorAuthenticationRequest, | |
| }; | |
| if (canUseAttestorAuthenticationRequest) { | |
| this.disposeAttestorAuthRequestListener(); | |
| let attestorAuthRequestSubscription = | |
| NativeReclaimInappModule.onReclaimAttestorAuthRequest( | |
| async (event) => { | |
| let result = await options.fetchAttestorAuthenticationRequest( | |
| event.reclaimHttpProviderJsonString | |
| ); | |
| NativeReclaimInappModule.replyWithString(event.replyId, result); | |
| } | |
| } | |
| try { | |
| return await NativeReclaimInappModule.setVerificationOptions({ | |
| options: args | |
| }); | |
| } catch (error) { | |
| throw new ReclaimVerification.ReclaimPlatformException("Failed to set verification options", error as Error); | |
| } | |
| ); | |
| const cancel = () => { | |
| attestorAuthRequestSubscription.remove(); | |
| }; | |
| this.previousAttestorAuthRequestCancelCallback = cancel; | |
| } | |
| } | |
| try { | |
| override async setVerificationOptions( | |
| options?: ReclaimVerification.VerificationOptions | null | |
| ): Promise<void> { | |
| // clean up any previous listener first | |
| this.disposeAttestorAuthRequestListener(); | |
| let args: NativeReclaimInappModuleTypes.VerificationOptions | null = null; | |
| if (options) { | |
| let canUseAttestorAuthenticationRequest = | |
| options.fetchAttestorAuthenticationRequest != null; | |
| args = { | |
| canDeleteCookiesBeforeVerificationStarts: | |
| options.canDeleteCookiesBeforeVerificationStarts, | |
| canUseAttestorAuthenticationRequest: | |
| canUseAttestorAuthenticationRequest, | |
| }; | |
| if (canUseAttestorAuthenticationRequest) { | |
| this.disposeAttestorAuthRequestListener(); | |
| let attestorAuthRequestSubscription = | |
| NativeReclaimInappModule.onReclaimAttestorAuthRequest( | |
| async (event) => { | |
| let result = await options.fetchAttestorAuthenticationRequest( | |
| event.reclaimHttpProviderJsonString | |
| ); | |
| NativeReclaimInappModule.replyWithString(event.replyId, result); | |
| } | |
| ); | |
| const cancel = () => { | |
| attestorAuthRequestSubscription.remove(); | |
| }; | |
| this.previousAttestorAuthRequestCancelCallback = cancel; | |
| } | |
| } | |
| try { |
src/platform.ts
Outdated
| let providerCallback = provider?.callback; | ||
| let providerOverride = !provider | ||
| ? null | ||
| : { | ||
| url: provider?.url, | ||
| jsonString: provider?.jsonString, | ||
| canFetchProviderInformationFromHost: !!providerCallback, | ||
| }; | ||
| if (providerCallback) { | ||
| this.disposeProviderRequestListener(); | ||
| let providerRequestSubscription = | ||
| NativeReclaimInappModule.onProviderInformationRequest(async (event) => { | ||
| try { | ||
| let result = await providerCallback(event); | ||
| NativeReclaimInappModule.replyWithString(event.replyId, result); | ||
| } catch (error) { | ||
| console.error(error); | ||
| NativeReclaimInappModule.replyWithString(event.replyId, ''); | ||
| } | ||
| }); | ||
| const cancel = () => { | ||
| providerRequestSubscription.remove(); | ||
| }; | ||
| this.previousProviderRequestCancelCallback = cancel; | ||
| } |
There was a problem hiding this comment.
Previous provider listener is never disposed when the new overrides omit callback
disposeProviderRequestListener() is only invoked inside the if (providerCallback) branch.
Scenario: call setOverrides once with a callback, then call again without it.
The first subscription remains active and will leak memory / deliver events to stale JS code.
@@ override async setOverrides({
- let providerCallback = provider?.callback;
+ // Always clean up previous listeners before we configure the new ones
+ this.disposeProviderRequestListener();
+ let providerCallback = provider?.callback;Apply the same unconditional cleanup to log and sessionManagement listeners to keep the lifecycle symmetrical.
package.json
Outdated
| @@ -99,7 +98,6 @@ | |||
| "workspaces": [ | |||
| "example" | |||
| ], | |||
| "packageManager": "[email protected]", | |||
There was a problem hiding this comment.
Hey, the yarn & yarn workspace is used for working on the sdk locally. I'll rename the example to avoid that confusion. RN devs should refer sample from samples/example_new_arch
pnpm-lock.yaml
Outdated
There was a problem hiding this comment.
This should be ignored. Can you put this in .gitignore?
|
@tx-tomcat, before going ahead, can you share how to reproduce the circular dependency problem? |
|
A circular dependency error was happening with the related inapp capacitor sdk - this was solved with this commit: reclaimprotocol/reclaim-inapp-capacitor-sdk@d4fba25. That was solved by moving platform.ts code in index.ts. Can doing a similar thing here can solve this issue for you? |
Let me check it |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/index.ts (1)
210-233:⚠️ Potential issueTypeScript source has the same illegal class definition
Quoted method name
'constructor'is rejected by the TypeScript compiler (TS1138: Parameter declaration expected.) and by browsers/Node at runtime.Replace with a proper constructor:
-export class ReclaimVerificationException extends Error { - readonly 'innerError': Error; - … - 'constructor'( +export class ReclaimVerificationException extends Error { + readonly innerError: Error; + … + constructor(Remove quotes from the other static helpers as well (or make them computed names).
♻️ Duplicate comments (2)
lib/module/index.js (2)
94-122: Same syntax error as the CommonJS buildThe ES-module bundle also contains invalid quoted method names (
'constructor','fromTypeName', …).
Apply the fix proposed inlib/commonjs/index.js.
198-233: Replicate unconditional listener disposal
setOverrides()repeats the leak pattern described in the CommonJS file.
Please apply the same refactor (dispose listeners at method entry).
🧹 Nitpick comments (2)
lib/commonjs/index.js (1)
154-160: Preferconsole.errorfor error reportingThe catch block logs thrown errors with
console.info, which can easily be missed in production debugging.
Switch toconsole.error(or a structured logger) to surface failures clearly.src/index.ts (1)
321-338: Log errors withconsole.errorinstead ofconsole.infoConsistent, high-severity logging helps debugging native bridge failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
lib/commonjs/index.js.mapis excluded by!**/*.maplib/module/index.js.mapis excluded by!**/*.maplib/typescript/commonjs/src/index.d.ts.mapis excluded by!**/*.maplib/typescript/module/src/index.d.ts.mapis excluded by!**/*.map
📒 Files selected for processing (6)
lib/commonjs/index.js(3 hunks)lib/module/index.js(2 hunks)lib/typescript/commonjs/src/index.d.ts(3 hunks)lib/typescript/module/src/index.d.ts(3 hunks)src/index.ts(9 hunks)src/platform.ts(0 hunks)
💤 Files with no reviewable changes (1)
- src/platform.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/typescript/commonjs/src/index.d.ts
- lib/typescript/module/src/index.d.ts
🧰 Additional context used
🪛 Biome (1.9.4)
lib/commonjs/index.js
[error] 8-8: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 144-144: 'ReclaimVerification' is a class.
'ReclaimVerification' is defined here.
(lint/suspicious/noClassAssign)
lib/module/index.js
[error] 138-138: 'ReclaimVerification' is a class.
'ReclaimVerification' is defined here.
(lint/suspicious/noClassAssign)
🔇 Additional comments (1)
lib/commonjs/index.js (1)
204-233: Listener clean-up does not run when overrides are removed
disposeProviderRequestListener()/disposeLogListener()/disposeSessionManagement()are invoked only when the corresponding new override is supplied.
Therefore, callingsetOverrides({})after a previous call that did register listeners leaves the old subscriptions dangling → memory-leaks & duplicated callbacks.Refactor: perform unconditional clean-up at the beginning of
setOverrides().async setOverrides({ provider, featureOptions, logConsumer, sessionManagement, appInfo, capabilityAccessToken }) { - let providerCallback = provider?.callback; + // ---- always reset previous listeners first ----------------- + this.disposeProviderRequestListener(); + this.disposeLogListener(); + this.disposeSessionManagement(); + + let providerCallback = provider?.callback;[ suggest_essential_refactor ]
| 'constructor'(message, innerError, type, sessionId, didSubmitManualVerification, reason) { | ||
| super(message); | ||
| this.innerError = innerError; | ||
| this.type = type; | ||
| this.sessionId = sessionId; | ||
| this.didSubmitManualVerification = didSubmitManualVerification; | ||
| this.reason = reason; | ||
| } | ||
| static fromTypeName(name) { | ||
| static 'fromTypeName'(name) { | ||
| switch (name) { | ||
| case "cancelled": | ||
| case "org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Cancelled": | ||
| case 'cancelled': | ||
| case 'org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Cancelled': | ||
| return ExceptionType.Cancelled; | ||
| case "dismissed": | ||
| case "org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Dismissed": | ||
| case 'dismissed': | ||
| case 'org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Dismissed': | ||
| return ExceptionType.Dismissed; | ||
| case "sessionExpired": | ||
| case "org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.SessionExpired": | ||
| case 'sessionExpired': | ||
| case 'org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.SessionExpired': | ||
| return ExceptionType.SessionExpired; | ||
| case "failed": | ||
| case "org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Failed": | ||
| case 'failed': | ||
| case 'org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Failed': | ||
| return ExceptionType.Failed; | ||
| } |
There was a problem hiding this comment.
❌ Invalid class‐method syntax renders the bundle unusable
Every method whose name is wrapped in single quotes ('constructor', 'fromTypeName', 'fromError', 'isReclaimVerificationException') violates the ECMAScript grammar for ClassElementName.
Running this file in Node / React-Native will immediately throw a SyntaxError: Unexpected string, so the SDK cannot be required/imported.
Fix (example for the constructor – apply similarly to the other methods):
-class ReclaimVerificationException extends Error {
- 'constructor'(message, innerError, type, sessionId,
- didSubmitManualVerification, reason) {
+class ReclaimVerificationException extends Error {
+ constructor(message, innerError, type, sessionId,
+ didSubmitManualVerification, reason) {If you really need a quoted key, use a computed property instead: ['fromTypeName'](...) { … }, but there is no technical reason for quoting here.
📝 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.
| 'constructor'(message, innerError, type, sessionId, didSubmitManualVerification, reason) { | |
| super(message); | |
| this.innerError = innerError; | |
| this.type = type; | |
| this.sessionId = sessionId; | |
| this.didSubmitManualVerification = didSubmitManualVerification; | |
| this.reason = reason; | |
| } | |
| static fromTypeName(name) { | |
| static 'fromTypeName'(name) { | |
| switch (name) { | |
| case "cancelled": | |
| case "org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Cancelled": | |
| case 'cancelled': | |
| case 'org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Cancelled': | |
| return ExceptionType.Cancelled; | |
| case "dismissed": | |
| case "org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Dismissed": | |
| case 'dismissed': | |
| case 'org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Dismissed': | |
| return ExceptionType.Dismissed; | |
| case "sessionExpired": | |
| case "org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.SessionExpired": | |
| case 'sessionExpired': | |
| case 'org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.SessionExpired': | |
| return ExceptionType.SessionExpired; | |
| case "failed": | |
| case "org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Failed": | |
| case 'failed': | |
| case 'org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Failed': | |
| return ExceptionType.Failed; | |
| } | |
| constructor(message, innerError, type, sessionId, | |
| didSubmitManualVerification, reason) { | |
| super(message); | |
| this.innerError = innerError; | |
| this.type = type; | |
| this.sessionId = sessionId; | |
| this.didSubmitManualVerification = didSubmitManualVerification; | |
| this.reason = reason; | |
| } | |
| static 'fromTypeName'(name) { | |
| switch (name) { | |
| case 'cancelled': | |
| case 'org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Cancelled': | |
| return ExceptionType.Cancelled; | |
| case 'dismissed': | |
| case 'org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Dismissed': | |
| return ExceptionType.Dismissed; | |
| case 'sessionExpired': | |
| case 'org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.SessionExpired': | |
| return ExceptionType.SessionExpired; | |
| case 'failed': | |
| case 'org.reclaimprotocol.inapp_sdk.ReclaimVerification.ReclaimVerificationException.Failed': | |
| return ExceptionType.Failed; | |
| } |
| override async setOverrides({ | ||
| provider, | ||
| featureOptions, | ||
| logConsumer, | ||
| sessionManagement, | ||
| appInfo, | ||
| capabilityAccessToken, | ||
| }: ReclaimVerification.OverrideConfig) { | ||
| let providerCallback = provider?.callback; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Memory-leak / duplicate handler risk – mirror the CommonJS fix
setOverrides() should start by disposing all previous listeners, regardless of which overrides are supplied next, to avoid multiple active subscriptions.
override async setOverrides({ … }: ReclaimVerification.OverrideConfig) {
+ this.disposeProviderRequestListener();
+ this.disposeLogListener();
+ this.disposeSessionManagement();
+
let providerCallback = provider?.callback;📝 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.
| override async setOverrides({ | |
| provider, | |
| featureOptions, | |
| logConsumer, | |
| sessionManagement, | |
| appInfo, | |
| capabilityAccessToken, | |
| }: ReclaimVerification.OverrideConfig) { | |
| let providerCallback = provider?.callback; | |
| override async setOverrides({ | |
| provider, | |
| featureOptions, | |
| logConsumer, | |
| sessionManagement, | |
| appInfo, | |
| capabilityAccessToken, | |
| }: ReclaimVerification.OverrideConfig) { | |
| this.disposeProviderRequestListener(); | |
| this.disposeLogListener(); | |
| this.disposeSessionManagement(); | |
| let providerCallback = provider?.callback; |
|
Note: Tests can be ignored, they weren't properly setup for this repo. |
Resolve Circular dependency bugs and refactor code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor