Fixed issues recommended by coderabbitai

actualbudget · Oct 25, 2024 · 0b226b8 · 0b226b8
1 parent b99d314
commit 0b226b8
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 22 deletions.
diff --git a/packages/desktop-client/src/components/modals/LoadBackupModal.tsx b/packages/desktop-client/src/components/modals/LoadBackupModal.tsx
@@ -147,8 +147,13 @@ export function LoadBackupModal({
                     isLoading={loading === 'backup'}
                     onPress={async () => {
                       setLoading('backup');
-                      await dispatch(makeBackup());
-                      setLoading(null);
+                      try {
+                        await dispatch(makeBackup());
+                      } catch (error) {
+                        console.error('Failed to create backup:', error);
+                      } finally {
+                        setLoading(null);
+                      }
                     }}
                   >
                     <Trans>Backup now</Trans>

diff --git a/packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx b/packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx
@@ -54,6 +54,9 @@ export function DuplicateFileModal({
     if (trimmedName.length > 100) {
       return t('Budget name is too long (max length 100)');
     }
+    if (!/^[a-zA-Z0-9 .\-_()]+$/.test(trimmedName)) {
+      return t('Budget name contains invalid characters');
+    }
     // Additional validation checks can go here
 
     return null;
@@ -75,22 +78,29 @@ export function DuplicateFileModal({
     if (!error) {
       setLoadingState(sync === 'cloudSync' ? 'cloud' : 'local');
 
-      await dispatch(
-        duplicateBudget({
-          id: 'id' in file ? file.id : undefined,
-          cloudId:
-            sync === 'cloudSync' && 'cloudFileId' in file
-              ? file.cloudFileId
-              : undefined,
-          oldName: file.name,
-          newName,
-          cloudSync: sync === 'cloudSync',
-          managePage,
-          loadBudget,
-        }),
-      );
-
-      setLoadingState(null);
+      try {
+        await dispatch(
+          duplicateBudget({
+            id: 'id' in file ? file.id : undefined,
+            cloudId:
+              sync === 'cloudSync' && 'cloudFileId' in file
+                ? file.cloudFileId
+                : undefined,
+            oldName: file.name,
+            newName,
+            cloudSync: sync === 'cloudSync',
+            managePage,
+            loadBudget,
+          }),
+        );
+      } catch (e) {
+        const newError =  new Error('Failed to duplicate budget');
+        if (onComplete) onComplete({ status: 'failed', error: newError });
+        else console.error('Failed to duplicate budget:', e);
+      } finally {
+        setLoadingState(null);
+      }
+
       if (onComplete) onComplete({ status: 'success' });
     } else {
       const failError = new Error(error);

diff --git a/packages/loot-core/src/server/backups.web.ts b/packages/loot-core/src/server/backups.web.ts
@@ -1,4 +1,5 @@
 // @ts-strict-ignore
+import { type Database } from '@jlongster/sql.js';
 import * as dateFns from 'date-fns';
 
 import * as connection from '../platform/server/connection';
@@ -116,10 +117,18 @@ export async function makeBackup(id: string) {
   );
 
   // Remove all the messages from the backup
-  const db = await sqlite.openDatabase(fs.join(budgetDir, backupId));
-  await sqlite.runQuery(db, 'DELETE FROM messages_crdt');
-  await sqlite.runQuery(db, 'DELETE FROM messages_clock');
-  sqlite.closeDatabase(db);
+  let db: Database;
+  try {
+    db = await sqlite.openDatabase(fs.join(budgetDir, backupId));
+    await sqlite.runQuery(db, 'DELETE FROM messages_crdt');
+    await sqlite.runQuery(db, 'DELETE FROM messages_clock');
+  } catch (error) {
+    console.error('Error cleaning up backup messages:', error);
+  } finally {
+    if (db) {
+      sqlite.closeDatabase(db);
+    }
+  }
 
   const toRemove = await updateBackups(await getBackups(id));
   for (const id of toRemove) {

diff --git a/packages/loot-core/src/server/main.ts b/packages/loot-core/src/server/main.ts
@@ -1820,6 +1820,12 @@ handlers['duplicate-budget'] = async function ({
   open,
 }): Promise<string> {
   if (!id) throw new Error('Unable to duplicate a budget that is not local.');
+  if (!newName?.trim()) {
+    throw new Error('Budget name is required and cannot be empty');
+  }
+  if (!/^[a-zA-Z0-9 .\-_()]+$/.test(newName)) {
+    throw new Error('Budget name contains invalid characters');
+  }
 
   const budgetDir = fs.getBudgetDir(id);