Conversation
martinzigrai
left a comment
martinzigrai
left a comment
There was a problem hiding this comment.
Nice work! 🚀 The logic seems solid. I just found a few spots that need a bit of polish. Left some comments below. 👇
| implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version" | ||
| implementation "org.jetbrains.kotlinx:kotlinx-serialization-json:1.4.1" | ||
| implementation "com.aheaditec.talsec.security:TalsecSecurity-Community-ReactNative:17.0.1" | ||
| implementation "com.aheaditec.talsec.security:TalsecSecurity-Community-ReactNative:18.0.1" |
There was a problem hiding this comment.
We already have 18.0.2, could you please use that version?
| } | ||
| } | ||
|
|
||
| private fun flushCache(registeredListener: WrapperExecutionStateListener) { |
There was a problem hiding this comment.
Just a suggestion 👀: Invoking the listener inside the synchronized block can lead to deadlocks. It might be safer to copy the events to a local list, clear the cache, and then dispatch them outside the lock.
| } | ||
| } | ||
|
|
||
| private fun flushCache(registeredListener: WrapperThreatListener) { |
There was a problem hiding this comment.
Same as in ExecutionStateDispatcher.
| @@ -218,17 +241,17 @@ class FreeraspReactNativeModule(private val reactContext: ReactApplicationContex | |||
|
|
|||
| @ReactMethod | |||
| fun storeExternalId( | |||
There was a problem hiding this comment.
The Talsec.storeExternalId method now enforces value restrictions (regex) and returns an ExternalIdResult instead of void. Your current implementation ignores this return value and always resolves the promise with "OK". This means if an invalid externalId is passed, the native side will reject it, but the React Native app will incorrectly think it was saved successfully.
| }; | ||
| eventsListener = eventEmitter.addListener(channel, listener); | ||
| eventsListener = eventEmitter.addListener(executionStateChannel, listener); | ||
| isInitializing = false; |
There was a problem hiding this comment.
If await removeRaspExecutionStateEventListener() or data fetching throws an error, isInitializing will remain true forever, blocking subsequent calls. I suggest wrapping the logic in a try { ... } finally { isInitializing = false; } block to ensure the lock is always released.
| }; | ||
| eventsListener = eventEmitter.addListener(channel, listener); | ||
| eventsListener = eventEmitter.addListener(threatChannel, listener); | ||
| isInitializing = false; |
There was a problem hiding this comment.
Same as in raspExecutionState.ts
2 participants
What's new
Describe the PR shortly
PR checklist
In-app checks:
README.mdis updated (if applicable)expoplugin is updated (if applicable)exampleapp is workingPre-release checks:
CHANGELOG.mdis updatedpackage.jsonversion is increasedreleaselabel is applied on PRTo be checked by maintainers:
sdkVersionproperty in logs is correctsdkPlatformproperty in logs is correctResolved issues
list of issues resolved by this PR
closes #136