Skip to content

Commit 54fe13d

Browse files
authored
feat(models): improve local model upload flow visibility (#676)
1 parent 9acd264 commit 54fe13d

8 files changed

Lines changed: 247 additions & 20 deletions

File tree

__mocks__/stores/modelStore.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ class MockModelStore {
2727

2828
refreshDownloadStatuses: jest.Mock;
2929
addLocalModel: jest.Mock;
30+
removeModelByFullPath: jest.Mock;
3031
setNContext: jest.Mock;
3132
updateUseAutoRelease: jest.Mock;
3233
setNoGpuDevices: jest.Mock;
@@ -68,6 +69,7 @@ class MockModelStore {
6869
engine: observable.ref,
6970
refreshDownloadStatuses: false,
7071
addLocalModel: false,
72+
removeModelByFullPath: false,
7173
setNContext: false,
7274
updateUseAutoRelease: false,
7375

@@ -107,6 +109,7 @@ class MockModelStore {
107109
});
108110
this.refreshDownloadStatuses = jest.fn();
109111
this.addLocalModel = jest.fn();
112+
this.removeModelByFullPath = jest.fn();
110113
this.setNContext = jest.fn();
111114
this.updateUseAutoRelease = jest.fn();
112115
this.setNoGpuDevices = jest.fn();

src/locales/en.json

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,9 @@
187187
"fileAlreadyExists": "File already exists",
188188
"fileAlreadyExistsMessage": "A file with this name already exists. What would you like to do?",
189189
"replace": "Replace",
190-
"keepBoth": "Keep Both"
190+
"keepBoth": "Keep Both",
191+
"copyingModel": "Copying model file...",
192+
"copyFailed": "Failed to copy model file"
191193
},
192194
"labels": {
193195
"localModel": "Local",

src/screens/ModelsScreen/ModelCard/ModelCard.tsx

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import {
2020
IconButton,
2121
Text,
2222
TouchableRipple,
23-
ActivityIndicator,
2423
Snackbar,
2524
Switch,
2625
HelperText,
@@ -545,7 +544,9 @@ export const ModelCard: React.FC<ModelCardProps> = observer(
545544
) {
546545
return (
547546
<Button
547+
testID="loading-indicator"
548548
disabled={true}
549+
loading={true}
549550
style={[
550551
styles.primaryActionButton,
551552
{
@@ -554,12 +555,7 @@ export const ModelCard: React.FC<ModelCardProps> = observer(
554555
},
555556
]}
556557
textColor={theme.colors.btnPrimaryText}>
557-
<ActivityIndicator
558-
testID="loading-indicator"
559-
animating={true}
560-
color={theme.colors.btnPrimaryText}
561-
size="small"
562-
/>
558+
{''}
563559
</Button>
564560
);
565561
}
@@ -854,25 +850,31 @@ export const ModelCard: React.FC<ModelCardProps> = observer(
854850
)}
855851

856852
{/* Context Length */}
857-
{model.hfModel?.specs?.gguf?.context_length && (
853+
{(model.hfModel?.specs?.gguf?.context_length ||
854+
model.ggufMetadata?.context_length) && (
858855
<View style={styles.technicalDetailCard}>
859856
<Text style={styles.technicalDetailLabel}>
860857
{l10n.models.modelCard.labels.contextLength}
861858
</Text>
862859
<Text style={styles.technicalDetailValue}>
863-
{model.hfModel.specs.gguf.context_length.toLocaleString()}
860+
{(
861+
model.hfModel?.specs?.gguf?.context_length ||
862+
model.ggufMetadata?.context_length
863+
)?.toLocaleString()}
864864
</Text>
865865
</View>
866866
)}
867867

868868
{/* Architecture */}
869-
{model.hfModel?.specs?.gguf?.architecture && (
869+
{(model.hfModel?.specs?.gguf?.architecture ||
870+
model.ggufMetadata?.architecture) && (
870871
<View style={styles.technicalDetailCard}>
871872
<Text style={styles.technicalDetailLabel}>
872873
{l10n.models.modelCard.labels.architecture}
873874
</Text>
874875
<Text style={styles.technicalDetailValue}>
875-
{model.hfModel.specs.gguf.architecture}
876+
{model.hfModel?.specs?.gguf?.architecture ||
877+
model.ggufMetadata?.architecture}
876878
</Text>
877879
</View>
878880
)}

src/screens/ModelsScreen/ModelsScreen.tsx

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import 'react-native-get-random-values';
77
import {observer} from 'mobx-react-lite';
88
import * as RNFS from '@dr.pogodin/react-native-fs';
99
import {pick, types} from '@react-native-documents/picker';
10-
import {Portal} from 'react-native-paper';
10+
import {Portal, Snackbar} from 'react-native-paper';
1111

1212
import {useTheme} from '../../hooks';
1313

@@ -35,7 +35,7 @@ export const ModelsScreen: React.FC = observer(() => {
3535
const l10n = useContext(L10nContext);
3636
const [refreshing, setRefreshing] = useState<boolean>(false);
3737
const [hfSearchVisible, setHFSearchVisible] = useState(false);
38-
const [_, setTrigger] = useState<boolean>(false);
38+
const [isCopyingModel, setIsCopyingModel] = useState(false);
3939
const [selectedModel, setSelectedModel] = useState<Model | undefined>();
4040
const [settingsVisible, setSettingsVisible] = useState(false);
4141

@@ -106,7 +106,6 @@ export const ModelsScreen: React.FC = observer(() => {
106106
const onRefresh = async () => {
107107
setRefreshing(true);
108108
await modelStore.refreshDownloadStatuses();
109-
setTrigger(prev => !prev);
110109
setRefreshing(false);
111110
};
112111

@@ -183,6 +182,10 @@ export const ModelsScreen: React.FC = observer(() => {
183182
};
184183

185184
const handleAddLocalModel = async () => {
185+
if (isCopyingModel) {
186+
return;
187+
}
188+
186189
pick({
187190
type: Platform.OS === 'ios' ? 'public.data' : types.allFiles,
188191
})
@@ -228,6 +231,7 @@ export const ModelsScreen: React.FC = observer(() => {
228231
switch (choice) {
229232
case 'replace':
230233
await RNFS.unlink(permanentPath);
234+
modelStore.removeModelByFullPath(permanentPath);
231235
break;
232236
case 'keep':
233237
let counter = 1;
@@ -245,9 +249,18 @@ export const ModelsScreen: React.FC = observer(() => {
245249
}
246250
}
247251

248-
await RNFS.copyFile(file.uri, permanentPath);
249-
await modelStore.addLocalModel(permanentPath);
250-
setTrigger(prev => !prev);
252+
try {
253+
setIsCopyingModel(true);
254+
await RNFS.copyFile(file.uri, permanentPath);
255+
await modelStore.addLocalModel(permanentPath);
256+
} catch (e) {
257+
Alert.alert(
258+
l10n.models.fileManagement.copyFailed,
259+
e instanceof Error ? e.message : String(e),
260+
);
261+
} finally {
262+
setIsCopyingModel(false);
263+
}
251264
}
252265
})
253266
.catch(e => console.log('No file picked, error: ', e.message));
@@ -422,6 +435,13 @@ export const ModelsScreen: React.FC = observer(() => {
422435
visible={hfSearchVisible}
423436
onDismiss={() => setHFSearchVisible(false)}
424437
/>
438+
<Snackbar
439+
testID="copy-model-snackbar"
440+
visible={isCopyingModel}
441+
onDismiss={() => {}}
442+
duration={86400000}>
443+
{l10n.models.fileManagement.copyingModel}
444+
</Snackbar>
425445
<FABGroup
426446
onAddHFModel={() => setHFSearchVisible(true)}
427447
onAddLocalModel={handleAddLocalModel}

src/screens/ModelsScreen/__tests__/ModelsScreen.test.tsx

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,9 @@ describe('ModelsScreen', () => {
164164
expect(RNFS.unlink).toHaveBeenCalledWith(
165165
'/path/to/documents/models/local/mockModelFile.bin',
166166
);
167+
expect(modelStore.removeModelByFullPath).toHaveBeenCalledWith(
168+
'/path/to/documents/models/local/mockModelFile.bin',
169+
);
167170
expect(RNFS.copyFile).toHaveBeenCalled();
168171
expect(modelStore.addLocalModel).toHaveBeenCalled();
169172
});
@@ -263,6 +266,132 @@ describe('ModelsScreen', () => {
263266
});
264267
});
265268

269+
it('shows a copying snackbar while file is being copied', async () => {
270+
let resolveCopy!: () => void;
271+
(RNFS.copyFile as jest.Mock).mockImplementation(
272+
() =>
273+
new Promise<void>(resolve => {
274+
resolveCopy = resolve;
275+
}),
276+
);
277+
(RNFS.exists as jest.Mock).mockImplementation(async (path: string) => {
278+
if (path.includes('models/local')) {
279+
return false;
280+
}
281+
return true;
282+
});
283+
(pick as jest.Mock).mockResolvedValue([
284+
{uri: '/mock/file/path', name: 'mockModelFile.bin'},
285+
]);
286+
287+
const {getByTestId} = render(<ModelsScreen />);
288+
289+
// Open FAB, press local
290+
const fabGroup = getByTestId('fab-group');
291+
fireEvent.press(fabGroup);
292+
await waitFor(() => {
293+
expect(
294+
getByTestId('local-fab', {includeHiddenElements: true}),
295+
).toBeTruthy();
296+
});
297+
await act(async () => {
298+
fireEvent.press(getByTestId('local-fab', {includeHiddenElements: true}));
299+
});
300+
301+
// Snackbar should be visible while copy is pending
302+
await waitFor(() => {
303+
expect(getByTestId('copy-model-snackbar')).toBeTruthy();
304+
});
305+
306+
// Resolve the copy
307+
await act(async () => {
308+
resolveCopy();
309+
});
310+
});
311+
312+
it('shows an error alert when copy fails', async () => {
313+
const alertSpy = jest.spyOn(Alert, 'alert');
314+
(RNFS.copyFile as jest.Mock).mockRejectedValue(new Error('Disk full'));
315+
(RNFS.exists as jest.Mock).mockImplementation(async (path: string) => {
316+
if (path.includes('models/local')) {
317+
return false;
318+
}
319+
return true;
320+
});
321+
(pick as jest.Mock).mockResolvedValue([
322+
{uri: '/mock/file/path', name: 'mockModelFile.bin'},
323+
]);
324+
325+
const {getByTestId} = render(<ModelsScreen />);
326+
327+
const fabGroup = getByTestId('fab-group');
328+
fireEvent.press(fabGroup);
329+
await waitFor(() => {
330+
expect(
331+
getByTestId('local-fab', {includeHiddenElements: true}),
332+
).toBeTruthy();
333+
});
334+
await act(async () => {
335+
fireEvent.press(getByTestId('local-fab', {includeHiddenElements: true}));
336+
});
337+
338+
await waitFor(() => {
339+
expect(alertSpy).toHaveBeenCalledWith(expect.any(String), 'Disk full');
340+
});
341+
});
342+
343+
it('prevents double-tap by ignoring second add while copy is in progress', async () => {
344+
// First call: copy never resolves so isCopyingModel stays true
345+
(RNFS.copyFile as jest.Mock).mockImplementation(
346+
() => new Promise<void>(() => {}),
347+
);
348+
(RNFS.exists as jest.Mock).mockImplementation(async (path: string) => {
349+
if (path.includes('models/local')) {
350+
return false;
351+
}
352+
return true;
353+
});
354+
(pick as jest.Mock).mockResolvedValue([
355+
{uri: '/mock/file/path', name: 'mockModelFile.bin'},
356+
]);
357+
358+
const {getByTestId} = render(<ModelsScreen />);
359+
360+
// Open FAB and press local to start the first copy
361+
const fabGroup = getByTestId('fab-group');
362+
fireEvent.press(fabGroup);
363+
await waitFor(() => {
364+
expect(
365+
getByTestId('local-fab', {includeHiddenElements: true}),
366+
).toBeTruthy();
367+
});
368+
await act(async () => {
369+
fireEvent.press(getByTestId('local-fab', {includeHiddenElements: true}));
370+
});
371+
372+
// Verify first copy started
373+
await waitFor(() => {
374+
expect(pick).toHaveBeenCalledTimes(1);
375+
});
376+
377+
// Reset pick mock to detect if it gets called again
378+
(pick as jest.Mock).mockClear();
379+
380+
// Try to press local FAB again while copy is in progress
381+
fireEvent.press(fabGroup);
382+
await waitFor(() => {
383+
expect(
384+
getByTestId('local-fab', {includeHiddenElements: true}),
385+
).toBeTruthy();
386+
});
387+
await act(async () => {
388+
fireEvent.press(getByTestId('local-fab', {includeHiddenElements: true}));
389+
});
390+
391+
// pick should NOT have been called again — the early return guard blocks it
392+
expect(pick).not.toHaveBeenCalled();
393+
});
394+
266395
// Add tests for model filtering and grouping
267396
describe('Model filtering and grouping', () => {
268397
beforeEach(() => {

0 commit comments

Comments
 (0)