Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions app/app/main/explore/page/[pageId]/_layout-base.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { ScrollRefProvider } from "@/hooks/use-scroll-ref";
import MaterialCommunityIcons from "@expo/vector-icons/MaterialCommunityIcons";
import { Link, Stack, usePathname } from "expo-router";
import { ComponentType, ReactNode } from "react";

export function withFullscreenLayout(
LayoutComp: ComponentType<{ children: ReactNode }>,
isFullscreen: boolean,
) {
function RootLayout() {
const pathname = usePathname();
const navigationName = isFullscreen
? pathname.replace("fullscreen", "map")
: pathname.replace("map", "fullscreen");

const stack = (
<Stack
screenOptions={{
headerRight: () => (
// @ts-ignore Can't be dynamically inferred
<Link asChild href={navigationName}>
<MaterialCommunityIcons
name={isFullscreen ? "fullscreen-exit" : "fullscreen"}
size={28}
color="inherit"
/>
</Link>
),
}}
/>
);

return (
<ScrollRefProvider>
<LayoutComp>{stack}</LayoutComp>
</ScrollRefProvider>
);
}

return RootLayout;
}
38 changes: 38 additions & 0 deletions app/app/main/explore/page/[pageId]/_main-base.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import useMoveTo from "@/hooks/use-move-to";
import useWikiQuery from "@/hooks/use-wiki-query";
import { useLocalSearchParams } from "expo-router";
import { useEffect } from "react";
import PageRootView from "./_page-root-view";
import { usePushCityVisit } from "@/hooks/visited-cities";

export function generatePage(isFullscreen: boolean) {
return function Page() {
let { pageId } = useLocalSearchParams();
pageId = typeof pageId === "string" ? pageId : pageId[0];
const pageQuery = useWikiQuery(pageId);
const moveTo = useMoveTo();

useEffect(() => {
if (pageQuery.data) {
moveTo(
// @ts-ignore NEEDS FIXING WHEN GEO REVISED
parseFloat(pageQuery.data.properties.geo["2"]),
// @ts-ignore NEEDS FIXING WHEN GEO REVISED
parseFloat(pageQuery.data.properties.geo["1"]),
// @ts-ignore NEEDS FIXING WHEN GEO REVISED
parseFloat(pageQuery.data.properties.geo?.zoom ?? "13"),
);
}
}, [pageQuery.data, moveTo]);
Comment on lines +15 to +26
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard moveTo: skip on fullscreen and validate geo to avoid NaN camera updates.

If geo is missing/invalid, parseFloat yields NaN; calling moveTo with NaN can misbehave. Also no need to move camera in fullscreen.

-    useEffect(() => {
-      if (pageQuery.data) {
-        moveTo(
-          // @ts-ignore NEEDS FIXING WHEN GEO REVISED
-          parseFloat(pageQuery.data.properties.geo["2"]),
-          // @ts-ignore NEEDS FIXING WHEN GEO REVISED
-          parseFloat(pageQuery.data.properties.geo["1"]),
-          // @ts-ignore NEEDS FIXING WHEN GEO REVISED
-          parseFloat(pageQuery.data.properties.geo?.zoom ?? "13"),
-        );
-      }
-    }, [pageQuery.data, moveTo]);
+    useEffect(() => {
+      if (isFullscreen || !pageQuery.data) return;
+      const geo = pageQuery.data.properties.geo;
+      const lon = Number.parseFloat(geo?.["2"]);
+      const lat = Number.parseFloat(geo?.["1"]);
+      const zoom = Number.parseFloat(geo?.zoom ?? "13");
+      if (Number.isFinite(lon) && Number.isFinite(lat)) {
+        moveTo(lon, lat, Number.isFinite(zoom) ? zoom : 13);
+      }
+    }, [isFullscreen, pageQuery.data, moveTo]);

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In app/app/main/explore/page/[pageId]/_main-base.tsx around lines 15 to 26, the
effect calls moveTo with parsed geo values without validating them and does so
even in fullscreen; parseFloat can produce NaN and cause bad camera updates.
Update the effect to first detect fullscreen mode and return early if
fullscreen, then read geo values safely (optional chaining), parse them and
verify each parsed value is a finite number (use Number.isFinite) before calling
moveTo; if validation fails, skip the moveTo call. Remove the // @ts-ignore by
using proper optional checks and typed guards so moveTo is only invoked with
valid numeric latitude, longitude and zoom.


usePushCityVisit({ id: pageId, title: pageQuery.data?.properties.title });

return (
<PageRootView
pageQuery={pageQuery}
id={pageId}
isFullscreen={isFullscreen}
/>
);
};
}
38 changes: 28 additions & 10 deletions app/app/main/explore/page/[pageId]/_page-root-view.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import { useIsFullscreen } from "@/hooks/use-is-fullscreen";
import { getCityAtom } from "@/utils/bookmarks";
import { MapMarker, MarkerType, useMapStore } from "@/utils/store";
import { NodeType, RootNode } from "@bcye/structured-wikivoyage-types";
import { BottomSheetScrollView } from "@gorhom/bottom-sheet";
import { UseQueryResult } from "@tanstack/react-query";
import { Link, Route, Stack } from "expo-router";
import { Link, Route, Stack, usePathname } from "expo-router";
import { useAtomValue } from "jotai/react";
import { filter, map, split, splitEvery } from "ramda";
import { useEffect } from "react";
Expand All @@ -14,23 +13,28 @@ import { Card, SkeletonView, Text, View } from "react-native-ui-lib";
function PageContent({
pageQuery,
id,
isFullscreen,
}: {
pageQuery: UseQueryResult<RootNode, Error>;
id: string;
isFullscreen: boolean;
}) {
const bookmarks = useAtomValue(getCityAtom(id));
const registerMarker = useMapStore((s) => s.registerMarker);
const deregisterMarker = useMapStore((s) => s.deregisterMarker);

useEffect(
function registerBookmarks() {
if (isFullscreen) return;

const markers: MapMarker[] = [];
for (const [bId, bookmark] of Object.entries(bookmarks)) {
const [lat, long] = map(parseFloat, split(",", bId));
const marker: MapMarker = {
id: bId,
// somehow broken else
link: `/main/explore/page/${id}/section/${bookmark.section}` as Route,
// if this is fullscreen, the markers are not gonna be available anywhere so it is safe to hardcode this to map view.
link: `/main/explore/page/${id}/map/section/${bookmark.section}` as Route,
Comment on lines +28 to +37
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Clarify the misleading comment.

The comment on lines 36-37 states "if this is fullscreen, the markers are not gonna be available anywhere," but the early return at line 28 ensures this code never executes when isFullscreen is true. The comment should clarify that markers are only registered in map view (non-fullscreen mode) and the hardcoded map route is intentional for that context.

Apply this diff to clarify the comment:

       if (isFullscreen) return;

       const markers: MapMarker[] = [];
       for (const [bId, bookmark] of Object.entries(bookmarks)) {
         const [lat, long] = map(parseFloat, split(",", bId));
         const marker: MapMarker = {
           id: bId,
-          // somehow broken else
-          // if this is fullscreen, the markers are not gonna be available anywhere so it is safe to hardcode this to map view.
+          // Markers are only registered in map view (non-fullscreen).
+          // Link hardcoded to map route since that's where bookmarks are displayed.
           link: `/main/explore/page/${id}/map/section/${bookmark.section}` as Route,
           lat,
           long,
           type: MarkerType.Bookmark,
📝 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
if (isFullscreen) return;
const markers: MapMarker[] = [];
for (const [bId, bookmark] of Object.entries(bookmarks)) {
const [lat, long] = map(parseFloat, split(",", bId));
const marker: MapMarker = {
id: bId,
// somehow broken else
link: `/main/explore/page/${id}/section/${bookmark.section}` as Route,
// if this is fullscreen, the markers are not gonna be available anywhere so it is safe to hardcode this to map view.
link: `/main/explore/page/${id}/map/section/${bookmark.section}` as Route,
if (isFullscreen) return;
const markers: MapMarker[] = [];
for (const [bId, bookmark] of Object.entries(bookmarks)) {
const [lat, long] = map(parseFloat, split(",", bId));
const marker: MapMarker = {
id: bId,
// Markers are only registered in map view (non-fullscreen).
// Link hardcoded to map route since that's where bookmarks are displayed.
link: `/main/explore/page/${id}/map/section/${bookmark.section}` as Route,
lat,
long,
type: MarkerType.Bookmark,
🤖 Prompt for AI Agents
In app/app/main/explore/page/[pageId]/_page-root-view.tsx around lines 28 to 37,
the inline comment is misleading because the early return at line 28 prevents
this block from running in fullscreen; update the comment to state clearly that
markers are only registered/used in the non-fullscreen map view and that
hardcoding the link to the map route is intentional for that context (e.g.,
"This code runs only in non-fullscreen map view — markers are registered here,
so hardcoding the map route is intentional").

lat,
long,
type: MarkerType.Bookmark,
Expand All @@ -45,7 +49,7 @@ function PageContent({
}
};
},
[bookmarks, id, registerMarker, deregisterMarker],
[bookmarks, id, registerMarker, deregisterMarker, isFullscreen],
);

return map(
Expand All @@ -71,12 +75,12 @@ function PageContent({
export default function PageRootView({
pageQuery,
id,
isFullscreen,
}: {
pageQuery: UseQueryResult<RootNode, Error>;
id: string | null;
isFullscreen: boolean;
}) {
const isFullscreen = useIsFullscreen();

return (
<View padding-8 flex>
<Stack.Screen
Expand All @@ -94,11 +98,19 @@ export default function PageRootView({
) : pageQuery.data && id ? (
!isFullscreen ? (
<BottomSheetScrollView>
<PageContent id={id} pageQuery={pageQuery} />
<PageContent
id={id}
pageQuery={pageQuery}
isFullscreen={isFullscreen}
/>
</BottomSheetScrollView>
) : (
<ScrollView>
<PageContent id={id} pageQuery={pageQuery} />
<PageContent
id={id}
pageQuery={pageQuery}
isFullscreen={isFullscreen}
/>
</ScrollView>
)
) : null
Expand All @@ -118,12 +130,18 @@ export default function PageRootView({
* @param pageId - The identifier for the page, used to construct the dynamic navigation route.
*/
function Infocard({ title, pageId }: { title: string; pageId: string }) {
const pathname = usePathname();
return (
<Link
asChild
href={{
pathname: "/main/explore/page/[pageId]/section/[title]",
params: { pageId, title },
// ugly debounce, pathname gets updated before navigation
// if screen stalls we end up navigating to section/X/section/X -> 404
// @ts-ignore cannot be inferred
pathname: pathname.includes("section")
? pathname
: pathname + "/section/[title]",
params: { title },
}}
Comment on lines +133 to 145
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Strengthen the pathname logic and address type suppression.

The current substring check pathname.includes("section") is fragile and could match unintended paths like /sections, /section-archive, or any pathname containing "section" as a substring. This workaround for a pathname timing issue (described as "ugly debounce") suggests a deeper architectural concern with state updates during navigation.

Additionally, the @ts-ignore at line 140 suppresses type safety for the route, which could hide legitimate type errors if the routing structure changes.

Consider these improvements:

  1. More precise pathname matching using a path segment check:
-        // ugly debounce, pathname gets updated before navigation
-        // if screen stalls we end up navigating to section/X/section/X -> 404
-        // @ts-ignore cannot be inferred
-        pathname: pathname.includes("section")
-          ? pathname
-          : pathname + "/section/[title]",
+        pathname: pathname.split("/").includes("section")
+          ? pathname
+          : (pathname + "/section/[title]") as Route,
         params: { title },
  1. Alternatively, investigate the root cause of the pathname update timing issue. If pathname is updating before navigation completes, consider using a ref or route state to track navigation intent rather than relying on pathname parsing. This would eliminate the need for the workaround entirely.

  2. For the type error: Rather than @ts-ignore, use a type assertion (as Route) after constructing the pathname string, which at least preserves some type checking.

📝 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 pathname = usePathname();
return (
<Link
asChild
href={{
pathname: "/main/explore/page/[pageId]/section/[title]",
params: { pageId, title },
// ugly debounce, pathname gets updated before navigation
// if screen stalls we end up navigating to section/X/section/X -> 404
// @ts-ignore cannot be inferred
pathname: pathname.includes("section")
? pathname
: pathname + "/section/[title]",
params: { title },
}}
const pathname = usePathname();
return (
<Link
asChild
href={{
pathname: pathname.split("/").includes("section")
? pathname
: (pathname + "/section/[title]") as Route,
params: { title },
}}

>
<Card flex padding-12 height={48}>
Expand Down
114 changes: 114 additions & 0 deletions app/app/main/explore/page/[pageId]/_section-base.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import WikiContent from "@/components/render-node";
import { useScrollRef } from "@/hooks/use-scroll-ref";
import useWikiQuery from "@/hooks/use-wiki-query";
import { citiesAtom, getCityAtom } from "@/utils/bookmarks";
import { NodeType, SectionNode } from "@bcye/structured-wikivoyage-types";
import { BottomSheetScrollView } from "@gorhom/bottom-sheet";
import { Stack, useLocalSearchParams } from "expo-router";
import { useAtom } from "jotai/react";
import { append, assoc, dissoc } from "ramda";
import { useCallback } from "react";
import { ScrollView } from "react-native";
import { SkeletonView, View } from "react-native-ui-lib";
import { toast } from "sonner-native";

export function generateSection(isFullscreen: boolean) {
/**
* Renders a specific section of a Wikipedia page in Markdown format.
*
* The component retrieves the page title and ID from local search parameters, fetches the page's wikitext
* via a TRPC query, and then extracts and converts the designated section—identified by the title—to Markdown.
* It slices off the section header from the converted Markdown and displays a skeleton view while the content loads.
*/
return function Section() {
const { title, pageId } = useLocalSearchParams();
const wikiQuery = useWikiQuery(pageId as string);
const section = wikiQuery.data?.children.find(
(c) => c.type === NodeType.Section && c.properties.title === title,
) as SectionNode | undefined;
const ref = useScrollRef();
const [bookmarks, setBookmarks] = useAtom(getCityAtom(pageId as string));
const [cities, setCities] = useAtom(citiesAtom);
const pageTitle = wikiQuery.data?.properties.title;

const isBookmarked = useCallback(
function isBookmarked(id: string) {
return !!bookmarks[id];
},
[bookmarks],
);

const toggleBookmarked = useCallback(
function toggleBookmarked(id: string) {
if (id == ",") {
toast.error(
"This listing doesn't have a location and can't be bookmarked. Add one on en.wikivoyage.org",
);
return;
}

if (isBookmarked(id)) {
setBookmarks(dissoc(id, bookmarks));
} else {
if (!cities.find((c) => c.qid === pageId)) {
setCities(
append({ qid: pageId as string, name: pageTitle! }, cities),
);
}
// no valid lat-lng
setBookmarks(
assoc(
id,
{ section: title, properties: section!.properties },
bookmarks,
),
);
}
},
[
bookmarks,
setBookmarks,
section,
isBookmarked,
pageTitle,
cities,
setCities,
pageId,
title,
],
);

if (!section) return null;

return (
<View padding-8 flex>
<Stack.Screen options={{ title: section.properties.title }} />
<SkeletonView
template={SkeletonView.templates.TEXT_CONTENT}
showContent={wikiQuery.isSuccess}
renderContent={() =>
!isFullscreen ? (
<BottomSheetScrollView ref={ref}>
<WikiContent
node={section}
root={true}
isBookmarked={isBookmarked}
toggleBookmarked={toggleBookmarked}
/>
</BottomSheetScrollView>
) : (
<ScrollView>
<WikiContent
node={section}
root={true}
isBookmarked={isBookmarked}
toggleBookmarked={toggleBookmarked}
/>
</ScrollView>
)
}
/>
</View>
);
};
}
4 changes: 4 additions & 0 deletions app/app/main/explore/page/[pageId]/fullscreen/_layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import { Fragment } from "react";
import { withFullscreenLayout } from "../_layout-base";

export default withFullscreenLayout(Fragment, true);
2 changes: 2 additions & 0 deletions app/app/main/explore/page/[pageId]/fullscreen/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import { generatePage } from "../_main-base";
export default generatePage(true);
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { generateSection } from "../../_section-base";

export default generateSection(true);
30 changes: 0 additions & 30 deletions app/app/main/explore/page/[pageId]/index.tsx

This file was deleted.

Loading
Loading