From 25e15fb5a98779c5438b38eb2ae67e9e9113535a Mon Sep 17 00:00:00 2001 From: A Ghorbani Date: Sat, 2 Nov 2024 18:57:32 +0100 Subject: [PATCH 1/2] fix: resolve model list merge issue during app update --- src/store/ModelStore.ts | 28 +++++---- src/store/__tests__/ModelStore.test.ts | 80 ++++++++++++++++++++++++++ src/store/defaultModels.ts | 2 +- src/utils/__tests__/utils.test.ts | 75 +++++++++++++++++++++++- src/utils/index.ts | 28 +++++++++ 5 files changed, 199 insertions(+), 14 deletions(-) create mode 100644 src/store/__tests__/ModelStore.test.ts diff --git a/src/store/ModelStore.ts b/src/store/ModelStore.ts index fcdf001..0629275 100644 --- a/src/store/ModelStore.ts +++ b/src/store/ModelStore.ts @@ -9,7 +9,7 @@ import {computed, makeAutoObservable, ObservableMap, runInAction} from 'mobx'; import {CompletionParams, LlamaContext, initLlama} from '@pocketpalai/llama.rn'; import AsyncStorage from '@react-native-async-storage/async-storage'; -import {bytesToGB, hasEnoughSpace} from '../utils'; +import {bytesToGB, deepMerge, hasEnoughSpace} from '../utils'; import {defaultModels, MODEL_LIST_VERSION} from './defaultModels'; import {chatTemplates} from '../utils/chat'; @@ -78,20 +78,24 @@ class ModelStore { const existingModelIndex = mergedModels.findIndex( m => m.id === defaultModel.id, ); - // console.log('existingModelIndex: ', existingModelIndex); if (existingModelIndex !== -1) { // Merge existing model with new defaults - mergedModels[existingModelIndex] = { - ...defaultModel, // Defaults take precedence (for new fields or updates) - ...mergedModels[existingModelIndex], // User changes override defaults - chatTemplate: - mergedModels[existingModelIndex].chatTemplate || - defaultModel.chatTemplate, - completionSettings: - mergedModels[existingModelIndex].completionSettings || - defaultModel.completionSettings, - }; + const existingModel = mergedModels[existingModelIndex]; + + // Deep merge chatTemplate and completionSettings + existingModel.chatTemplate = deepMerge( + existingModel.chatTemplate || {}, + defaultModel.chatTemplate || {}, + ); + + existingModel.completionSettings = deepMerge( + existingModel.completionSettings || {}, + defaultModel.completionSettings || {}, + ); + + // Merge other properties + mergedModels[existingModelIndex] = existingModel; } else { // Add new model if it doesn't exist mergedModels.push(defaultModel); diff --git a/src/store/__tests__/ModelStore.test.ts b/src/store/__tests__/ModelStore.test.ts new file mode 100644 index 0000000..bdb416b --- /dev/null +++ b/src/store/__tests__/ModelStore.test.ts @@ -0,0 +1,80 @@ +jest.unmock('../ModelStore'); // This is not really needed, as only importing from store is mocked. +import {modelStore} from '../ModelStore'; +import {runInAction} from 'mobx'; +import {defaultModels} from '../defaultModels'; + +describe('ModelStore', () => { + beforeEach(() => { + jest.clearAllMocks(); + modelStore.models = []; // Clear models before each test + }); + + describe('mergeModelLists', () => { + it('should add missing default models to the existing model list', () => { + modelStore.models = []; // Start with no existing models + + runInAction(() => { + modelStore.mergeModelLists(); + }); + + expect(modelStore.models.length).toBeGreaterThan(0); + expect(modelStore.models).toEqual(expect.arrayContaining(defaultModels)); + }); + + it('should merge existing models with default models, adding any that are missing', () => { + const notExistedModel = defaultModels[0]; + modelStore.models = defaultModels.slice(1); // Start with all but the first model, so we can test if it's added back + expect(modelStore.models.length).toBe(defaultModels.length - 1); + expect(modelStore.models).not.toContainEqual(notExistedModel); + + runInAction(() => { + modelStore.mergeModelLists(); + }); + + expect(modelStore.models.length).toBeGreaterThan(0); + expect(modelStore.models).toContainEqual(notExistedModel); + }); + + it('should retain value of existing variables while merging new variables', () => { + const newDefaultModel = defaultModels[0]; + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const {temperature, ...completionSettingsWithoutTemperature} = + newDefaultModel.completionSettings; // Exclude temperature + + // Apply changes to the existing model: + // - chatTemplate.template: existing variable with a value different from the default + // - completionSettings.n_predict: existing variable with a value different from the default + // - temperature: new variable in the default model - not present in the existing model + const existingModel = { + ...newDefaultModel, + chatTemplate: { + ...newDefaultModel.chatTemplate, + template: 'existing', + }, + completionSettings: { + ...completionSettingsWithoutTemperature, // Use the completionSettings without temperature - simulates new parameters + n_predict: 101010, + }, + }; + + modelStore.models[0] = existingModel; + + runInAction(() => { + modelStore.mergeModelLists(); + }); + + expect(modelStore.models[0].chatTemplate).toEqual( + expect.objectContaining({ + template: 'existing', // Existing value should remain + }), + ); + + expect(modelStore.models[0].completionSettings).toEqual( + expect.objectContaining({ + n_predict: 101010, // Existing value should remain + temperature: newDefaultModel.completionSettings.temperature, // Non-existing value should be merged + }), + ); + }); + }); +}); diff --git a/src/store/defaultModels.ts b/src/store/defaultModels.ts index 45651c1..e571fb5 100644 --- a/src/store/defaultModels.ts +++ b/src/store/defaultModels.ts @@ -2,7 +2,7 @@ import {Model} from '../utils/types'; import {chatTemplates, defaultCompletionParams} from '../utils/chat'; import {Platform} from 'react-native'; -export const MODEL_LIST_VERSION = 5; +export const MODEL_LIST_VERSION = 6; export const defaultModels: Model[] = [ { diff --git a/src/utils/__tests__/utils.test.ts b/src/utils/__tests__/utils.test.ts index 6f0e1f8..1953e8e 100644 --- a/src/utils/__tests__/utils.test.ts +++ b/src/utils/__tests__/utils.test.ts @@ -1,4 +1,4 @@ -import {formatBytes, getTextSizeInBytes, unwrap} from '..'; +import {deepMerge, formatBytes, getTextSizeInBytes, unwrap} from '..'; describe('formatBytes', () => { it('formats bytes correctly when the size is 0', () => { @@ -38,3 +38,76 @@ describe('unwrap', () => { expect(unwrap(prop)).toStrictEqual(prop); }); }); + +describe('deepMerge', () => { + it('should merge two flat objects', () => { + const target = {a: 1, b: 2}; + const source = {b: 3, c: 4}; + const result = deepMerge(target, source); + expect(result).toEqual({a: 1, b: 2, c: 4}); // b should remain 2 + }); + + it('should merge nested objects', () => { + const target = {a: {b: 1}}; + const source = {a: {c: 2}}; + const result = deepMerge(target, source); + expect(result).toEqual({a: {b: 1, c: 2}}); // c should be added + }); + + it('should overwrite nested properties', () => { + const target = {a: {b: 1, c: 2}}; + const source = {a: {b: 3}}; + const result = deepMerge(target, source); + expect(result).toEqual({a: {b: 1, c: 2}}); // b should remain 1 + }); + + it('should handle arrays correctly', () => { + const target = {a: [1, 2]}; + const source = {a: [3, 4]}; + const result = deepMerge(target, source); + expect(result).toEqual({a: [1, 2]}); + }); + + it('should handle null values', () => { + const target = {a: null}; + const source = {a: {b: 1}}; + const result = deepMerge(target, source); + expect(result).toEqual({a: {b: 1}}); // Replaces null with the object + }); + + it('should handle flat to nested', () => { + const target = {a: 1, c: {d: 2, e: 3}}; + const source = {a: {b: 1}, c: 4}; + const result = deepMerge(target, source); + expect(result).toEqual({a: {b: 1}, c: 4}); + }); + + it('should not modify the original objects', () => { + const target = {a: 1}; + const source = {b: 2}; + deepMerge(target, source); + expect(target).toEqual({a: 1, b: 2}); + expect(source).toEqual({b: 2}); // Source should remain unchanged + }); + + it('should handle empty objects', () => { + const target = {}; + const source = {a: 1}; + const result = deepMerge(target, source); + expect(result).toEqual({a: 1}); // Merges from source + }); + + it('should handle deeply nested objects', () => { + const target = {a: {b: {c: 1}}}; + const source = {a: {b: {d: 2}}}; + const result = deepMerge(target, source); + expect(result).toEqual({a: {b: {c: 1, d: 2}}}); + }); + + it('should merge multiple levels of nesting', () => { + const target = {a: {b: {c: {d: 1, e: 3}}}}; + const source = {a: {b: {c: {e: 2, f: 4}}}}; + const result = deepMerge(target, source); + expect(result).toEqual({a: {b: {c: {d: 1, e: 3, f: 4}}}}); + }); +}); diff --git a/src/utils/index.ts b/src/utils/index.ts index 48ae454..949b621 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -313,4 +313,32 @@ export async function hasEnoughSpace(model: Model): Promise { } } +/** + * Merges properties from the source object into the target object deeply. + * Only sets properties that do not already exist in the target or if the types differ. + * + * @param target - The target object to merge properties into. + * @param source - The source object from which properties are taken. + * @returns The updated target object after merging. + */ +export const deepMerge = (target: any, source: any): any => { + for (const key in source) { + if (source.hasOwnProperty(key)) { + if (typeof source[key] === 'object' && source[key] !== null) { + // If the property is an object, recursively merge. + // If target[key] is not an object, it means the property type is different so we will replace it. + target[key] = + target[key] && typeof target[key] === 'object' ? target[key] : {}; + deepMerge(target[key], source[key]); + } else { + // Set the property in the target only if it doesn't exist or if the types differ + if (!(key in target) || typeof target[key] !== typeof source[key]) { + target[key] = source[key]; + } + } + } + } + return target; +}; + export const randId = () => Math.random().toString(36).substring(2, 11); From 3334012cc6d30324b87f6ba27dc8417e23ba5a00 Mon Sep 17 00:00:00 2001 From: A Ghorbani Date: Sat, 2 Nov 2024 19:29:14 +0100 Subject: [PATCH 2/2] chore: bump version --- android/app/build.gradle | 4 ++-- ios/PocketPal.xcodeproj/project.pbxproj | 8 ++++---- package.json | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/android/app/build.gradle b/android/app/build.gradle index b3d8c28..1cdd2c6 100644 --- a/android/app/build.gradle +++ b/android/app/build.gradle @@ -81,8 +81,8 @@ android { applicationId "com.pocketpalai" minSdkVersion rootProject.ext.minSdkVersion targetSdkVersion rootProject.ext.targetSdkVersion - versionCode 11 - versionName "1.4.4" + versionCode 12 + versionName "1.4.5" ndk { abiFilters "arm64-v8a", "x86_64" } diff --git a/ios/PocketPal.xcodeproj/project.pbxproj b/ios/PocketPal.xcodeproj/project.pbxproj index a56618b..a86bdb6 100644 --- a/ios/PocketPal.xcodeproj/project.pbxproj +++ b/ios/PocketPal.xcodeproj/project.pbxproj @@ -479,7 +479,7 @@ buildSettings = { ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; CLANG_ENABLE_MODULES = YES; - CURRENT_PROJECT_VERSION = 11; + CURRENT_PROJECT_VERSION = 12; DEVELOPMENT_TEAM = MYXGXY23Y6; ENABLE_BITCODE = NO; INFOPLIST_FILE = PocketPal/Info.plist; @@ -488,7 +488,7 @@ "$(inherited)", "@executable_path/Frameworks", ); - MARKETING_VERSION = 1.4.4; + MARKETING_VERSION = 1.4.5; OTHER_CPLUSPLUSFLAGS = ( "$(OTHER_CFLAGS)", "-DFOLLY_NO_CONFIG", @@ -521,7 +521,7 @@ buildSettings = { ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; CLANG_ENABLE_MODULES = YES; - CURRENT_PROJECT_VERSION = 11; + CURRENT_PROJECT_VERSION = 12; DEVELOPMENT_TEAM = MYXGXY23Y6; INFOPLIST_FILE = PocketPal/Info.plist; IPHONEOS_DEPLOYMENT_TARGET = 14.0; @@ -529,7 +529,7 @@ "$(inherited)", "@executable_path/Frameworks", ); - MARKETING_VERSION = 1.4.4; + MARKETING_VERSION = 1.4.5; OTHER_CPLUSPLUSFLAGS = ( "$(OTHER_CFLAGS)", "-DFOLLY_NO_CONFIG", diff --git a/package.json b/package.json index a91b684..14c9df0 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "PocketPal", - "version": "1.4.4", + "version": "1.4.5", "private": true, "scripts": { "prepare": "husky",