From 7e9b3981c3e948d0670421a79e6474c04054f14d Mon Sep 17 00:00:00 2001 From: engineer Date: Thu, 16 Jul 2026 15:57:31 -0700 Subject: [PATCH] =?UTF-8?q?fix(directory-picker):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20modal=20layering,=20root=20nav,=20stale=20state?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - HIGH: the "Browse Folders..." entry in the New Session RN expanded a sibling BottomSheet, which a native Modal always covers (a BottomSheetModal through the root portal would be covered too), so the primary entry point was invisible/untouchable. The modal is now closed before the sheet expands and restored on cancel via a new onDismiss callback (restoreNewSessionOnDismiss ref); picking a folder proceeds to session creation without reopening the modal. - MEDIUM: parentOf("/") returned "/" so Up at the POSIX root looped forever; it now returns null at "/", "\" and Windows drive roots alike, disabling the Up button there. - LOW: opening the sheet with no known start directory (server home not loaded yet) showed the previous open's stale entries; it now clears state, invalidates in-flight loads, and shows an "Enter a path above to start browsing" empty state. Sheet init also no longer re-runs on snap-point drags (wasOpen guard). - Extracted the pure path helpers (stripTrailingSlash/parentOf/nameOf) into src/lib/path-utils.ts (no RN imports) with node --test coverage for POSIX root, Windows drive roots, trailing slashes, and backslash paths. typecheck clean; 97/97 tests pass (16 new). --- app/(tabs)/index.tsx | 21 ++++- src/components/chat/DirectoryBrowserSheet.tsx | 67 ++++++++------ src/lib/path-utils.test.ts | 88 +++++++++++++++++++ src/lib/path-utils.ts | 38 ++++++++ 4 files changed, 184 insertions(+), 30 deletions(-) create mode 100644 src/lib/path-utils.test.ts create mode 100644 src/lib/path-utils.ts diff --git a/app/(tabs)/index.tsx b/app/(tabs)/index.tsx index 81c40b8..6ca78ae 100644 --- a/app/(tabs)/index.tsx +++ b/app/(tabs)/index.tsx @@ -275,17 +275,28 @@ export default function SessionsScreen() { } } + // The browser sheet is a sibling of the New Session . A native RN + // Modal layers above everything in the React root (including bottom-sheet + // portals), so the modal must be closed before the sheet is shown; this ref + // remembers to bring it back if the user cancels without picking a folder. + const restoreNewSessionOnDismiss = useRef(false) + const openBrowser = useCallback( (startDir: string | null, mode: "create" | "switch") => { setBrowseStartDir(startDir || serverHome || null) setBrowseMode(mode) + if (mode === "create" && showNewSession) { + restoreNewSessionOnDismiss.current = true + setShowNewSession(false) + } browserSheetRef.current?.expand() }, - [serverHome], + [serverHome, showNewSession], ) const onBrowserSelect = useCallback( (directory: string) => { + restoreNewSessionOnDismiss.current = false if (browseMode === "switch") { handleSwitchDirectory(directory) dirSheetRef.current?.close() @@ -296,6 +307,13 @@ export default function SessionsScreen() { [browseMode, handleSwitchDirectory, onCreateInDirectory], ) + const onBrowserDismiss = useCallback(() => { + if (restoreNewSessionOnDismiss.current) { + restoreNewSessionOnDismiss.current = false + setShowNewSession(true) + } + }, []) + const onFabPress = () => { // Quick create in current project onCreateSession() @@ -666,6 +684,7 @@ export default function SessionsScreen() { clientForDirectory={clientForDirectory} isDark={isDark} onSelect={onBrowserSelect} + onDismiss={onBrowserDismiss} /> ) diff --git a/src/components/chat/DirectoryBrowserSheet.tsx b/src/components/chat/DirectoryBrowserSheet.tsx index 4b486d1..5f9df9a 100644 --- a/src/components/chat/DirectoryBrowserSheet.tsx +++ b/src/components/chat/DirectoryBrowserSheet.tsx @@ -3,6 +3,7 @@ import { ActivityIndicator, StyleSheet, Text, TouchableOpacity, View } from "rea import { Ionicons } from "@expo/vector-icons" import BottomSheet, { BottomSheetBackdrop, BottomSheetFlatList, BottomSheetTextInput } from "@gorhom/bottom-sheet" import type { Client, FileEntry } from "../../lib/sdk" +import { parentOf, nameOf } from "../../lib/path-utils" interface Props { sheetRef: React.RefObject @@ -13,32 +14,18 @@ interface Props { isDark: boolean // Called with the chosen absolute directory when the user taps "Use this folder". onSelect: (directory: string) => void + // Called whenever the sheet fully closes (selection or cancel). + onDismiss?: () => void } -// Absolute-path helpers. Server working directories can be POSIX (/a/b) or -// Windows (C:\a\b, D:/a/b) since the mobile app can point at either kind of -// opencode server, so both separators are handled. -function stripTrailingSlash(dir: string): string { - return dir.replace(/[\\/]+$/, "") || dir -} - -function parentOf(dir: string): string | null { - const trimmed = stripTrailingSlash(dir) - const lastSlash = Math.max(trimmed.lastIndexOf("/"), trimmed.lastIndexOf("\\")) - if (lastSlash < 0) return null - const head = trimmed.slice(0, lastSlash) - if (!head) return trimmed[0] === "\\" ? "\\" : "/" // reached posix root - if (/^[a-zA-Z]:$/.test(head)) return `${head}\\` // reached a windows drive root - return head -} - -function nameOf(dir: string): string { - const trimmed = stripTrailingSlash(dir) - const lastSlash = Math.max(trimmed.lastIndexOf("/"), trimmed.lastIndexOf("\\")) - return lastSlash >= 0 ? trimmed.slice(lastSlash + 1) || trimmed : trimmed -} - -export function DirectoryBrowserSheet({ sheetRef, startDirectory, clientForDirectory, isDark, onSelect }: Props) { +export function DirectoryBrowserSheet({ + sheetRef, + startDirectory, + clientForDirectory, + isDark, + onSelect, + onDismiss, +}: Props) { const [browseDir, setBrowseDir] = useState(null) const [entries, setEntries] = useState([]) const [loading, setLoading] = useState(false) @@ -84,14 +71,32 @@ export function DirectoryBrowserSheet({ sheetRef, startDirectory, clientForDirec [load], ) - // Reset to the starting directory every time the sheet opens. + // Reset to the starting directory when the sheet transitions from closed + // to open (not on drags between snap points), and notify on full close. + const wasOpen = useRef(false) const handleSheetChange = useCallback( (index: number) => { - if (index < 0) return + if (index < 0) { + wasOpen.current = false + onDismiss?.() + return + } + if (wasOpen.current) return // snap-point change while already open + wasOpen.current = true setJumpPath("") - if (startDirectory) enter(startDirectory) + if (startDirectory) { + enter(startDirectory) + } else { + // No starting directory known (e.g. server home not loaded yet): + // show an explicit empty state instead of a previous open's entries. + loadToken.current++ + setBrowseDir(null) + setEntries([]) + setError(null) + setLoading(false) + } }, - [startDirectory, enter], + [startDirectory, enter, onDismiss], ) const goUp = useCallback(() => { @@ -196,7 +201,11 @@ export function DirectoryBrowserSheet({ sheetRef, startDirectory, clientForDirec ) : null } ListEmptyComponent={ - !loading && !error ? No subfolders here : null + !loading && !error ? ( + + {browseDir ? "No subfolders here" : "Enter a path above to start browsing"} + + ) : null } /> diff --git a/src/lib/path-utils.test.ts b/src/lib/path-utils.test.ts new file mode 100644 index 0000000..7b50892 --- /dev/null +++ b/src/lib/path-utils.test.ts @@ -0,0 +1,88 @@ +import { test } from "node:test" +import assert from "node:assert/strict" +import { stripTrailingSlash, parentOf, nameOf } from "./path-utils.ts" + +// stripTrailingSlash ------------------------------------------------------- + +test("stripTrailingSlash: removes single and repeated trailing separators", () => { + assert.equal(stripTrailingSlash("/a/b/"), "/a/b") + assert.equal(stripTrailingSlash("/a/b///"), "/a/b") + assert.equal(stripTrailingSlash("C:\\proj\\"), "C:\\proj") +}) + +test("stripTrailingSlash: keeps input when stripping would empty it", () => { + assert.equal(stripTrailingSlash("/"), "/") + assert.equal(stripTrailingSlash("\\"), "\\") + assert.equal(stripTrailingSlash("///"), "///") +}) + +test("stripTrailingSlash: leaves paths without trailing separator unchanged", () => { + assert.equal(stripTrailingSlash("/a/b"), "/a/b") + assert.equal(stripTrailingSlash("D:/work"), "D:/work") +}) + +// parentOf ----------------------------------------------------------------- + +test("parentOf: POSIX root has no parent", () => { + assert.equal(parentOf("/"), null) + assert.equal(parentOf("//"), null) +}) + +test("parentOf: bare backslash has no parent", () => { + assert.equal(parentOf("\\"), null) +}) + +test("parentOf: Windows drive roots have no parent", () => { + assert.equal(parentOf("C:\\"), null) + assert.equal(parentOf("D:/"), null) + assert.equal(parentOf("c:"), null) +}) + +test("parentOf: first-level POSIX dir returns the root", () => { + assert.equal(parentOf("/home"), "/") + assert.equal(parentOf("/home/"), "/") +}) + +test("parentOf: nested POSIX paths walk up one level", () => { + assert.equal(parentOf("/home/user/project"), "/home/user") + assert.equal(parentOf("/home/user/project/"), "/home/user") +}) + +test("parentOf: first-level Windows dir returns the drive root", () => { + assert.equal(parentOf("C:\\projects"), "C:\\") + assert.equal(parentOf("D:/work"), "D:\\") +}) + +test("parentOf: nested Windows backslash paths walk up one level", () => { + assert.equal(parentOf("C:\\projects\\app"), "C:\\projects") + assert.equal(parentOf("C:\\projects\\app\\"), "C:\\projects") +}) + +test("parentOf: mixed-separator Windows paths walk up one level", () => { + assert.equal(parentOf("D:/work/repo"), "D:/work") +}) + +test("parentOf: relative segment without separators has no parent", () => { + assert.equal(parentOf("project"), null) +}) + +// nameOf --------------------------------------------------------------------- + +test("nameOf: returns the last POSIX segment", () => { + assert.equal(nameOf("/home/user/project"), "project") + assert.equal(nameOf("/home/user/project/"), "project") +}) + +test("nameOf: returns the last Windows segment", () => { + assert.equal(nameOf("C:\\projects\\app"), "app") + assert.equal(nameOf("D:/work/repo"), "repo") +}) + +test("nameOf: root paths fall back to the trimmed input", () => { + assert.equal(nameOf("/"), "/") + assert.equal(nameOf("C:\\"), "C:") +}) + +test("nameOf: bare segment is returned as-is", () => { + assert.equal(nameOf("project"), "project") +}) diff --git a/src/lib/path-utils.ts b/src/lib/path-utils.ts new file mode 100644 index 0000000..176fe71 --- /dev/null +++ b/src/lib/path-utils.ts @@ -0,0 +1,38 @@ +// Pure absolute-path helpers for the server-filesystem browser. +// No React Native imports — unit-testable with node --test. +// +// Server working directories can be POSIX (/a/b) or Windows (C:\a\b, D:/a/b) +// since the mobile app can point at either kind of opencode server, so both +// separators are handled. + +/** Remove trailing slashes/backslashes, keeping the input if that would empty it. */ +export function stripTrailingSlash(dir: string): string { + return dir.replace(/[\\/]+$/, "") || dir +} + +function isRoot(trimmed: string): boolean { + // POSIX root ("/", "//"), a bare backslash, or a Windows drive root ("C:"). + return /^[\\/]+$/.test(trimmed) || /^[a-zA-Z]:$/.test(trimmed) +} + +/** + * Parent directory of an absolute path, or null when already at a + * filesystem root (POSIX "/" or a Windows drive root like "C:\"). + */ +export function parentOf(dir: string): string | null { + const trimmed = stripTrailingSlash(dir) + if (isRoot(trimmed)) return null + const lastSlash = Math.max(trimmed.lastIndexOf("/"), trimmed.lastIndexOf("\\")) + if (lastSlash < 0) return null + const head = trimmed.slice(0, lastSlash) + if (!head) return trimmed[0] === "\\" ? "\\" : "/" // reached posix root + if (/^[a-zA-Z]:$/.test(head)) return `${head}\\` // reached a windows drive root + return head +} + +/** Last path segment, e.g. "/a/b/" -> "b", "C:\\proj" -> "proj". */ +export function nameOf(dir: string): string { + const trimmed = stripTrailingSlash(dir) + const lastSlash = Math.max(trimmed.lastIndexOf("/"), trimmed.lastIndexOf("\\")) + return lastSlash >= 0 ? trimmed.slice(lastSlash + 1) || trimmed : trimmed +}