fix(directory-picker): address review — modal layering, root nav, stale state
- HIGH: the "Browse Folders..." entry in the New Session RN <Modal> 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).
This commit is contained in:
@@ -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<BottomSheet | null>
|
||||
@@ -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<string | null>(null)
|
||||
const [entries, setEntries] = useState<FileEntry[]>([])
|
||||
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 ? <Text style={[s.emptyText, isDark && s.dimDark]}>No subfolders here</Text> : null
|
||||
!loading && !error ? (
|
||||
<Text style={[s.emptyText, isDark && s.dimDark]}>
|
||||
{browseDir ? "No subfolders here" : "Enter a path above to start browsing"}
|
||||
</Text>
|
||||
) : null
|
||||
}
|
||||
/>
|
||||
|
||||
|
||||
88
src/lib/path-utils.test.ts
Normal file
88
src/lib/path-utils.test.ts
Normal file
@@ -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")
|
||||
})
|
||||
38
src/lib/path-utils.ts
Normal file
38
src/lib/path-utils.ts
Normal file
@@ -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
|
||||
}
|
||||
Reference in New Issue
Block a user