Supportgemma4#232
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Gemma 4 models, updating the llama.rn dependency and implementing specialized parsing for thinking mode and tool-calling tokens. The model import process is also improved to address iOS temporary file eviction and URI resolution. Feedback focuses on ensuring proper URI decoding for file checks, aborting imports when files are missing, and maintaining UI consistency with project-specific alert utilities. Additionally, the reviewer suggests removing the heartbeat interval to reduce production overhead and scoping tool-usage heuristic changes specifically to Gemma 4 to avoid regressions in other models.
| const file1ExistsBefore = await RNFS.exists(file1.uri.replace('file://', '')); | ||
| const file2ExistsBefore = await RNFS.exists(file2.uri.replace('file://', '')); |
There was a problem hiding this comment.
The RNFS.exists check is performed on an encoded URI string. This will fail for file paths containing spaces or special characters (e.g., %20). The path should be decoded before checking for existence to prevent silent failures during the import process.
| const file1ExistsBefore = await RNFS.exists(file1.uri.replace('file://', '')); | |
| const file2ExistsBefore = await RNFS.exists(file2.uri.replace('file://', '')); | |
| const file1ExistsBefore = await RNFS.exists(decodeURIComponent(file1.uri.replace('file://', ''))); | |
| const file2ExistsBefore = await RNFS.exists(decodeURIComponent(file2.uri.replace('file://', ''))); |
References
- To prevent silent failures, ensure a directory exists (creating it if necessary) before performing operations on it.
| if (!mainExistsAfter || !mmProjExistsAfter) { | ||
| console.log('[IMPORT] ⚠️ FILES GONE after dialog! iOS deleted temp inbox files during the', dialogDurationMs, 'ms dialog wait.'); | ||
| console.log('[IMPORT] This confirms the temp file eviction bug. Need keepLocalCopy() before dialog.'); | ||
| } |
There was a problem hiding this comment.
The code detects that files might be missing after the dialog (likely due to iOS temp file eviction) but only logs the issue. To prevent silent failures, the process should be aborted with a user-friendly error message if the files are no longer available, rather than proceeding to a guaranteed failure in importLocalModel.
| if (!mainExistsAfter || !mmProjExistsAfter) { | |
| console.log('[IMPORT] ⚠️ FILES GONE after dialog! iOS deleted temp inbox files during the', dialogDurationMs, 'ms dialog wait.'); | |
| console.log('[IMPORT] This confirms the temp file eviction bug. Need keepLocalCopy() before dialog.'); | |
| } | |
| if (!mainExistsAfter || !mmProjExistsAfter) { | |
| setAlertState(showAlert('Import Failed', 'The selected files are no longer available. This can happen on iOS if the system clears temporary files while waiting for confirmation. Please try again.')); | |
| return; | |
| } |
References
- To prevent silent failures, ensure a directory exists (creating it if necessary) before performing operations on it.
| Alert.alert( | ||
| 'Import Vision Model?', | ||
| `Main model: ${mainFile.name}\nProjector: ${mmProjFile.name}\n\nIf these look wrong, cancel and rename your files.`, | ||
| [ | ||
| { text: 'Cancel', style: 'cancel', onPress: () => resolve(false) }, | ||
| { text: 'Import', onPress: () => resolve(true) }, | ||
| ], | ||
| { cancelable: false }, | ||
| ); |
There was a problem hiding this comment.
This code uses the native Alert.alert instead of the project's showAlert utility. For UI consistency and to adhere to the project's established patterns, please use setAlertState(showAlert(...)) as was done in the previous implementation.
| const heuristicMatch = shouldUseToolsForMessage(messageText, enabledTools); | ||
| const activeTools = (isRemote || heuristicMatch) ? enabledTools : []; | ||
| const systemPrompt = (!isRemote && activeTools.length > 0) ? `${basePrompt}${buildToolSystemPromptHint(activeTools)}` : basePrompt; | ||
| const activeTools = enabledTools; |
There was a problem hiding this comment.
The removal of the shouldUseToolsForMessage heuristic means all enabled tools are now passed to the model for every message. This increases prompt size and can lead to hallucinations. If this change is required specifically for Gemma 4, it should be scoped to that model to avoid regressions in efficiency for other models.
| const heartbeat = setInterval(() => { | ||
| console.log(`[IMPORT][scan] ⏱ HEARTBEAT — still running at ${elapsed()}, current step: ${heartbeatStep}`); | ||
| }, 3000); |
There was a problem hiding this comment.
The 'heartbeat' interval adds unnecessary noise and overhead in production. It should be removed or guarded by a debug flag.
1 participant
Summary
Type of Change
Screenshots / Screen Recordings
Android
iOS
Checklist
General
Testing
npm test)React Native Specific
project.pbxproj)SPACING/TYPOGRAPHYconstants from the themeuseThemedStylespattern (not inline or staticStyleSheet.create)FlatList/FlashList(not.map()insideScrollView)Performance & Models
/vs\\)Security
Related Issues
Additional Notes