Lists: use separate add/remove functions instead of toggle

src/places.ts

@@ -156,16 +156,19 @@ export interface PlacesClient {
     delete(id: string): Promise<void>;
 
     /**
-     * Add or remove a place from a list.
+     * Add a place to a list.
      * @param listId - The slug ID of the list.
      * @param placeId - The ID of the place.
-     * @param geohash - The geohash of the place (needed for reference).
+     * @param geohash - The geohash of the place.
      */
-    togglePlace(
+    addPlace(listId: string, placeId: string, geohash: string): Promise<List>;
-      listId: string,
+
-      placeId: string,
+    /**
-      geohash: string
+     * Remove a place from a list.
-    ): Promise<List>;
+     * @param listId - The slug ID of the list.
+     * @param placeId - The ID of the place.
+     */
+    removePlace(listId: string, placeId: string): Promise<List>;
   };
 }
 
@@ -263,7 +266,7 @@ const Places = function (
       await privateClient.remove(`_lists/${id}`);
     },
 
-    async togglePlace(
+    async addPlace(
       listId: string,
       placeId: string,
       geohash: string
@@ -277,16 +280,33 @@ const Places = function (
 
       const index = list.placeRefs.findIndex((ref: any) => ref.id === placeId);
 
-      if (index !== -1) {
+      if (index === -1) {
-        // Remove
+        // Add only if not present
-        list.placeRefs.splice(index, 1);
-      } else {
-        // Add
         list.placeRefs.push({ id: placeId, geohash });
+        list.updatedAt = new Date().toISOString();
+        await privateClient.storeObject('list', path, list);
+      }
+
+      return list;
+    },
+
+    async removePlace(listId: string, placeId: string): Promise<List> {
+      const path = `_lists/${listId}`;
+      const list = (await privateClient.getObject(path)) as List;
+
+      if (!list) {
+        throw new Error(`List not found: ${listId}`);
+      }
+
+      const index = list.placeRefs.findIndex((ref: any) => ref.id === placeId);
+
+      if (index !== -1) {
+        // Remove only if present
+        list.placeRefs.splice(index, 1);
+        list.updatedAt = new Date().toISOString();
+        await privateClient.storeObject('list', path, list);
       }
 
-      list.updatedAt = new Date().toISOString();
-      await privateClient.storeObject('list', path, list);
       return list;
     },
 
@@ -327,6 +347,20 @@ const Places = function (
       if (!id || !geohash) {
         throw new Error('Both id and geohash are required to remove a place');
       }
+
+      // Cleanup: Remove this place from all lists
+      const allLists = await lists.getAll();
+      await Promise.all(
+        allLists.map(async (list) => {
+          const index = list.placeRefs.findIndex((ref: any) => ref.id === id);
+          if (index !== -1) {
+            list.placeRefs.splice(index, 1);
+            list.updatedAt = new Date().toISOString();
+            await privateClient.storeObject('list', `_lists/${list.id}`, list);
+          }
+        })
+      );
+
       const path = getPath(geohash, id);
       return privateClient.remove(path);
     },

test/places.test.ts

@@ -48,10 +48,39 @@ describe('Places Module', () => {
         const id = 'some-id';
         const geohash = 'u33dc0'; // u3/3d/
 
+        mockClient.getAll.mockResolvedValue([]);
+
         await places.remove(id, geohash);
 
         expect(mockClient.remove).toHaveBeenCalledWith('u3/3d/some-id');
       });
+
+      it('cleans up references from lists', async () => {
+        const id = 'some-id';
+        const geohash = 'u33dc0';
+
+        const mockLists = {
+          'to-go': {
+            id: 'to-go',
+            placeRefs: [{ id: 'some-id', geohash: 'u33dc0' }],
+          },
+          'to-do': { id: 'to-do', placeRefs: [] },
+        };
+        mockClient.getAll.mockResolvedValue(Object.values(mockLists));
+
+        await places.remove(id, geohash);
+
+        expect(mockClient.getAll).toHaveBeenCalledWith('_lists/');
+
+        // Expect "to-go" to be updated without the reference
+        expect(mockClient.storeObject).toHaveBeenCalledWith(
+          'list',
+          '_lists/to-go',
+          expect.objectContaining({
+            placeRefs: [],
+          })
+        );
+      });
     });
 
     describe('get', () => {
@@ -241,7 +270,7 @@ describe('Places Module', () => {
       });
     });
 
-    describe('togglePlace', () => {
+    describe('addPlace', () => {
       it('adds a place reference when not present', async () => {
         const now = '2023-01-03T00:00:00.000Z';
         vi.setSystemTime(new Date(now));
@@ -253,7 +282,7 @@ describe('Places Module', () => {
         };
         mockClient.getObject.mockResolvedValue(list);
 
-        await lists.togglePlace('hiking', 'place-123', 'w1q7');
+        await lists.addPlace('hiking', 'place-123', 'w1q7');
 
         expect(mockClient.storeObject).toHaveBeenCalledWith(
           'list',
@@ -267,6 +296,21 @@ describe('Places Module', () => {
         vi.useRealTimers();
       });
 
+      it('does nothing if place is already present', async () => {
+        const list = {
+          id: 'hiking',
+          placeRefs: [{ id: 'place-123', geohash: 'w1q7' }],
+          updatedAt: 'old-date',
+        };
+        mockClient.getObject.mockResolvedValue(list);
+
+        await lists.addPlace('hiking', 'place-123', 'w1q7');
+
+        expect(mockClient.storeObject).not.toHaveBeenCalled();
+      });
+    });
+
+    describe('removePlace', () => {
       it('removes a place reference when present', async () => {
         const now = '2023-01-04T00:00:00.000Z';
         vi.setSystemTime(new Date(now));
@@ -278,7 +322,7 @@ describe('Places Module', () => {
         };
         mockClient.getObject.mockResolvedValue(list);
 
-        await lists.togglePlace('hiking', 'place-123', 'w1q7');
+        await lists.removePlace('hiking', 'place-123');
 
         expect(mockClient.storeObject).toHaveBeenCalledWith(
           'list',
@@ -291,35 +335,80 @@ describe('Places Module', () => {
 
         vi.useRealTimers();
       });
+
+      it('does nothing if place is not present', async () => {
+        const list = {
+          id: 'hiking',
+          placeRefs: [],
+          updatedAt: 'old-date',
+        };
+        mockClient.getObject.mockResolvedValue(list);
+
+        await lists.removePlace('hiking', 'place-123');
+
+        expect(mockClient.storeObject).not.toHaveBeenCalled();
+      });
     });
 
     describe('initDefaults', () => {
-      it('creates default lists if they do not exist', async () => {
+      it('creates "Want to go" list if missing', async () => {
-        // Mock getObject to return null for both
         mockClient.getObject.mockResolvedValue(null);
 
         await lists.initDefaults();
 
-        // Should check and create "to-go"
         expect(mockClient.getObject).toHaveBeenCalledWith('_lists/to-go');
         expect(mockClient.storeObject).toHaveBeenCalledWith(
           'list',
           '_lists/to-go',
           expect.objectContaining({
             title: 'Want to go',
+            id: 'to-go',
+            color: '#ff00ff',
           })
         );
+      });
+
+      it('creates "To do" list if missing', async () => {
+        mockClient.getObject.mockResolvedValue(null);
+
+        await lists.initDefaults();
 
-        // Should check and create "to-do"
         expect(mockClient.getObject).toHaveBeenCalledWith('_lists/to-do');
         expect(mockClient.storeObject).toHaveBeenCalledWith(
           'list',
           '_lists/to-do',
           expect.objectContaining({
             title: 'To do',
+            id: 'to-do',
+            color: '#008000',
           })
         );
       });
+
+      it('does not overwrite existing lists', async () => {
+        // Mock that "to-go" exists but "to-do" does not
+        mockClient.getObject.mockImplementation(async (path: string) => {
+          if (path === '_lists/to-go')
+            return { id: 'to-go', title: 'Existing' };
+          return null;
+        });
+
+        await lists.initDefaults();
+
+        // Should NOT write to-go
+        expect(mockClient.storeObject).not.toHaveBeenCalledWith(
+          'list',
+          '_lists/to-go',
+          expect.anything()
+        );
+
+        // Should write to-do
+        expect(mockClient.storeObject).toHaveBeenCalledWith(
+          'list',
+          '_lists/to-do',
+          expect.anything()
+        );
+      });
     });
   });
 });