Conversation
…k detail catalog for MVP. Ship landmark pins and preview on the travel map, landmark detail modal with cached catalog data, profile history/visited/help/privacy flows, and map-country-detail navigation polish. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThis PR adds profile help, privacy, edit, history, and visited screens; expands recently viewed and discovery tracking; introduces landmark detail enrichment and static catalog tooling; and adds travel-map landmark pins, legend filtering, preview cards, and landmark-focused map or country-detail navigation. ChangesProfile surfaces
Landmark detail and travel map
Sequence Diagram(s)sequenceDiagram
participant Profile as Profile
participant Map as Travel Map
participant Preview as Landmark Preview
participant Detail as Country Detail
Profile->>Map: openTravelMap()
Map->>Preview: show travel landmark preview
Preview->>Detail: open landmark country detail
Detail->>Map: return to map session
Estimated code review effort🎯 5 (Critical) | ⏱️ ~105 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7f25aa3. Configure here.
| useRecentlyViewedStore.getState().recordLandmarkView(item); | ||
| useCountryDetailFocusStore.getState().setFocusLandmarkId(item.landmark.id); | ||
| openCountryDetail(item.country, { from }); | ||
| } |
There was a problem hiding this comment.
Travel map return flag cleared
High Severity
Tapping "View Landmark" from the map preview sets countryDetailReturnToMap to true, but openLandmarkCountryDetail then implicitly resets it to false. This happens because openCountryDetail is called without the returnToMap option, overriding the flag. As a result, navigating back from country detail doesn't return to the map or restore the landmark preview.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7f25aa3. Configure here.
| void Haptics.impactAsync(Haptics.ImpactFeedbackStyle.Light); | ||
| onClose(); | ||
| openLandmarkOnMap(landmark, country); | ||
| }; |
There was a problem hiding this comment.
Map action closes modal first
Medium Severity
handleViewOnMap always calls onClose() before openLandmarkOnMap. When the landmark has no valid coordinates, openLandmarkOnMap returns false and navigation never happens, but the detail sheet is already dismissed, so the user loses context with no feedback.
Reviewed by Cursor Bugbot for commit 7f25aa3. Configure here.
| if (focusedLandmark) { | ||
| setSelectedLandmark(focusedLandmark); | ||
| } | ||
| }, [focusLandmarkId, landmarkKey, landmarks]); |
There was a problem hiding this comment.
Focus modal reopens after dismiss
Medium Severity
A useEffect sets selectedLandmark whenever focusLandmarkId matches and landmarkKey changes, without checking whether the user closed LandmarkDetailModal. After dismiss, a landmarks refresh or reorder updates landmarkKey and forces the modal open again while focus is still active.
Reviewed by Cursor Bugbot for commit 7f25aa3. Configure here.
| const hasMore = landmarks.length > collapsedCount; | ||
| const visibleLandmarks = expanded | ||
| ? landmarks | ||
| : landmarks.slice(0, collapsedCount); |
There was a problem hiding this comment.
Grid focus stays collapsed hidden
Medium Severity
shouldExpandForFocus only returns true when focusIndex > 0, not when the focused landmark falls outside the grid’s collapsed window (four cards). A focused landmark at index four or higher can stay out of visibleLandmarks while scroll and highlight logic still run, so deep-linked landmarks may not appear in the section.
Reviewed by Cursor Bugbot for commit 7f25aa3. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/profile/profile-settings-row.tsx (1)
96-107:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
disableddoes not disable the toggle control.When
showToggleanddisabledare both true, the row is non-pressable but theSwitchis still interactive. Please passdisabledintoSwitchand guardonValueChangeto keep disabled semantics consistent.Suggested fix
{showToggle ? ( <Switch value={toggleValue} - onValueChange={onToggle} + onValueChange={disabled ? undefined : onToggle} + disabled={disabled} trackColor={{ false: "rgba(255,255,255,0.18)", true: PROFILE_SWITCH_TRACK_ON, }}Also applies to: 118-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/profile/profile-settings-row.tsx` around lines 96 - 107, The Switch component in the showToggle conditional block is missing the disabled prop, making it interactive even when the row is disabled. Add the disabled prop to the Switch component and modify the onValueChange handler to guard against executing onToggle when disabled is true, ensuring consistent disabled semantics between the row and the toggle control.
🧹 Nitpick comments (17)
hooks/use-recent-history-list.ts (1)
9-15: 💤 Low valueConsider case-insensitive country name matching for robustness.
The
finduses strict equality (===) on country names. If country name casing ever varies between the feed and history stores, matches will fail silently.🛡️ Optional: Make comparison case-insensitive
function mergeCountryFromFeed( country: Country, feedCountries: Country[], ): Country { - const match = feedCountries.find((c) => c.name === country.name); + const match = feedCountries.find( + (c) => c.name.toLowerCase() === country.name.toLowerCase() + ); return match ?? country; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hooks/use-recent-history-list.ts` around lines 9 - 15, The mergeCountryFromFeed function uses a strict case-sensitive comparison when finding matching countries by name in the feedCountries array. To make the matching more robust and handle variations in casing, modify the find method to compare country names in a case-insensitive manner by converting both c.name and country.name to lowercase before performing the equality check.types/history.ts (1)
18-23: 💤 Low valueConsider trimming landmark ID for consistency.
Line 20 trims the country name, but line 22 doesn't trim the landmark ID. While landmark IDs are trimmed during store normalization, defensive trimming here would ensure consistency and guard against edge cases.
♻️ Add defensive trim
export function historyEntryKey(entry: HistoryEntry): string { if (entry.kind === "country") { return `country:${entry.country.name.trim()}`; } - return `landmark:${entry.item.landmark.id}`; + return `landmark:${entry.item.landmark.id.trim()}`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@types/history.ts` around lines 18 - 23, In the historyEntryKey function, there is an inconsistency where the country name is trimmed with .trim() but the landmark ID is not trimmed in the corresponding return statement. Add defensive trimming by calling .trim() on entry.item.landmark.id in the return statement to match the trimming behavior applied to the country name and ensure consistency across both branches of the conditional logic.store/use-recently-viewed-store.ts (1)
243-253: ⚡ Quick winImprove type safety in the migration function.
Line 244 casts
persistedStatetoRecentlyViewedState, which expectsentries: HistoryEntry[]. However, whenversion < RECENTLY_VIEWED_STORAGE_VERSION, the persisted entries are actuallyLegacyRecentlyViewedEntry[]. The subsequent cast on line 246 works around this, but the intermediate type is incorrect.♻️ More type-safe migration
migrate: (persistedState, version) => { - const state = persistedState as RecentlyViewedState; + const state = persistedState as RecentlyViewedState & { + entries: LegacyRecentlyViewedEntry[] | HistoryEntry[]; + }; if (version < RECENTLY_VIEWED_STORAGE_VERSION) { const legacy = state.entries as LegacyRecentlyViewedEntry[]; return {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store/use-recently-viewed-store.ts` around lines 243 - 253, The migrate function in use-recently-viewed-store.ts has a type safety issue where persistedState is immediately cast to RecentlyViewedState (which expects entries to be HistoryEntry[]), but when version is less than RECENTLY_VIEWED_STORAGE_VERSION, the entries are actually LegacyRecentlyViewedEntry[]. Instead of casting persistedState directly to RecentlyViewedState at the beginning, use a more accurate intermediate type that represents the actual shape of legacy persisted state, then perform the appropriate type narrowing based on the version check before calling migrateLegacyEntries. This eliminates the need for the workaround cast to LegacyRecentlyViewedEntry[] and makes the type system accurately reflect the data transformations happening in the migration flow.constants/support.ts (1)
2-2: Consider using a dedicated business email instead of a personal Gmail address for production support.The email
birthrand@gmail.comis consistently used across the codebase in legal documents and the Help center UI. While this appears intentional, using a personal email for production support contact is not aligned with best practices—consider using a dedicated support email address (e.g., support@domain.com) instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@constants/support.ts` at line 2, Replace the personal Gmail address in the SUPPORT_EMAIL constant with a dedicated business email address (such as support@yourdomain.com). This ensures that production support contact information follows best practices by using a professional business email rather than a personal email account. Update the SUPPORT_EMAIL constant value throughout the codebase where it is referenced.components/saved/saved-countries-list.tsx (2)
177-184: ⚡ Quick winDuplicated conditional logic renders
SavedCountryCardMenutwice whenrenderCardMenuis not provided.When
renderCardMenuis undefined:
- Line 130 creates
<SavedCountryCardMenu country={country} />and assigns it to the unusedcardMenuvariable- Line 180-183 creates a second instance of
<SavedCountryCardMenu country={country} style={styles.gridMenuTrigger} />This wastes render cycles and diverges from the list row pattern (lines 201-205, 253), which correctly defines
cardMenuonce and uses it.See the refactor suggestion in the previous comment (lines 127-131).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/saved/saved-countries-list.tsx` around lines 177 - 184, The SavedCountryCardMenu component is being instantiated twice when renderCardMenu is undefined: once at line 130 where it is assigned to the cardMenu variable, and again in the else clause at lines 180-183 of the conditional block. Remove the conditional rendering logic that creates a new SavedCountryCardMenu instance in the else clause and instead use the existing cardMenu variable consistently. Ensure that when renderCardMenu is not provided, the cardMenu variable (which should already be properly created with the gridMenuTrigger style applied) is used directly, following the same pattern as the list row rendering (see lines 201-205 and 253).
127-131: ⚡ Quick winDead code:
cardMenuvariable is defined but never used.The
cardMenuvariable is computed but not referenced in the grid card's JSX. Instead, lines 177-184 duplicate the conditional menu rendering logic.♻️ Suggested refactor to remove duplication and dead code
Remove the unused
cardMenuvariable and fix the menu rendering at lines 177-184 to match the pattern used inSavedCountryListRow(which correctly defines and usescardMenu):}) { const heroUri = getCountryCardHeroUri(country); - const cardMenu = renderCardMenu ? ( - renderCardMenu(country) - ) : ( - <SavedCountryCardMenu country={country} /> - ); return ( <View style={[styles.card, { width }]}> <Pressable accessibilityRole="button" accessibilityLabel={`Open ${country.name}`} onPressIn={() => warmCountryDetail(country)} onPress={() => openCountryDetail(country, { from: detailOrigin })} style={({ pressed }) => [ styles.cardPressable, pressed && styles.cardPressed, ]} > {/* ... */} </Pressable> {renderCardMenu ? ( - <View style={styles.gridMenuTrigger}>{cardMenu}</View> + <View style={styles.gridMenuTrigger}>{renderCardMenu(country)}</View> ) : ( <SavedCountryCardMenu country={country} style={styles.gridMenuTrigger} /> )} </View> ); }Alternatively, keep the
cardMenuvariable and use it consistently:}) { const heroUri = getCountryCardHeroUri(country); const cardMenu = renderCardMenu ? ( renderCardMenu(country) ) : ( <SavedCountryCardMenu country={country} /> ); return ( <View style={[styles.card, { width }]}> <Pressable {/* ... */} > {/* ... */} </Pressable> - {renderCardMenu ? ( - <View style={styles.gridMenuTrigger}>{cardMenu}</View> - ) : ( - <SavedCountryCardMenu - country={country} - style={styles.gridMenuTrigger} - /> - )} + <View style={styles.gridMenuTrigger}>{cardMenu}</View> </View> ); }Note: The second approach requires
SavedCountryCardMenuto acceptstyleas a prop and apply it, or the styling needs adjustment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/saved/saved-countries-list.tsx` around lines 127 - 131, The cardMenu variable defined at lines 127-131 is computed but never used in the JSX, creating dead code. Remove this unused variable definition and instead consolidate the menu rendering logic by applying the same conditional pattern (checking renderCardMenu and falling back to SavedCountryCardMenu component) directly at lines 177-184 where the menu is actually rendered, eliminating the duplication.components/history/history-landmark-card-menu.tsx (1)
38-67: ⚡ Quick winConsider extracting the MenuRow component to reduce duplication.
The
MenuRowsubcomponent is duplicated acrosshistory-country-card-menu.tsx,history-landmark-card-menu.tsx, andvisited-country-card-menu.tsx. Consider extracting it to a shared module (e.g.,components/shared/menu-row.tsx) to follow DRY principles and simplify maintenance.Also applies to: 38-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/history/history-landmark-card-menu.tsx` around lines 38 - 67, The MenuRow component in history-landmark-card-menu.tsx is duplicated across multiple files (history-country-card-menu.tsx, visited-country-card-menu.tsx). Extract the MenuRow function and MenuRowProps type definition from history-landmark-card-menu.tsx into a new shared module at components/shared/menu-row.tsx, then update all three files to import MenuRow and MenuRowProps from this shared module instead of maintaining duplicate definitions. This will eliminate code duplication and make future maintenance easier.lib/open-country-on-map.ts (1)
103-118: ⚡ Quick winSimplify the travel session preservation logic.
Lines 112-114 contain a confusing double-negative:
if (!identity.travelMapSessionActive) { identity.setTravelMapSessionActive(false); }This sets the travel session to
falseonly when it's alreadyfalse, which is redundant. The comment indicates the intent is to preserve an active travel session. While functionally correct (it achieves preservation by doing nothing when active), the backwards condition makes this hard to read.♻️ Simplify the logic
} else if (source === "countryDetail") { identity.setExploreMapSessionActive(false); - // Preserve travel map when detouring country detail → map → country detail. - if (!identity.travelMapSessionActive) { - identity.setTravelMapSessionActive(false); - } + // Preserve travel session when detouring country detail → map → country detail. + // No action needed - travel session state is preserved as-is. }Alternatively, if there's a side-effect reason to call the setter:
- if (!identity.travelMapSessionActive) { - identity.setTravelMapSessionActive(false); - } + // Explicitly maintain travel session state (preserves active session, ensures inactive is cleared) + identity.setTravelMapSessionActive(identity.travelMapSessionActive);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/open-country-on-map.ts` around lines 103 - 118, The "countryDetail" branch contains a confusing double-negative condition that checks if travel map session is inactive, then redundantly sets it to false. To fix this, in the else if branch where source === "countryDetail", remove the entire if-block (lines 112-114) that contains the condition checking !identity.travelMapSessionActive and the setTravelMapSessionActive call. Since the goal is to preserve the travel map session by not modifying it, this conditional logic is unnecessary and removing it will make the intent clearer while achieving the same preservation behavior.backend/src/services/cache.service.ts (1)
71-72: 💤 Low valueClarify cache key comment.
The comment states "v8 — includes landmark city" but the cache key itself is still
landmarks:v8:${cca2}without a city parameter. This suggests the city is stored in the cached value structure (not the key), but the comment is ambiguous. Consider revising to: "v8 — cached landmark data now includes city field (Wikidata P131 / OSM addr tags)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/services/cache.service.ts` around lines 71 - 72, The comment for the landmarks cache key function is ambiguous because it states "includes landmark city" when the city information is actually part of the cached value structure, not the cache key itself. Locate the landmarks function (around line 71-72) and revise its comment to clarify that the v8 version stores landmark data with an additional city field in the cached value. Change the comment to something like "v8 — cached landmark data now includes city field (Wikidata P131 / OSM addr tags)" to make it clear that the city data is in the cached value structure, not in the key generation logic.lib/format-country.ts (2)
107-277: 💤 Low valueStatic analysis: ReDoS warnings in city inference patterns.
Similar to the description-thin patterns, the Wikipedia city extraction regexes use the variable
WIKI_CITY_NAMEpattern and are flagged for ReDoS risk.Assessment: The risk remains low because:
- Input is sentence-bounded via
splitWikiSentences- The
WIKI_CITY_NAMEpattern uses non-greedy*?- Patterns have word boundaries and specific terminators
This is inherent to natural language parsing. Consider it acceptable for best-effort city inference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/format-country.ts` around lines 107 - 277, The static analysis tools are flagging ReDoS warnings in the regex patterns used throughout the city inference functions, specifically in inferCityFromWikiText and the WIKI_CITY_NAME pattern usage. While the reviewer has assessed this risk as acceptable due to sentence-bounded input via splitWikiSentences, non-greedy matching with *?, and specific terminators, add explanatory comments above the WIKI_CITY_NAME constant and in the inferCityFromWikiText function to document why these ReDoS warnings are acceptable in this context, referencing that input is sentence-limited and patterns use non-greedy quantifiers with word boundaries.Source: Linters/SAST tools
27-68: 💤 Low valueStatic analysis: ReDoS warnings in regex patterns.
The static analysis tool flags regex patterns constructed from variables (landmark name/type) as potential ReDoS risks. While the
escapeRegExphelper prevents injection of special regex characters, the patterns themselves contain quantifiers like.+that could theoretically be slow on pathological input.Assessment: The practical risk is low because:
- Patterns are anchored with
^and$- Input strings (landmark descriptions) are typically short
- The use case (detecting placeholder text) is non-critical
No changes required, but be aware of this limitation if descriptions become unexpectedly long or if performance issues arise.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/format-country.ts` around lines 27 - 68, The review comment identifies potential ReDoS (Regular Expression Denial of Service) risks in the regex patterns within the isLandmarkDescriptionThin function due to quantifiers like .+ in dynamically constructed patterns. While the comment notes that practical risk is low due to anchoring, short input lengths, and non-critical use case, and states that no changes are currently required, be aware that if landmark descriptions become unexpectedly long or performance issues arise, the regex patterns could be refactored to use simpler non-greedy patterns, stricter input validation on description length, or alternative string matching approaches instead of complex regex operations.Source: Linters/SAST tools
types/landmark-details-catalog.ts (1)
4-4: 💤 Low valueConsider removing the type alias.
LandmarkDetailAiContentis just an alias forLandmarkAiContent. If the types are identical, import and useLandmarkAiContentdirectly throughout the codebase to reduce indirection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@types/landmark-details-catalog.ts` at line 4, Remove the unnecessary type alias LandmarkDetailAiContent from the types/landmark-details-catalog.ts file by deleting the export statement. Then search the entire codebase for all references to LandmarkDetailAiContent and replace them with LandmarkAiContent directly. Update any imports that are importing LandmarkDetailAiContent to import LandmarkAiContent instead, or remove the import entirely if it becomes unnecessary.components/ai-explorer/country-facts-carousel.tsx (1)
83-83: 💤 Low valueConsider removing index from the key.
Using
indexinkeyExtractorcan cause reconciliation issues if the facts array changes dynamically. Since facts are probably static per country, this works in practice, but a more robust approach would be:keyExtractor={(fact) => fact.slice(0, 50)}or use a stable identifier if available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/ai-explorer/country-facts-carousel.tsx` at line 83, In the keyExtractor property of the carousel component, remove the index parameter from the key generation logic. Instead of combining both index and fact content, use only the fact content itself (for example, fact.slice(0, 50)) to create a stable, unique key that will work correctly if the facts array order or content changes dynamically.lib/static-landmark-details.ts (1)
50-52: 💤 Low valueSimplify redundant check.
The check
Boolean(getStaticLandmarkAiById(landmarkId)?.fact?.trim())is redundant becausegetStaticLandmarkAiByIdalready returnsnullif the fact is not trimmed (line 43). Simplify to:♻️ Proposed simplification
export function isStaticLandmarkDetailEnriched(landmarkId: string): boolean { - return Boolean(getStaticLandmarkAiById(landmarkId)?.fact?.trim()); + return Boolean(getStaticLandmarkAiById(landmarkId)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/static-landmark-details.ts` around lines 50 - 52, The isStaticLandmarkDetailEnriched function contains a redundant Boolean() wrapper around a check that already handles null cases through optional chaining. Since getStaticLandmarkAiById already returns null when appropriate (as noted in line 43), you can simplify the function body by removing the Boolean() wrapper and instead use the double negation operator (!!) or rely directly on the optional chaining result to implicitly convert to a boolean value. Replace the current return statement to directly return the boolean result of the optional chaining chain without the explicit Boolean conversion.hooks/use-travel-map-pins.ts (1)
26-36: ⚡ Quick winReconsider the memoization pattern.
The
useMemodependency array includes all store-derived values, butgetTravelMapPinData()doesn't accept parameters—it reads directly from stores via.getState(). This means the memoization won't prevent recalculations when stores change, because the function always reads fresh state regardless of the dependencies.♻️ Consider one of these approaches
Option 1 (preferred): Pass store values as parameters
export function useTravelMapPinData(): TravelMapPinData { const feedCountries = useCountryFeedStore((s) => s.countries); const savedCountries = useSavedCountriesStore((s) => s.savedCountries); const savedLandmarks = useSavedLandmarksStore((s) => s.savedLandmarks); const historyEntries = useRecentlyViewedStore((s) => s.entries); const visitedCountryIds = useDiscoveryProgressStore( (s) => s.visitedCountryIds, ); const visitedCountryById = useDiscoveryProgressStore( (s) => s.visitedCountryById, ); return useMemo( - () => getTravelMapPinData(), + () => + getTravelMapPinData({ + feedCountries, + savedCountries, + savedLandmarks, + historyEntries, + visitedCountryIds, + visitedCountryById, + }), [ feedCountries, savedCountries, savedLandmarks, historyEntries, visitedCountryIds, visitedCountryById, ], ); }Then update
getTravelMapPinDatainlib/travel-map-pins.tsto accept these parameters instead of reading stores directly.Option 2: Remove useMemo (simpler but less explicit)
export function useTravelMapPinData(): TravelMapPinData { - const feedCountries = useCountryFeedStore((s) => s.countries); - const savedCountries = useSavedCountriesStore((s) => s.savedCountries); - const savedLandmarks = useSavedLandmarksStore((s) => s.savedLandmarks); - const historyEntries = useRecentlyViewedStore((s) => s.entries); - const visitedCountryIds = useDiscoveryProgressStore( - (s) => s.visitedCountryIds, - ); - const visitedCountryById = useDiscoveryProgressStore( - (s) => s.visitedCountryById, - ); - - return useMemo( - () => getTravelMapPinData(), - [ - feedCountries, - savedCountries, - savedLandmarks, - historyEntries, - visitedCountryIds, - visitedCountryById, - ], - ); + return getTravelMapPinData(); }Since
getTravelMapPinData()already reads fresh store state, the memoization adds complexity without benefit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hooks/use-travel-map-pins.ts` around lines 26 - 36, The memoization is ineffective because getTravelMapPinData() doesn't accept parameters and reads directly from stores via .getState(), so the dependency array changes won't prevent recalculations. To fix this, modify getTravelMapPinData in lib/travel-map-pins.ts to accept the store-derived values (feedCountries, savedCountries, savedLandmarks, historyEntries, visitedCountryIds, visitedCountryById) as function parameters instead of reading them directly from stores. Then update the useMemo call in use-travel-map-pins.ts to pass these values as arguments to getTravelMapPinData() so the memoization properly prevents unnecessary recalculations when the dependencies haven't changed.lib/travel-map-pins.ts (1)
328-370: ⚖️ Poor tradeoffReconstructed landmark feed items have minimal data.
When
resolveTravelMapLandmarkFeedItemcannot find a saved or recently viewed match, it reconstructs aPlaceFeedItemfrom the pin with sparse landmark data:imageUrl: null, emptydescription, and generic"Landmark"type. Downstream UI components (preview cards, modals, or tooltips) may expect complete landmark data, which could result in missing images, empty descriptions, or fallback rendering.Consider one of these approaches:
- Document the contract: Add a TypeScript comment warning callers that the returned
PlaceFeedItemmay have incomplete landmark data when reconstructed from a pin.- Enrich on demand: If the pin came from a source that should have full data (e.g., static catalog), look up and merge complete landmark details before returning.
- Return a discriminated union: Return
PlaceFeedItem | MinimalPlaceFeedItemso TypeScript enforces handling of sparse data at call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/travel-map-pins.ts` around lines 328 - 370, The resolveTravelMapLandmarkFeedItem function reconstructs a PlaceFeedItem with incomplete landmark data when no saved or recently viewed match is found, setting imageUrl to null, description to empty string, and type to generic "Landmark". Add a TypeScript JSDoc comment above the function return statement documenting that the reconstructed landmark data may be incomplete when returned from this fallback path, or alternatively enrich the landmark object by looking up complete details from a data source before returning it so downstream UI components receive complete landmark information instead of sparse fallback data.backend/src/utils/landmark-ranking.ts (1)
5-16: 💤 Low valueInconsistent handling of negative years (BCE dates).
The regex on line 9 matches a leading
+or-sign ([+-]?), but line 13 rejects anyyear <= 0. This means:
- Input
"-500"matches, parses to-500, then returnsnull- The negative sign matching is dead code
If BCE dates should be rejected, simplify the regex to not match the minus sign. If BCE dates should be supported, adjust the validation.
Proposed fix (assuming BCE dates are not supported)
- const signedMatch = trimmed.match(/^([+-]?\d{1,4})/); + const signedMatch = trimmed.match(/^(\d{1,4})/); if (!signedMatch) return null; const year = Number.parseInt(signedMatch[1], 10); - if (!Number.isFinite(year) || year <= 0 || year > 9999) return null; + if (!Number.isFinite(year) || year < 1 || year > 9999) return null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/utils/landmark-ranking.ts` around lines 5 - 16, The regex pattern in the parseLandmarkYearBuilt function matches an optional plus or minus sign with [+-]?, but the validation check immediately rejects any year value that is less than or equal to zero. Remove the [+-]? from the regex pattern on line 9 so it only matches digits without a leading sign, making the pattern /^(\d{1,4})/ instead. This will eliminate the dead code that attempts to match negative signs when they are not actually supported by the validation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/`(tabs)/profile/history.tsx:
- Around line 54-58: The loadInitialFeed function is async but its promise
rejection is not being handled in the useEffect hook. Add error handling to the
loadInitialFeed call by either attaching a .catch() method to handle potential
promise rejections, or by wrapping the call in an async IIFE with a try-catch
block. This ensures that network errors, API failures, or other async errors
during feed loading are properly caught and handled rather than causing
unhandled promise rejections that could crash the app or leave the history
screen in a broken state.
In `@components/ai-explorer/country-landmarks-section.tsx`:
- Around line 577-586: The `shouldExpandForFocus` function currently returns
true when `focusIndex > 0`, but this doesn't account for landmarks already
visible in the collapsed grid view. Since GRID_COLLAPSED_COUNT is 4 (meaning
landmarks at indices 0-3 are visible when collapsed), focusing on indices 1-3
should not trigger expansion. Change the condition from `focusIndex > 0` to
`focusIndex >= collapsedCount` where collapsedCount represents the number of
visible landmarks in the collapsed state. You'll need to either pass
`collapsedCount` as a parameter to the `shouldExpandForFocus` function or
determine it from the layout/display state, then update all calls to this
function to provide the collapsedCount argument.
In `@components/explore/swipe-dismiss-sheet.tsx`:
- Around line 161-164: The dismiss duration calculation in the duration variable
assignment duplicates the formula already defined in the dismissDurationMs
helper function. Replace the entire duration calculation (lines 161-164) with a
direct call to the dismissDurationMs function, passing the distance and
dismissExitY parameters. This eliminates code duplication and ensures that any
future updates to the dismiss duration formula only need to be made in one
location.
In `@components/profile/help-center-contact-row.tsx`:
- Around line 15-19: The `handleCopyEmail` function lacks error handling for the
`Clipboard.setStringAsync` operation on the SUPPORT_EMAIL. Wrap the await call
to `Clipboard.setStringAsync` in a try-catch block to properly handle potential
failures from clipboard operations (such as permission issues or
platform-specific errors). In the catch block, log or alert the user about the
error instead of allowing the promise rejection to go unhandled.
---
Outside diff comments:
In `@components/profile/profile-settings-row.tsx`:
- Around line 96-107: The Switch component in the showToggle conditional block
is missing the disabled prop, making it interactive even when the row is
disabled. Add the disabled prop to the Switch component and modify the
onValueChange handler to guard against executing onToggle when disabled is true,
ensuring consistent disabled semantics between the row and the toggle control.
---
Nitpick comments:
In `@backend/src/services/cache.service.ts`:
- Around line 71-72: The comment for the landmarks cache key function is
ambiguous because it states "includes landmark city" when the city information
is actually part of the cached value structure, not the cache key itself. Locate
the landmarks function (around line 71-72) and revise its comment to clarify
that the v8 version stores landmark data with an additional city field in the
cached value. Change the comment to something like "v8 — cached landmark data
now includes city field (Wikidata P131 / OSM addr tags)" to make it clear that
the city data is in the cached value structure, not in the key generation logic.
In `@backend/src/utils/landmark-ranking.ts`:
- Around line 5-16: The regex pattern in the parseLandmarkYearBuilt function
matches an optional plus or minus sign with [+-]?, but the validation check
immediately rejects any year value that is less than or equal to zero. Remove
the [+-]? from the regex pattern on line 9 so it only matches digits without a
leading sign, making the pattern /^(\d{1,4})/ instead. This will eliminate the
dead code that attempts to match negative signs when they are not actually
supported by the validation logic.
In `@components/ai-explorer/country-facts-carousel.tsx`:
- Line 83: In the keyExtractor property of the carousel component, remove the
index parameter from the key generation logic. Instead of combining both index
and fact content, use only the fact content itself (for example, fact.slice(0,
50)) to create a stable, unique key that will work correctly if the facts array
order or content changes dynamically.
In `@components/history/history-landmark-card-menu.tsx`:
- Around line 38-67: The MenuRow component in history-landmark-card-menu.tsx is
duplicated across multiple files (history-country-card-menu.tsx,
visited-country-card-menu.tsx). Extract the MenuRow function and MenuRowProps
type definition from history-landmark-card-menu.tsx into a new shared module at
components/shared/menu-row.tsx, then update all three files to import MenuRow
and MenuRowProps from this shared module instead of maintaining duplicate
definitions. This will eliminate code duplication and make future maintenance
easier.
In `@components/saved/saved-countries-list.tsx`:
- Around line 177-184: The SavedCountryCardMenu component is being instantiated
twice when renderCardMenu is undefined: once at line 130 where it is assigned to
the cardMenu variable, and again in the else clause at lines 180-183 of the
conditional block. Remove the conditional rendering logic that creates a new
SavedCountryCardMenu instance in the else clause and instead use the existing
cardMenu variable consistently. Ensure that when renderCardMenu is not provided,
the cardMenu variable (which should already be properly created with the
gridMenuTrigger style applied) is used directly, following the same pattern as
the list row rendering (see lines 201-205 and 253).
- Around line 127-131: The cardMenu variable defined at lines 127-131 is
computed but never used in the JSX, creating dead code. Remove this unused
variable definition and instead consolidate the menu rendering logic by applying
the same conditional pattern (checking renderCardMenu and falling back to
SavedCountryCardMenu component) directly at lines 177-184 where the menu is
actually rendered, eliminating the duplication.
In `@constants/support.ts`:
- Line 2: Replace the personal Gmail address in the SUPPORT_EMAIL constant with
a dedicated business email address (such as support@yourdomain.com). This
ensures that production support contact information follows best practices by
using a professional business email rather than a personal email account. Update
the SUPPORT_EMAIL constant value throughout the codebase where it is referenced.
In `@hooks/use-recent-history-list.ts`:
- Around line 9-15: The mergeCountryFromFeed function uses a strict
case-sensitive comparison when finding matching countries by name in the
feedCountries array. To make the matching more robust and handle variations in
casing, modify the find method to compare country names in a case-insensitive
manner by converting both c.name and country.name to lowercase before performing
the equality check.
In `@hooks/use-travel-map-pins.ts`:
- Around line 26-36: The memoization is ineffective because
getTravelMapPinData() doesn't accept parameters and reads directly from stores
via .getState(), so the dependency array changes won't prevent recalculations.
To fix this, modify getTravelMapPinData in lib/travel-map-pins.ts to accept the
store-derived values (feedCountries, savedCountries, savedLandmarks,
historyEntries, visitedCountryIds, visitedCountryById) as function parameters
instead of reading them directly from stores. Then update the useMemo call in
use-travel-map-pins.ts to pass these values as arguments to
getTravelMapPinData() so the memoization properly prevents unnecessary
recalculations when the dependencies haven't changed.
In `@lib/format-country.ts`:
- Around line 107-277: The static analysis tools are flagging ReDoS warnings in
the regex patterns used throughout the city inference functions, specifically in
inferCityFromWikiText and the WIKI_CITY_NAME pattern usage. While the reviewer
has assessed this risk as acceptable due to sentence-bounded input via
splitWikiSentences, non-greedy matching with *?, and specific terminators, add
explanatory comments above the WIKI_CITY_NAME constant and in the
inferCityFromWikiText function to document why these ReDoS warnings are
acceptable in this context, referencing that input is sentence-limited and
patterns use non-greedy quantifiers with word boundaries.
- Around line 27-68: The review comment identifies potential ReDoS (Regular
Expression Denial of Service) risks in the regex patterns within the
isLandmarkDescriptionThin function due to quantifiers like .+ in dynamically
constructed patterns. While the comment notes that practical risk is low due to
anchoring, short input lengths, and non-critical use case, and states that no
changes are currently required, be aware that if landmark descriptions become
unexpectedly long or performance issues arise, the regex patterns could be
refactored to use simpler non-greedy patterns, stricter input validation on
description length, or alternative string matching approaches instead of complex
regex operations.
In `@lib/open-country-on-map.ts`:
- Around line 103-118: The "countryDetail" branch contains a confusing
double-negative condition that checks if travel map session is inactive, then
redundantly sets it to false. To fix this, in the else if branch where source
=== "countryDetail", remove the entire if-block (lines 112-114) that contains
the condition checking !identity.travelMapSessionActive and the
setTravelMapSessionActive call. Since the goal is to preserve the travel map
session by not modifying it, this conditional logic is unnecessary and removing
it will make the intent clearer while achieving the same preservation behavior.
In `@lib/static-landmark-details.ts`:
- Around line 50-52: The isStaticLandmarkDetailEnriched function contains a
redundant Boolean() wrapper around a check that already handles null cases
through optional chaining. Since getStaticLandmarkAiById already returns null
when appropriate (as noted in line 43), you can simplify the function body by
removing the Boolean() wrapper and instead use the double negation operator (!!)
or rely directly on the optional chaining result to implicitly convert to a
boolean value. Replace the current return statement to directly return the
boolean result of the optional chaining chain without the explicit Boolean
conversion.
In `@lib/travel-map-pins.ts`:
- Around line 328-370: The resolveTravelMapLandmarkFeedItem function
reconstructs a PlaceFeedItem with incomplete landmark data when no saved or
recently viewed match is found, setting imageUrl to null, description to empty
string, and type to generic "Landmark". Add a TypeScript JSDoc comment above the
function return statement documenting that the reconstructed landmark data may
be incomplete when returned from this fallback path, or alternatively enrich the
landmark object by looking up complete details from a data source before
returning it so downstream UI components receive complete landmark information
instead of sparse fallback data.
In `@store/use-recently-viewed-store.ts`:
- Around line 243-253: The migrate function in use-recently-viewed-store.ts has
a type safety issue where persistedState is immediately cast to
RecentlyViewedState (which expects entries to be HistoryEntry[]), but when
version is less than RECENTLY_VIEWED_STORAGE_VERSION, the entries are actually
LegacyRecentlyViewedEntry[]. Instead of casting persistedState directly to
RecentlyViewedState at the beginning, use a more accurate intermediate type that
represents the actual shape of legacy persisted state, then perform the
appropriate type narrowing based on the version check before calling
migrateLegacyEntries. This eliminates the need for the workaround cast to
LegacyRecentlyViewedEntry[] and makes the type system accurately reflect the
data transformations happening in the migration flow.
In `@types/history.ts`:
- Around line 18-23: In the historyEntryKey function, there is an inconsistency
where the country name is trimmed with .trim() but the landmark ID is not
trimmed in the corresponding return statement. Add defensive trimming by calling
.trim() on entry.item.landmark.id in the return statement to match the trimming
behavior applied to the country name and ensure consistency across both branches
of the conditional logic.
In `@types/landmark-details-catalog.ts`:
- Line 4: Remove the unnecessary type alias LandmarkDetailAiContent from the
types/landmark-details-catalog.ts file by deleting the export statement. Then
search the entire codebase for all references to LandmarkDetailAiContent and
replace them with LandmarkAiContent directly. Update any imports that are
importing LandmarkDetailAiContent to import LandmarkAiContent instead, or remove
the import entirely if it becomes unnecessary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 26f35fba-91ba-4d43-9c82-5722a327fe76
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (119)
app/(tabs)/_layout.tsxapp/(tabs)/map.tsxapp/(tabs)/profile/_layout.tsxapp/(tabs)/profile/edit-profile.tsxapp/(tabs)/profile/help-center-contact.tsxapp/(tabs)/profile/help-center-faqs.tsxapp/(tabs)/profile/help-center-guides.tsxapp/(tabs)/profile/help-center.tsxapp/(tabs)/profile/history.tsxapp/(tabs)/profile/index.tsxapp/(tabs)/profile/privacy-data-and-cookies.tsxapp/(tabs)/profile/privacy-policy.tsxapp/(tabs)/profile/privacy-terms-of-service.tsxapp/(tabs)/profile/privacy.tsxapp/(tabs)/profile/settings.tsxapp/(tabs)/profile/visited.tsxapp/country/[name]/index.tsxbackend/src/api/country.routes.tsbackend/src/controllers/profile.controller.tsbackend/src/lib/validation.tsbackend/src/services/ai.service.tsbackend/src/services/cache.service.tsbackend/src/services/landmarks.service.tsbackend/src/services/osm.service.tsbackend/src/services/wikidata.service.tsbackend/src/services/wikipedia.service.tsbackend/src/types/landmarks.tsbackend/src/types/wikipedia.tsbackend/src/utils/landmark-ranking.tscomponents/ai-explorer/country-detail-collapsing-header.tsxcomponents/ai-explorer/country-facts-carousel.tsxcomponents/ai-explorer/country-landmarks-section.tsxcomponents/ai-explorer/country-profile-card.tsxcomponents/ai-explorer/landmark-detail-modal.tsxcomponents/ai-explorer/profile-section.tsxcomponents/explore/country-feed-page.tsxcomponents/explore/explore-swipe-card.tsxcomponents/explore/explore-swipe-place-card.tsxcomponents/explore/swipe-dismiss-sheet.tsxcomponents/history/history-country-card-menu.tsxcomponents/history/history-landmark-card-menu.tsxcomponents/history/history-list.tsxcomponents/history/history-space-header.tsxcomponents/map/map-canvas.tsxcomponents/map/map-country-focus-header.tsxcomponents/map/map-country-marker.tsxcomponents/map/map-country-preview-card.tsxcomponents/map/map-landmark-pin.tsxcomponents/map/map-landmark-preview-card.tsxcomponents/map/world-map-view.tsxcomponents/profile/help-center-contact-row.tsxcomponents/profile/help-center-faq-item.tsxcomponents/profile/help-center-guide-item.tsxcomponents/profile/help-center-screen-shell.tsxcomponents/profile/privacy-document-content.tsxcomponents/profile/profile-completion-card.tsxcomponents/profile/profile-edit-field-modal.tsxcomponents/profile/profile-hero-header.tsxcomponents/profile/profile-settings-header.tsxcomponents/profile/profile-settings-row.tsxcomponents/saved/saved-countries-list.tsxcomponents/saved/saved-landmarks-list.tsxcomponents/travel-map/travel-map-legend-badges.tsxcomponents/travel-map/travel-map-legend.tsxcomponents/visited/visited-country-card-menu.tsxcomponents/visited/visited-space-header.tsxconstants/country-detail-layout.tsconstants/legal.tsconstants/map-focus-tiers.tsconstants/static-catalog.tsconstants/support.tsconstants/travel-map-legend.tsdata/help-center-faqs.tsdata/help-center-guides.tsdata/landmark-details.jsondata/legal-documents.tshooks/use-history-profile-row.tshooks/use-map-logic.tshooks/use-profile-stats.tshooks/use-recent-history-list.tshooks/use-travel-map-pins.tshooks/use-visited-countries-list.tslib/api.tslib/format-country.tslib/landmark-image-dimensions.tslib/map-discovery-flight.tslib/map-navigation-intent.tslib/map-presentation.test.tslib/map-presentation.tslib/map-transition-engine.tslib/navigate-back-from-country-detail.tslib/navigate-back-from-map.tslib/open-country-on-map.tslib/open-landmark-country-detail.tslib/open-landmark-on-map.tslib/open-travel-map.tslib/static-landmark-details.tslib/travel-map-pins.tslib/travel-map-session.tspackage.jsonprompts-worldloop/16-landmark-details-static-catalog.mdprompts-worldloop/16a-landmark-details-build-script.mdprompts-worldloop/README.mdprompts/10g-help-center-ui.mdprompts/10h-privacy-terms-ui.mdscripts/build-landmark-details.tsscripts/lib/landmark-details-validation.tsscripts/validate-landmark-details.tsstore/use-country-detail-focus-store.tsstore/use-discovery-progress-store.tsstore/use-identity-store.tsstore/use-map-landmark-focus-store.tsstore/use-map-store.tsstore/use-profile-settings-store.tsstore/use-recently-viewed-store.tsstore/use-travel-map-legend-store.tstypes/history.tstypes/landmark-details-catalog.tstypes/map-presentation.ts
💤 Files with no reviewable changes (1)
- store/use-profile-settings-store.ts
Comment on lines
+54
to
+58
| useEffect(() => { | ||
| if (feedCountries.length === 0 && feedStatus === "idle") { | ||
| void loadInitialFeed(); | ||
| } | ||
| }, [feedCountries.length, feedStatus, loadInitialFeed]); |
There was a problem hiding this comment.
Add error handling for async feed loading.
The loadInitialFeed function is async but the promise is voided on line 56. If the feed loading fails (network errors, API issues), the rejection will be unhandled, potentially causing a crash or silent failure that prevents the history screen from working correctly.
🛡️ Proposed fix with error handling
useEffect(() => {
if (feedCountries.length === 0 && feedStatus === "idle") {
- void loadInitialFeed();
+ loadInitialFeed().catch((error) => {
+ console.error("Failed to load initial feed:", error);
+ });
}
}, [feedCountries.length, feedStatus, loadInitialFeed]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (feedCountries.length === 0 && feedStatus === "idle") { | |
| void loadInitialFeed(); | |
| } | |
| }, [feedCountries.length, feedStatus, loadInitialFeed]); | |
| useEffect(() => { | |
| if (feedCountries.length === 0 && feedStatus === "idle") { | |
| loadInitialFeed().catch((error) => { | |
| console.error("Failed to load initial feed:", error); | |
| }); | |
| } | |
| }, [feedCountries.length, feedStatus, loadInitialFeed]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/`(tabs)/profile/history.tsx around lines 54 - 58, The loadInitialFeed
function is async but its promise rejection is not being handled in the
useEffect hook. Add error handling to the loadInitialFeed call by either
attaching a .catch() method to handle potential promise rejections, or by
wrapping the call in an async IIFE with a try-catch block. This ensures that
network errors, API failures, or other async errors during feed loading are
properly caught and handled rather than causing unhandled promise rejections
that could crash the app or leave the history screen in a broken state.
Comment on lines
+577
to
+586
| function shouldExpandForFocus( | ||
| landmarks: CountryLandmark[], | ||
| focusLandmarkId: string | null | undefined, | ||
| ): boolean { | ||
| if (!focusLandmarkId) return false; | ||
| const focusIndex = landmarks.findIndex( | ||
| (landmark) => landmark.id === focusLandmarkId, | ||
| ); | ||
| return focusIndex > 0; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how shouldExpandForFocus is used and whether layout mode should be considered
rg -nC5 'shouldExpandForFocus'Repository: birthrand/worldloop
Length of output: 3126
🏁 Script executed:
#!/bin/bash
# Find GRID_COLLAPSED_COUNT and LIST_COLLAPSED_COUNT definitions
rg -n '(GRID|LIST)_COLLAPSED_COUNT' components/ai-explorer/country-landmarks-section.tsxRepository: birthrand/worldloop
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Get more context around the shouldExpandForFocus function and how collapsedCount is determined
rg -n 'collapsedCount|const collapsedCount' -A2 -B2 components/ai-explorer/country-landmarks-section.tsx | head -100Repository: birthrand/worldloop
Length of output: 499
🏁 Script executed:
#!/bin/bash
# See how the expanded state is used in the rendering logic
rg -n 'const expanded|expanded &&|expanded \?|return.*expanded' components/ai-explorer/country-landmarks-section.tsx | head -30Repository: birthrand/worldloop
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Find GRID_COLUMNS definition
rg -n 'GRID_COLUMNS' components/ai-explorer/country-landmarks-section.tsxRepository: birthrand/worldloop
Length of output: 293
🏁 Script executed:
#!/bin/bash
# Check the full context around focusLandmarkId initialization and usage
rg -n 'focusLandmarkId' components/ai-explorer/country-landmarks-section.tsx | head -20Repository: birthrand/worldloop
Length of output: 825
Fix expansion logic to account for visible landmarks in grid mode.
shouldExpandForFocus returns true when focusIndex > 0, but this doesn't account for which landmarks are already visible. In grid mode, GRID_COLLAPSED_COUNT = 4 (2 columns × 2 rows), so landmarks at indices 0–3 are already visible when collapsed. Focusing on a landmark at index 1, 2, or 3 will unnecessarily expand the section.
Change the logic to expand only when the focused landmark is outside the visible slice: focusIndex >= collapsedCount. This requires passing collapsedCount as a parameter to the function or determining it based on the layout state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/ai-explorer/country-landmarks-section.tsx` around lines 577 - 586,
The `shouldExpandForFocus` function currently returns true when `focusIndex >
0`, but this doesn't account for landmarks already visible in the collapsed grid
view. Since GRID_COLLAPSED_COUNT is 4 (meaning landmarks at indices 0-3 are
visible when collapsed), focusing on indices 1-3 should not trigger expansion.
Change the condition from `focusIndex > 0` to `focusIndex >= collapsedCount`
where collapsedCount represents the number of visible landmarks in the collapsed
state. You'll need to either pass `collapsedCount` as a parameter to the
`shouldExpandForFocus` function or determine it from the layout/display state,
then update all calls to this function to provide the collapsedCount argument.
Comment on lines
+161
to
+164
| const duration = Math.round( | ||
| SHEET_DISMISS_DURATION_MS * | ||
| (0.55 + Math.min(1, distance / dismissExitY) * 0.45), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Eliminate code duplication — call dismissDurationMs directly.
Lines 161-164 duplicate the dismiss-duration formula defined in the dismissDurationMs function (lines 35-38). If the formula changes, both locations must be updated.
♻️ Refactor to reuse the helper function
- const duration = Math.round(
- SHEET_DISMISS_DURATION_MS *
- (0.55 + Math.min(1, distance / dismissExitY) * 0.45),
- );
+ const duration = dismissDurationMs(distance, dismissExitY);
translateY.value = withTiming(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const duration = Math.round( | |
| SHEET_DISMISS_DURATION_MS * | |
| (0.55 + Math.min(1, distance / dismissExitY) * 0.45), | |
| ); | |
| const duration = dismissDurationMs(distance, dismissExitY); | |
| translateY.value = withTiming( |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/explore/swipe-dismiss-sheet.tsx` around lines 161 - 164, The
dismiss duration calculation in the duration variable assignment duplicates the
formula already defined in the dismissDurationMs helper function. Replace the
entire duration calculation (lines 161-164) with a direct call to the
dismissDurationMs function, passing the distance and dismissExitY parameters.
This eliminates code duplication and ensures that any future updates to the
dismiss duration formula only need to be made in one location.
Comment on lines
+15
to
+19
| const handleCopyEmail = async () => { | ||
| void Haptics.notificationAsync(Haptics.NotificationFeedbackType.Success); | ||
| await Clipboard.setStringAsync(SUPPORT_EMAIL); | ||
| Alert.alert("Email copied", `${SUPPORT_EMAIL} is ready to paste.`); | ||
| }; |
There was a problem hiding this comment.
Add error handling for async clipboard operation.
The handleCopyEmail function is async and uses await on line 17, but when called from onPress on line 46 the promise is voided. If Clipboard.setStringAsync fails (due to permissions or platform issues), the rejection will be unhandled, potentially causing a crash or silent failure.
🛡️ Proposed fix with try-catch
const handleCopyEmail = async () => {
void Haptics.notificationAsync(Haptics.NotificationFeedbackType.Success);
- await Clipboard.setStringAsync(SUPPORT_EMAIL);
- Alert.alert("Email copied", `${SUPPORT_EMAIL} is ready to paste.`);
+ try {
+ await Clipboard.setStringAsync(SUPPORT_EMAIL);
+ Alert.alert("Email copied", `${SUPPORT_EMAIL} is ready to paste.`);
+ } catch (error) {
+ Alert.alert("Copy failed", "Unable to copy email to clipboard.");
+ }
};Also applies to: 46-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/profile/help-center-contact-row.tsx` around lines 15 - 19, The
`handleCopyEmail` function lacks error handling for the
`Clipboard.setStringAsync` operation on the SUPPORT_EMAIL. Wrap the await call
to `Clipboard.setStringAsync` in a try-catch block to properly handle potential
failures from clipboard operations (such as permission issues or
platform-specific errors). In the catch block, log or alert the user about the
error instead of allowing the promise rejection to go unhandled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ship landmark pins and preview on the travel map, landmark detail modal with cached catalog data, profile history/visited/help/privacy flows, and map-country-detail navigation polish.
Note
Medium Risk
Broad touch across map navigation, profile auth metadata (Clerk), and new LLM-backed landmark endpoints; regressions could affect map handoffs or profile saves rather than core auth.
Overview
Adds a profile-driven travel map with visited-country styling, a legend, landmark pins, and landmark preview cards, plus navigation back from country detail with landmark-focused headers. Travel map is opened from profile via
openTravelMap()instead of a generic map tab jump.Profile grows into a small hub: recently viewed history, visited countries list, dedicated edit profile (Clerk name/bio/location), slimmed settings (help & privacy wired to real screens), and help center / privacy legal document routes.
Landmarks get a detail modal (save, view on map, AI “did you know”), grid/list layouts and bookmarks on country detail, scroll-to-focus when opening from map/history, and a facts carousel for country AI facts. Explore cards use facts matched to the current hero image index.
Backend adds
landmark-wikipediaandlandmark-aiendpoints, landmark AI generation with caching, and richer landmark catalog data (city, year built, UNESCO flag) with cache migration from legacy landmark keys.Reviewed by Cursor Bugbot for commit 7f25aa3. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit