-
Notifications
You must be signed in to change notification settings - Fork 600
newArc and old Arc support and bugs fixes #575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
@greptile review |
|
@dev-amirzubair thank you for the fixes! Can you rebase and address (if any) comments from greptile |
Greptile Summary
Important Files Changed
Confidence score: 2/5
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (7)
-
android/src/oldarch/VoiceSpec.kt, line 18 (link)style: consider using
UnsupportedOperationExceptioninstead ofNotImplementedErrorfor better Java interoperabilityNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
react-native-voice.podspec, line 19 (link)logic: Repository URL changed from official react-native-voice organization to personal fork. This should be reverted before merging to main unless this is intentional for development. Is this repository URL change intended to be permanent, or should this point back to the official react-native-voice repository?
-
package.json, line 67 (link)logic: dev-sync script references old package path
@react-native-voice/voiceinstead of new package name@dev-amirzubair/react-native-voice -
example/src/VoiceTest.tsx, line 88-96 (link)style: All callback functions in dependency array will cause useEffect to re-run whenever any callback changes, potentially causing Voice event handlers to be re-registered unnecessarily. Would it be better to use useRef for stable callback references or remove some dependencies?
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
example/src/VoiceTest.tsx, line 72 (link)style: Parameter
eshould be typed instead of usinganyNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
src/index.ts, line 271 (link)logic:
cancelTranscription()callsVoice.cancelSpeech()but should callVoice.cancelTranscription() -
src/index.ts, line 189-194 (link)style:
startTranscription()uses old manual listener setup whilestart()uses new_setupListeners()approach, creating inconsistency
19 files reviewed, 7 comments
|
@safaiyeh, I have resolved the conflicts and addressed the comments. |
|
@dev-amirzubair After installing your plugin what's need to change , please kindly tell me |
|
@dev-amirzubair Thanks bro. Everything working fine in 0.82.0 version ..Great |
|
@dev-amirzubair @safaiyeh Any confirmations when we ca expect the new version update for the same ? also is the known issue of "required condition is false: IsFormatSampleRateAndChannelCountValid(format)" has been addressed in this PR or not ? |
|
@greptile review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (6)
-
src/VoiceModuleTypes.ts, line 53 (link)logic: Breaking change:
segmentsfield changed fromstring[]toTranscriptionSegment[]. This will break existing code that expects string arrays despite the backward compatibility claim. Should this be a major version bump since it's a breaking change to the segments API? -
src/VoiceModuleTypes.ts, line 47-49 (link)logic: Documentation incorrectly claims backward compatibility when the segments field type has fundamentally changed
-
example/src/VoiceTest.tsx, line 72-74 (link)syntax: setPitch receives potentially undefined value from e.value, causing type mismatch with useState<string>
-
README.md, line 18 (link)logic: Package name mismatch: shows
@react-native-voice/voicebut package.json indicates this fork is@dev-amirzubair/react-native-voiceShould this installation command reference your fork package name instead?
-
README.md, line 175 (link)logic: Plugin reference uses wrong package name - should match your fork name from package.json
-
src/index.ts, line 309-359 (link)style: Calling
_setupListeners()on every event setter creates performance overhead - listeners are removed and re-added repeatedly even when unchanged. Would it be more efficient to track which events have changed and only update listeners when necessary?Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
18 files reviewed, 6 comments
|
@greptile review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (7)
-
src/VoiceModuleTypes.ts, line 58 (link)logic: This is a breaking change despite the documentation claiming otherwise. Changing
segmentsfromstring[]toTranscriptionSegment[]will cause TypeScript errors and runtime failures for existing code that expects string arrays. Was this intended as a breaking change, or should a new field be added instead to maintain true backward compatibility? -
android/src/newarch/VoiceSpec.kt, line 54 (link)style: Inconsistent indentation - should align with other override methods
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
src/NativeVoiceAndroid.ts, line 6-8 (link)logic: Breaking change:
EXTRA_MAX_RESULTSchanged fromstringtonumberandEXTRA_PARTIAL_RESULTSfromstringtoboolean. Existing code passing strings will fail at runtime. -
package.json, line 41 (link)logic: The version range allows for React 16, 17, or 18 but the React peer dep requires 18+ - this creates a potential conflict
-
src/index.ts, line 125-127 (link)logic: Logic error:
!this._loaded && !this._listenersshould be||not&&- if either condition is false, the method should continue. Should this check if either_loadedis false OR_listenersis empty, rather than both conditions being false? -
src/index.ts, line 140-142 (link)logic: Same logical issue as destroy() method - should use
||instead of&& -
src/index.tslogic: Same logical issue repeated in stop() - early return condition should use
||
19 files reviewed, 7 comments
…e safety - Fix TranscriptionResultsEvent segments to support both string[] and TranscriptionSegment[] for backward compatibility - Fix SpeechOptions to support both legacy string and new number/boolean types - Fix Android Kotlin code to handle both string and number/boolean formats - Fix logic errors in stopTranscription(), cancel(), and cancelTranscription() methods (use || instead of &&) - Fix react-is version range to match React 18+ requirement (>=18.0.0) - Fix indentation in VoiceSpec.kt - Update documentation to clarify breaking changes
|
@greptile review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (5)
-
src/VoiceModuleTypes.ts, line 63 (link)style: The union type
string[]| TranscriptionSegment[]creates type ambiguity for consumers. Consider using a discriminated union with aformatfield or providing separate event types to distinguish between legacy and new formats. How will consumers differentiate between the two formats at runtime to safely access the data?Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
example/src/VoiceTestFuncComp.tsx, line 124 (link)style: Services state is not cleared in
_clearState()function - consider addingsetServices([])to maintain consistency with other state clearing. Should the services list be cleared when clearing other state, or should it persist across speech recognition sessions?Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
src/index.ts, line 33 (link)style: Variable
isBridgelessEnabledis declared but never used in the codeNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
src/index.ts, line 120 (link)style: Using
JSON.parse(JSON.stringify([]))to create empty array is unnecessarily complex - use[]insteadNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
src/index.ts, line 150 (link)style: Inconsistent array clearing: line 120 uses
JSON.parse(JSON.stringify([]))while line 150 also uses it - consider using[]for consistency and performanceNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
19 files reviewed, 5 comments
- Add isTranscriptionSegmentArray() type guard to help consumers differentiate between string[] and TranscriptionSegment[] formats - Update TranscriptionResultsEvent documentation to clarify runtime vs TypeScript types - Remove unused isBridgelessEnabled variable - Simplify array initialization (replace JSON.parse(JSON.stringify([])) with []) - Fix state clearing consistency in VoiceTestFuncComp example
|
@greptile review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (2)
-
src/VoiceModuleTypes.ts, line 102-104 (link)logic: Empty array check returns false for new format - this could be unexpected behavior since empty
TranscriptionSegment[]is still the new format. Should an empty segments array be considered the new TranscriptionSegment format rather than returning false? -
src/NativeVoiceAndroid.ts, line 50 (link)logic: Using
getEnforcing()here but the iOS implementation switched toget()with null checks - this inconsistency could cause issues if the Android module isn't available. Should this also useget()with proper null handling like the iOS implementation?
19 files reviewed, 2 comments
- Fix isTranscriptionSegmentArray() to return true for empty arrays (new format) - Change Android TurboModuleRegistry.getEnforcing() to get() for consistency with iOS - Add graceful fallback handling for Android module availability
|
@greptile review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (5)
-
src/NativeVoiceIOS.ts, line 30 (link)logic: Breaking change: switched from
getEnforcing()toget()which returns nullable instead of throwing. Consumer code must now handle null cases. Has the consuming code been updated to handle the nullable return value? -
src/NativeVoiceAndroid.ts, line 50-51 (link)logic: Breaking change: switching from
getEnforcing()toget()changes error handling behavior - the module can now return null instead of throwing, requiring null checks in consuming code. How is null handling implemented in the consuming code to prevent runtime errors when the TurboModule isn't available? -
src/index.ts, line 122 (link)logic: logic changed from
!this._loaded && !this._listenersto!this._loaded || this._listeners.length === 0- this may prevent operations when only one condition is false. Should this method return early if_loadedis true but there are no listeners, or vice versa? -
src/VoiceModuleTypes.ts, line 107-108 (link)logic: Empty array assumption may be incorrect - if the native implementation can legitimately return an empty string array, this logic would incorrectly identify it as
TranscriptionSegment[]format. Can the native iOS implementation ever return an empty string array, or is it guaranteed that empty arrays will always be in the new TranscriptionSegment format? -
src/VoiceModuleTypes.ts, line 112 (link)logic: Type guard relies only on presence of 'transcription' property - objects with this property but different structure could incorrectly pass the check
19 files reviewed, 5 comments
Turbo modules fixed
will work for both new and old architecture
also both Android and iOS