From 37cf47b3ddd683203eb1b928c85f82ba6ca849bd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A2u=20Cao?= <raucao@kosmos.org>
Date: Fri, 13 Mar 2026 14:57:19 +0400
Subject: [PATCH] Properly handle place removals

* Transition to OSM route or index instead of staying on ghost route/ID
  (closes sidebar if it was a custom place)
* Ensure save button and lists are in the correct state
---
 app/components/place-details.gjs              | 17 ++++--
 app/components/place-lists-manager.gjs        | 34 ++++++++++-
 app/services/storage.js                       | 11 ++++
 tests/acceptance/navigation-test.js           |  3 +
 tests/acceptance/search-test.js               |  9 +++
 .../components/place-details-test.gjs         | 59 ++++++++++++++++++-
 6 files changed, 124 insertions(+), 9 deletions(-)

diff --git a/app/components/place-details.gjs b/app/components/place-details.gjs
index f2dc19c..3738fb1 100644
--- a/app/components/place-details.gjs
+++ b/app/components/place-details.gjs
@@ -1,4 +1,5 @@
 import Component from '@glimmer/component';
+import { service } from '@ember/service';
 import { on } from '@ember/modifier';
 import { htmlSafe } from '@ember/template';
 import { humanizeOsmTag } from '../utils/format-text';
@@ -13,9 +14,14 @@ import { tracked } from '@glimmer/tracking';
 import { action } from '@ember/object';
 
 export default class PlaceDetails extends Component {
+  @service storage;
   @tracked isEditing = false;
   @tracked showLists = false;
 
+  get isSaved() {
+    return this.storage.isPlaceSaved(this.place.id || this.place.osmId);
+  }
+
   get place() {
     return this.args.place || {};
   }
@@ -38,7 +44,7 @@ export default class PlaceDetails extends Component {
 
   @action
   startEditing() {
-    if (!this.place.createdAt) return; // Only allow editing saved places
+    if (!this.isSaved) return; // Only allow editing saved places
     this.isEditing = true;
   }
 
@@ -276,7 +282,7 @@ export default class PlaceDetails extends Component {
           <button
             type="button"
             class={{if
-              this.place.createdAt
+              this.isSaved
               "btn btn-secondary"
               "btn btn-outline"
             }}
@@ -284,20 +290,21 @@ export default class PlaceDetails extends Component {
           >
             <Icon
               @name="bookmark"
-              @color={{if this.place.createdAt "currentColor" "#007bff"}}
+              @color={{if this.isSaved "currentColor" "#007bff"}}
             />
-            {{if this.place.createdAt "Saved" "Save"}}
+            {{if this.isSaved "Saved" "Save"}}
           </button>
 
           {{#if this.showLists}}
             <PlaceListsManager
               @place={{this.saveablePlace}}
               @onClose={{this.closeLists}}
+              @isSaved={{this.isSaved}}
             />
           {{/if}}
         </div>
 
-        {{#if this.place.createdAt}}
+        {{#if this.isSaved}}
           <button
             type="button"
             class="btn btn-outline"
diff --git a/app/components/place-lists-manager.gjs b/app/components/place-lists-manager.gjs
index eb01d4b..a5731b8 100644
--- a/app/components/place-lists-manager.gjs
+++ b/app/components/place-lists-manager.gjs
@@ -1,6 +1,7 @@
 import Component from '@glimmer/component';
 import { service } from '@ember/service';
 import { action } from '@ember/object';
+import { tracked } from '@glimmer/tracking';
 import { on } from '@ember/modifier';
 import { fn } from '@ember/helper';
 import { htmlSafe } from '@ember/template';
@@ -8,12 +9,15 @@ import onClickOutside from '../modifiers/on-click-outside';
 
 export default class PlaceListsManager extends Component {
   @service storage;
+  @service router;
+  @tracked _forceClear = false;
 
   get isSaved() {
-    return !!this.args.place.createdAt;
+    return this.args.isSaved;
   }
 
   get placeListIds() {
+    if (this._forceClear) return [];
     return this.args.place._listIds || [];
   }
 
@@ -30,7 +34,35 @@ export default class PlaceListsManager extends Component {
   @action
   async toggleSaved() {
     if (this.isSaved) {
+      const { osmId, osmType } = this.args.place;
+
       await this.storage.removePlace(this.args.place);
+
+      // Clean up the local object reference immediately to prevent UI flicker
+      // or stale state if the transition is delayed/cancelled.
+      if (this.args.place) {
+        this.args.place.id = null;
+        this.args.place.createdAt = null;
+        this.args.place._listIds = [];
+        this._forceClear = true;
+      }
+
+      // Transition immediately to the canonical state
+      if (osmId && osmType) {
+        // Create a transient copy that looks like a fresh OSM result
+        const rawPlace = { ...this.args.place };
+        delete rawPlace.id;
+        delete rawPlace.createdAt;
+        delete rawPlace._listIds;
+
+        // Transition to the place route using the raw object
+        // This updates the URL to 'osm:...' and renders immediately
+        this.router.transitionTo('place', rawPlace);
+      } else {
+        // Custom place deleted -> go home
+        this.router.transitionTo('index');
+      }
+
       if (this.args.onClose) this.args.onClose();
     } else {
       await this.storage.storePlace(this.args.place);
diff --git a/app/services/storage.js b/app/services/storage.js
index fa978f6..227fad0 100644
--- a/app/services/storage.js
+++ b/app/services/storage.js
@@ -282,11 +282,22 @@ export default class StorageService extends Service {
     let place = this.savedPlaces.find((p) => p.id && String(p.id) === strId);
     if (place) return place;
 
+    // Check placesInView as fallback
+    place = this.placesInView.find((p) => p.id && String(p.id) === strId);
+    if (place) return place;
+
     // Then search by OSM ID
     place = this.savedPlaces.find((p) => p.osmId && String(p.osmId) === strId);
+    if (place) return place;
+
+    place = this.placesInView.find((p) => p.osmId && String(p.osmId) === strId);
     return place;
   }
 
+  isPlaceSaved(id) {
+    return !!this.findPlaceById(id);
+  }
+
   async storePlace(placeData) {
     const savedPlace = await this.places.store(placeData);
 
diff --git a/tests/acceptance/navigation-test.js b/tests/acceptance/navigation-test.js
index f89eb9f..e76d4af 100644
--- a/tests/acceptance/navigation-test.js
+++ b/tests/acceptance/navigation-test.js
@@ -42,6 +42,9 @@ class MockStorageService extends Service {
   findPlaceById() {
     return null;
   }
+  isPlaceSaved() {
+    return false;
+  }
   loadPlacesInBounds() {
     return [];
   }
diff --git a/tests/acceptance/search-test.js b/tests/acceptance/search-test.js
index 405cb82..fd642b5 100644
--- a/tests/acceptance/search-test.js
+++ b/tests/acceptance/search-test.js
@@ -41,6 +41,9 @@ module('Acceptance | search', function (hooks) {
       findPlaceById() {
         return null;
       }
+      isPlaceSaved() {
+        return false;
+      }
       rs = {
         on: () => {},
       };
@@ -85,6 +88,9 @@ module('Acceptance | search', function (hooks) {
       findPlaceById() {
         return null;
       }
+      isPlaceSaved() {
+        return false;
+      }
       rs = {
         on: () => {},
       };
@@ -130,6 +136,9 @@ module('Acceptance | search', function (hooks) {
         if (id === '999') return this.savedPlaces[0];
         return null;
       }
+      isPlaceSaved(id) {
+        return !!this.findPlaceById(id);
+      }
       rs = {
         on: () => {},
       };
diff --git a/tests/integration/components/place-details-test.gjs b/tests/integration/components/place-details-test.gjs
index 9f48188..0ccbd9b 100644
--- a/tests/integration/components/place-details-test.gjs
+++ b/tests/integration/components/place-details-test.gjs
@@ -13,6 +13,14 @@ module('Integration | Component | place-details', function (hooks) {
       { id: 'to-do', title: 'To do', color: '#008000' },
     ];
 
+    isPlaceSaved(id) {
+      return false;
+    }
+
+    findPlaceById(id) {
+      return null;
+    }
+
     async storePlace(place) {
       return { ...place, id: '123', createdAt: new Date().toISOString() };
     }
@@ -28,6 +36,12 @@ module('Integration | Component | place-details', function (hooks) {
 
   hooks.beforeEach(function () {
     this.owner.register('service:storage', StorageService);
+
+    // Mock Router for all tests
+    class MockRouter extends Service {
+      transitionTo() {}
+    }
+    this.owner.register('service:router', MockRouter);
   });
 
   test('it formats coordinates correctly', async function (assert) {
@@ -95,6 +109,12 @@ module('Integration | Component | place-details', function (hooks) {
         storedPlace = place;
         return { ...place, id: 'new-id', createdAt: new Date().toISOString() };
       }
+      isPlaceSaved() {
+        return false;
+      }
+      findPlaceById() {
+        return null;
+      }
     }
     this.owner.register('service:storage', MockStorage);
 
@@ -126,12 +146,15 @@ module('Integration | Component | place-details', function (hooks) {
   });
 
   test('it handles removing a saved place via master toggle', async function (assert) {
-    let removedPlace = null;
+    let removedPlaceId = null;
 
     class MockStorage extends Service {
       lists = [];
       async removePlace(place) {
-        removedPlace = place;
+        removedPlaceId = place.id;
+      }
+      isPlaceSaved() {
+        return true;
       }
     }
     this.owner.register('service:storage', MockStorage);
@@ -160,7 +183,9 @@ module('Integration | Component | place-details', function (hooks) {
     // Click it to remove
     await click(masterToggle);
 
-    assert.strictEqual(removedPlace.id, 'saved-id', 'removePlace was called');
+    assert.strictEqual(removedPlaceId, 'saved-id', 'removePlace was called');
+
+    assert.deepEqual(place._listIds, [], '_listIds was cleared on the object');
   });
 
   test('it adds place to a list', async function (assert) {
@@ -175,6 +200,9 @@ module('Integration | Component | place-details', function (hooks) {
         listId = id;
         shouldAdd = add;
       }
+      isPlaceSaved() {
+        return true;
+      }
     }
     this.owner.register('service:storage', MockStorage);
 
@@ -202,4 +230,29 @@ module('Integration | Component | place-details', function (hooks) {
     assert.strictEqual(placeArg.id, 'p1');
     assert.true(shouldAdd);
   });
+
+  test('it respects storage service state over stale place object', async function (assert) {
+    class MockStorage extends Service {
+      lists = [];
+      isPlaceSaved() {
+        return false;
+      }
+      findPlaceById() {
+        return null;
+      }
+    }
+    this.owner.register('service:storage', MockStorage);
+
+    const place = {
+      id: 'stale-id',
+      title: 'Stale Place',
+      createdAt: '2023-01-01', // Looks saved
+    };
+
+    await render(<template><PlaceDetails @place={{place}} /></template>);
+
+    // Button should say "Save", not "Saved" because isPlaceSaved returns false
+    assert.dom('.actions button').hasText('Save');
+    assert.dom('.actions button').doesNotHaveClass('btn-secondary');
+  });
 });