-
-
Notifications
You must be signed in to change notification settings - Fork 80
feat: hidden persistence #1180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
feat: hidden persistence #1180
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| routeTypes: {}, | ||
| }, | ||
| hideList: new Set(), | ||
| hideList: getHiddenEntitySet(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enforce hide expiration during active sessions
Hidden entries are supposed to reappear after an hour, but the hide set is only initialized once at startup via getHiddenEntitySet() and never refreshed; the map filter just checks hideList.has(...) without reapplying the one‑hour TTL. In a session that stays open longer than an hour, anything you hide remains in hideList indefinitely, so markers never return unless the page is reloaded or “Clean Hidden” is clicked, breaking the advertised 1‑hour persistence window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements a persistent "Hide" feature that allows users to temporarily hide Pokemon, gyms, pokestops, nests, stations, and tappables for 1 hour, with the ability to restore them via Settings > Clean Hidden.
Key changes:
- New utility module for managing hidden entities with timestamp-based expiration
- Persistent storage using localStorage with automatic 1-hour cleanup
- User feedback via temporary snackbar notifications (limited to 3 displays)
- Settings UI to manually clear all hidden entities
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/pokemon/hiddenPokemon.js | New utility module providing localStorage-based persistence for hidden entities with timestamp tracking, automatic cleanup, and snackbar notification system |
| src/store/useMemory.js | Initialize hideList with persisted hidden entities from localStorage on app startup |
| src/features/tappable/TappablePopup.jsx | Update hide action to use persistent storage and show snackbar notification |
| src/features/station/StationPopup.jsx | Update hide action to use persistent storage and show snackbar notification |
| src/features/pokestop/PokestopPopup.jsx | Update hide action to use persistent storage and show snackbar notification |
| src/features/pokemon/PokemonPopup.jsx | Update hide action to use persistent storage (missing snackbar notification) |
| src/features/nest/NestPopup.jsx | Update hide action to use persistent storage and show snackbar notification |
| src/features/gym/GymPopup.jsx | Update hide action to use persistent storage and show snackbar notification |
| src/features/drawer/settings/index.jsx | Add "Clean Hidden" button to manually clear all hidden entities and reset state |
| packages/locales/lib/human/pl.json | Add Polish translations for "hidden_for_hour" and "clean_hidden" |
| packages/locales/lib/human/en.json | Add English translations for "hidden_for_hour" and "clean_hidden" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { usePokemonBackgroundVisual } from '@hooks/usePokemonBackgroundVisuals' | ||
| import { BackgroundCard } from '@components/popups/BackgroundCard' | ||
| import { getFormDisplay } from '@utils/getFormDisplay' | ||
| import { addHiddenEntity } from '@utils/pokemon/hiddenPokemon' |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The showHideSnackbar import is missing from this file. The handleHide function calls showHideSnackbar on line 113 (in TappablePopup.jsx), but PokemonPopup.jsx only imports addHiddenEntity. To be consistent with other popups and provide user feedback, this file needs to import showHideSnackbar as well.
| import { addHiddenEntity } from '@utils/pokemon/hiddenPokemon' | |
| import { addHiddenEntity } from '@utils/pokemon/hiddenPokemon' | |
| import { showHideSnackbar } from '@utils/showHideSnackbar' |
| return new Set(entries.map((e) => e.id)) | ||
| } | ||
|
|
||
| /** @type {{ current: number | null }} */ |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type annotation for snackbarTimer.current should be ReturnType<typeof setTimeout> instead of number. In browser environments, setTimeout returns a number, but the return type varies across environments. Using ReturnType<typeof setTimeout> ensures type safety and compatibility. Additionally, this type should match the type used for window.setTimeout on line 139.
| /** @type {{ current: number | null }} */ | |
| /** @type {{ current: ReturnType<typeof setTimeout> | null }} */ |
| export function clearHiddenEntities() { | ||
| try { | ||
| localStorage.removeItem(STORAGE_KEY) | ||
| } catch { | ||
| // localStorage may be disabled | ||
| } | ||
| } |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clearHiddenEntities function should also clear the snackbar count from localStorage. When users explicitly clear their hidden entities via the "Clean Hidden" button, they likely expect a full reset of the hide feature state, including the snackbar display count. Without clearing the snackbar count, users who have already seen the snackbar 3 times won't see it again even after clearing their hidden items, which could be confusing. Consider adding localStorage.removeItem(SNACKBAR_COUNT_KEY) to this function.
| snackbarRef.current.remove() | ||
| } | ||
|
|
||
| incrementSnackbarCount() |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incrementing the snackbar count before checking if the snackbar can be displayed creates a potential issue. If the DOM manipulation fails after incrementSnackbarCount is called on line 119, the count will be incremented but the user won't have seen the snackbar. This means users might not see all 3 snackbars they're entitled to. Consider moving incrementSnackbarCount to after the snackbar is successfully appended to the DOM on line 136, or wrap the increment in a try-catch that reverts on failure.
| "hidden_for_hour": "Hidden for an hour", | ||
| "clean_hidden": "Clean Hidden", |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The translations for 'hidden_for_hour' and 'clean_hidden' are only added to the English and Polish locale files. This creates an inconsistent user experience for users of other supported languages (de, es, fr, it, ja, nl, pt-br, ru, sv, th, tr, zh-tw). When these users trigger the hide action or access the settings, they will see untranslated keys instead of proper messages. All locale files in packages/locales/lib/human/ should include these new translation keys.
| */ | ||
| export function addHiddenEntity(id) { | ||
| const entries = cleanOldEntries(loadEntries()) | ||
| if (!entries.some((e) => e.id === id)) { |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an already-hidden entity is hidden again, its timestamp is not updated. If a user hides the same entity multiple times within the hour, they might expect the 1-hour timer to reset from the last hide action, but the current implementation on lines 56-58 only adds new entries and doesn't update the timestamp of existing entries. This could be confusing when an entity disappears close to the original 1-hour mark even though the user recently re-hid it. Consider updating the timestamp when the entry already exists by finding the existing entry and updating its 'ts' field.
| if (!entries.some((e) => e.id === id)) { | |
| const existing = entries.find((e) => e.id === id) | |
| if (existing) { | |
| existing.ts = Date.now() | |
| } else { |
| const handleHide = () => { | ||
| setAnchorEl(null) | ||
| useMemory.setState((prev) => ({ hideList: new Set(prev.hideList).add(id) })) | ||
| useMemory.setState({ hideList: addHiddenEntity(id) }) | ||
| } |
Copilot
AI
Dec 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PokemonPopup component is missing the showHideSnackbar call when hiding an entity. All other popup components (TappablePopup, StationPopup, PokestopPopup, NestPopup, GymPopup) call showHideSnackbar after adding a hidden entity to provide user feedback, but PokemonPopup only calls addHiddenEntity. This creates an inconsistent user experience where users don't receive feedback when hiding Pokémon.
Mygod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you intend “hide for hour” to apply only to Pokémon, we should scope it to Pokémon; if you intend it to work for all hidden entities, we should add an automatic refresh/expiry so the in-memory Set drops expired IDs.
To continue this session, run codex resume 019b2474-8894-73f2-a399-a4a5be980d90
fix: clean outdated storage 15s after map loads
|
@Mygod Renamed things. It will run cleanup once 15s after map loads. There's no need to run this in the background loop at all (but so wasn't on add). Tested. |
Mygod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That commit only partially addresses it.
- It does clean expired entries once after load and updates the in-memory Set (src/pages/map/components/Container.jsx:42 + src/utils/hiddenEntities.js:77), so stale hides from a previous session can drop ~15s after you open the map.
- It still does not make hides expire during a long-running session: after an hour passes, nothing re-runs cleanup, so useMemory().hideList stays stale until reload.
- It’s actually worse than before on “another hide refreshes it”: addHiddenEntity no longer calls cleanOldEntries, so hiding something later won’t drop expired IDs either (src/utils/hiddenEntities.js:54).
If “hidden for an hour” is still the intended behavior, you’ll need either cleanup-on-add back, or a one-shot setTimeout scheduled for the next expiry (not a loop) to refresh hideList when entries age out.
|
As a human, I personally think hiding for exactly one hour is too arbitrary and the use case is too niche. |
The original idea was for Pokemons, since map tends to refresh in background. Guess I can just bring back those and drop the rest |
5f95179 to
da54f9a
Compare
|
Could things be hidden till midnight? Iirc I already had few requests for such feature but for quests mostly so I personally think 1h is not enough |
Persistent "Hide" feature.
Hides everything for 1h can be restored via Settings > Clean Hidden.
Tested, working.