feat: Visited list + local-first user-data framework + My Valley hub (#429)#451
Conversation
…429) Mark POIs as visited with progress tracking ("23 of 487 explored"), surfaced alongside Following and Trips in a new "My Valley" hub. - DB: migration 074 user_visits (mirrors user_poi_favorites) - API: routes/visited.js (list/stats/mark/unmark), visited[] in /auth/user - Framework: generalize the duplicated "POI-id list in localStorage, synced on login" machinery into createPoiIdListStore() (frontend) and syncPoiIdList() (backend); reuse for favorites + visited. Codified as a constitution rule (User Data: Local-First with Login Sync, 2.0.0->2.1.0) + docs/USER_DATA_FRAMEWORK.md - AuthContext: visited state + isVisited/toggleVisited (anon + sync) - UI: VisitedToggle in the POI sidebar; MyValley hub (progress + Visited / Following / Trips subtabs, Settings-style); TripsManager extracted from MyTripsModal and embedded in the Trips subtab; "My Trips" dropdown link removed; Following's news feed dropped (lives in notifications) - Spec 031-visited-list Closes #429 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Progress counter uses /api/visited/stats for signed-in users so N and M match the backend's point-based definition (HIGH) - Anonymous visited/following rows fall back to "Saved place" instead of being silently dropped when a name isn't in the loaded destinations (MEDIUM) - Rename MyTripsModal.css -> TripsManager.css to reflect the shared component Justified (not changed): /api/visited/stats count scan (small table + GIN index on poi_roles, infrequent); MyTripsModal retained (still opened by TripBuilder); authed/sync covered manually per the poiSubscriptions test convention (test container enforces auth). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request implements the local-first 'Visited' list feature and the 'My Valley' personalization hub, which aggregates a user's visited places, followed locations, and saved trips. It introduces a reusable local-first framework for POI-id lists, a database migration for user_visits, backend routes, and corresponding frontend components like MyValley and VisitedToggle. Feedback suggests passing the full pois list instead of just destinations to MyValley to correctly resolve names for non-point features (like trails) and filtering point POIs on the client side for accurate progress stats. Additionally, it is recommended to deduplicate the incoming POI IDs in the backend sync helper to avoid redundant database queries.
| <MyValley | ||
| open={showMyValley} | ||
| onClose={() => setShowMyValley(false)} | ||
| destinations={destinations} | ||
| /> |
There was a problem hiding this comment.
Pass the full pois list (which includes linear features and virtual organizations) instead of just destinations (which only contains point POIs). This ensures that anonymous users can correctly resolve names for visited or followed trails/rivers/organizations in the My Valley hub, rather than seeing them as generic 'Saved place' placeholders.
| <MyValley | |
| open={showMyValley} | |
| onClose={() => setShowMyValley(false)} | |
| destinations={destinations} | |
| /> | |
| <MyValley | |
| open={showMyValley} | |
| onClose={() => setShowMyValley(false)} | |
| pois={pois} | |
| /> |
| export default function MyValley({ open, onClose, destinations = [] }) { | ||
| const { isAuthenticated, visited, favorites, toggleVisited, toggleFavorite } = useAuth(); | ||
| const [view, setView] = useState('visited'); | ||
| const [visitedList, setVisitedList] = useState([]); | ||
| const [followingList, setFollowingList] = useState([]); | ||
| const [tripCount, setTripCount] = useState(0); | ||
| const [stats, setStats] = useState(null); | ||
| const [loading, setLoading] = useState(false); | ||
|
|
||
| // id -> name lookup over the markable locations the map already loaded, used | ||
| // to render names for anonymous (localStorage-only) visited/followed ids. | ||
| const nameById = useMemo(() => { | ||
| const map = new Map(); | ||
| for (const d of destinations) map.set(d.id, d.name); | ||
| return map; | ||
| }, [destinations]); | ||
|
|
||
| // Signed-in: trust the server's point-based counters (/api/visited/stats) so N | ||
| // and M match the spec's "markable point locations". Signed-out: derive from | ||
| // localStorage against the loaded destinations (point POIs) as the denominator. | ||
| const total = isAuthenticated && stats ? stats.total : destinations.length; | ||
| const exploredCount = isAuthenticated | ||
| ? (stats ? stats.visited : visitedList.length) | ||
| : visited.length; | ||
| const percent = total > 0 ? Math.min(100, Math.round((exploredCount / total) * 100)) : 0; |
There was a problem hiding this comment.
Rename the destinations prop to pois to accept the full list of POIs. Then, filter point POIs on the client side to compute progress stats. This fixes two major issues for anonymous users:
- Visited or followed trails/rivers/organizations will now display their correct names instead of showing up as generic 'Saved place' placeholders.
- The progress bar stats (explored count and total) will only count point POIs, preventing the progress stats from dropping or changing unexpectedly when the user signs in.
export default function MyValley({ open, onClose, pois = [] }) {
const { isAuthenticated, visited, favorites, toggleVisited, toggleFavorite } = useAuth();
const [view, setView] = useState('visited');
const [visitedList, setVisitedList] = useState([]);
const [followingList, setFollowingList] = useState([]);
const [tripCount, setTripCount] = useState(0);
const [stats, setStats] = useState(null);
const [loading, setLoading] = useState(false);
// id -> name lookup over all loaded locations, used
// to render names for anonymous (localStorage-only) visited/followed ids.
const nameById = useMemo(() => {
const map = new Map();
for (const p of pois) map.set(p.id, p.name);
return map;
}, [pois]);
// Filter point POIs for progress stats to match the server-side counters
const pointPoiIds = useMemo(() => {
return new Set(pois.filter(p => p.poi_roles?.includes('point')).map(p => p.id));
}, [pois]);
// Signed-in: trust the server's point-based counters (/api/visited/stats) so N
// and M match the spec's "markable point locations". Signed-out: derive from
// localStorage against the loaded point POIs as the denominator.
const total = isAuthenticated && stats ? stats.total : pointPoiIds.size;
const exploredCount = isAuthenticated
? (stats ? stats.visited : visitedList.length)
: visited.filter(id => pointPoiIds.has(id)).length;
const percent = total > 0 ? Math.min(100, Math.round((exploredCount / total) * 100)) : 0;| const poiIds = ids | ||
| .map(Number) | ||
| .filter(n => Number.isInteger(n) && n > 0) | ||
| .slice(0, 500); |
There was a problem hiding this comment.
Deduplicate the incoming ids array before slicing and querying the database. This prevents redundant elements from being sent to the database and avoids unnecessary conflict resolution overhead in the INSERT ... ON CONFLICT DO NOTHING query.
const poiIds = [...new Set(ids.map(Number).filter(n => Number.isInteger(n) && n > 0))].slice(0, 500);
Summary
user_visitstable (migration 074),routes/visited.js(list/stats/mark/unmark),visited[]in/auth/user, AuthContexttoggleVisited,VisitedToggle.createPoiIdListStore()(frontend) +syncPoiIdList()(backend), now shared by Favorites and Visited. Codified as a constitution rule ("User Data: Local-First with Login Sync", 2.0.0→2.1.0) +docs/USER_DATA_FRAMEWORK.md.TripsManagerwas extracted fromMyTripsModaland embedded in the Trips subtab; the standalone "My Trips" dropdown link was removed.031-visited-list.Review
/api/visited/stats; anon list no longer silently drops ids; CSS renamed). Perf/test findings justified (small indexed table; test container enforces auth, sync verified via dev API).Closes #429
Test plan
./run.sh build🤖 Generated with Claude Code