diff --git a/app/session/[id].tsx b/app/session/[id].tsx index 97f885f0..a3eac2ab 100644 --- a/app/session/[id].tsx +++ b/app/session/[id].tsx @@ -11,6 +11,9 @@ import { Platform, ActivityIndicator, Alert, + type CellRendererProps, + type NativeSyntheticEvent, + type NativeScrollEvent, } from "react-native" import { useLocalSearchParams, Stack, useRouter, useFocusEffect } from "expo-router" import { Ionicons } from "@expo/vector-icons" @@ -39,6 +42,7 @@ import { useConnections } from "../../src/stores/connections" import { useAuth } from "../../src/stores/auth" import { useCatalog } from "../../src/stores/catalog" import { useSpeech } from "../../src/lib/speech" +import { MessageNavigation } from "../../src/lib/message-navigation" // --- Builtin slash commands --- const BUILTIN_COMMANDS: SlashCommand[] = [ @@ -80,6 +84,9 @@ export default function SessionScreen() { const { t } = useTranslation() const flatListRef = useRef(null) + const navigation = useRef(new MessageNavigation()).current + const retry = useRef | undefined>(undefined) + const attempts = useRef(0) const modelSheetRef = useRef(null) const variantSheetRef = useRef(null) const [input, setInput] = useState("") @@ -181,6 +188,7 @@ export default function SessionScreen() { const messageData = useMemo( () => (messages || []) + .filter((msg) => msg.role === "user" || msg.role === "assistant") .filter((msg) => !revertMessageID || msg.id.startsWith("temp-") || msg.id < revertMessageID) .map((msg) => ({ message: msg, @@ -190,6 +198,42 @@ export default function SessionScreen() { [messages, parts, revertMessageID], ) + navigation.sync(messageData.map((item) => item.message.id)) + + const seekMessage = useCallback(() => { + if (retry.current) clearTimeout(retry.current) + if (flatListRef.current && navigation.seek(flatListRef.current, attempts.current++)) { + retry.current = setTimeout(seekMessage, 100) + } + }, [navigation]) + + const MessageCell = useCallback((props: CellRendererProps<(typeof messageData)[number]>) => { + const key = props.item.message.id + const { onLayout, ...cell } = props + useEffect(() => () => { navigation.frames.delete(key) }, [key, navigation]) + return ( + { + navigation.frames.set(key, event.nativeEvent.layout) + onLayout?.(event) + if (navigation.target === key) seekMessage() + }}> + {props.children} + + ) + }, [navigation, seekMessage]) + + useEffect(() => { + navigation.manual() + navigation.frames.clear() + return () => { if (retry.current) clearTimeout(retry.current) } + }, [id, navigation]) + + const moveMessage = (direction: 1 | -1) => { + if (navigation.move(direction) === undefined) return + attempts.current = 0 + seekMessage() + } + // Tracks the latest composer text without pulling `input` into // handleMessageLongPress's deps — kept as a plain ref assignment (not // state) so the callback below stays referentially stable across @@ -252,8 +296,10 @@ export default function SessionScreen() { }, [applyRevertResult, t]) const scrollToBottom = useCallback((animated = true) => { + navigation.manual(0) + if (retry.current) clearTimeout(retry.current) flatListRef.current?.scrollToOffset({ offset: 0, animated }) - }, []) + }, [navigation]) // Re-select on every focus, not just mount. currentSession/messages/ // permissions are a single global store, and the native stack keeps screens @@ -451,10 +497,11 @@ export default function SessionScreen() { } // In inverted mode, offset 0 = bottom. Show scroll button when scrolled away from bottom. - const handleScroll = useCallback((event: any) => { - const { contentOffset } = event.nativeEvent + const handleScroll = useCallback((event: NativeSyntheticEvent) => { + const { contentOffset, layoutMeasurement } = event.nativeEvent + navigation.observe(contentOffset.y, layoutMeasurement.height) setShowScrollButton(contentOffset.y > 200) - }, []) + }, [navigation]) // Debounce: onEndReached can fire multiple times during a single scroll gesture const loadingTriggered = useRef(false) @@ -621,6 +668,8 @@ export default function SessionScreen() { if (hasMore && !loadingMore) loadOlderMessages() }} onScrollToTop={() => { + navigation.manual() + if (retry.current) clearTimeout(retry.current) flatListRef.current?.scrollToEnd({ animated: true }) }} onClose={() => setShowInfo(false)} @@ -669,6 +718,17 @@ export default function SessionScreen() { ref={flatListRef} data={messageData} inverted + CellRendererComponent={MessageCell} + onLayout={(event) => { + navigation.observe(navigation.offset, event.nativeEvent.layout.height) + }} + onScrollBeginDrag={() => { + navigation.manual() + if (retry.current) clearTimeout(retry.current) + }} + onScrollToIndexFailed={({ index, averageItemLength }) => { + flatListRef.current?.scrollToOffset({ offset: averageItemLength * index, animated: false }) + }} keyExtractor={(item) => item.message.id} renderItem={({ item }) => ( {t("session.empty.hint")} )} - {showScrollButton && ( - scrollToBottom(true)}> - - + {messageData.length > 0 && ( + + moveMessage(1)}> + + + moveMessage(-1)}> + + + {showScrollButton && ( + scrollToBottom(true)}> + + + + )} + {!showScrollButton && } + )} )} @@ -868,13 +943,23 @@ const s = StyleSheet.create({ listWrap: { flex: 1, position: "relative" }, // Messages - messageList: { padding: 16, paddingBottom: 8 }, + // Reserve 44pt controls + 16pt inset + 16pt gap on the physical right. + // Android's inverted list flips both axes, so its content padding is mirrored. + messageList: { + padding: 16, + paddingBottom: 8, + paddingLeft: Platform.OS === "android" ? 76 : 16, + paddingRight: Platform.OS === "android" ? 16 : 76, + }, // Scroll button - scrollBtn: { + scrollControls: { position: "absolute", bottom: 16, right: 16, + gap: 4, + }, + scrollBtn: { width: 44, height: 44, borderRadius: 22, diff --git a/docs/qa/ISSUE-214-MESSAGE-NAVIGATION.md b/docs/qa/ISSUE-214-MESSAGE-NAVIGATION.md new file mode 100644 index 00000000..760e01cf --- /dev/null +++ b/docs/qa/ISSUE-214-MESSAGE-NAVIGATION.md @@ -0,0 +1,95 @@ +# Issue 214: Message Navigation Validation + +## Scope + +Previous and Next use single carets immediately above double-caret Latest. +Targets are the starts of rendered user/assistant messages in the inverted +FlatList. Layout-only/system rows are not navigation targets. + +The change does not modify native keyboard handling, the composer, safe-area +configuration, server code, release metadata, or unrelated IME PR #208. + +## Automated Gates + +- `npm ci --legacy-peer-deps`: completed using the existing lockfile. +- `npm test`: 348 passed, zero failures (15 navigation regressions). +- `npm run typecheck`: passed. +- `npm run check:versions`: passed, existing 0.4.15 / versionCode 42 unchanged. +- `git diff --check`: passed. +- `npm run lint`: unavailable; package.json has no lint script despite the + contributing guide naming one. No lint pass is claimed. +- Android `./gradlew assembleRelease -PreactNativeArchitectures=arm64-v8a + --max-workers=2 --console=plain`: passed under a capacity lease, with SDK/JDK + environment configured and `SENTRY_DISABLE_AUTO_UPLOAD=true`. + +The first build reached signing and failed because the checkout lacked +`android/app/debug.keystore`. Generating the ignored development key using the +same setup as CI allowed the build to pass; no signing material is committed. + +## Physical Android Runtime + +Device: Pixel 8 Pro. The installed production package had a different signing +certificate, so it was preserved. A temporary external Gradle init script used +the supported `androidComponents.finalizeDsl` API to build a separate +`cc.agentlabs.opencode.navigation214` test package. No tracked Android source or +configuration changed. The JavaScript bundle contains the actual feature diff. + +The existing `tests/fixtures/mock-opencode-server.ts` supplied a separate test +connection via ADB reverse. The fixture was seeded through its REST API with +25 numbered user messages and 25 assistant responses, each containing 70 lines. +This is mock protocol coverage, not live AI/backend validation. + +Snapshot-driven ADB assertions passed these flows: + +1. From the newest response's partially visible end, Previous reaches user + Message 25 at the viewport top, then the older assistant's line 1, then user + Message 24. The repeated tap advances at exact message-start boundaries. +2. Next reaches the adjacent assistant's line 1, then user Message 25. +3. Manually scrolling into the middle of the long response discards the tap + anchor; Previous reaches Message 24 from the new viewport position. +4. With an unsent draft and keyboard open, tapping navigation preserves the + draft and composer `focused=true`; Android input-method state remains shown. +5. Latest reaches line 70 of the newest response and preserves the draft. +6. Seventeen consecutive Previous taps reach numbered Message 17 through every + intermediate message start, beyond the initial render window. Latest then + returns to the newest response end. + +The harness first needed two test-only corrections: keyboard capitalization +must be accounted for, and keyboard dismissal must not send a blind Back key +when the keyboard is already closed. Neither required a product change. + +Screenshots from the actual physical-device build: + +- [Message start and controls](screenshots/issue-214-message-navigation.png) +- [Keyboard, draft and controls](screenshots/issue-214-keyboard-navigation.png) + +The keyboard screenshot is not evidence that the composer is fully visible +above this device's IME. Existing keyboard layout work remains outside scope. + +## Independent Review + +An independent actual-diff code/behavior/UX review initially found three +issues: virtualized estimates could stall, completed targets could repin a +streaming response away from Latest, and resizing could leave a stale anchor. +All three were remediated and regression-tested. + +The independent final review ran the 15 focused tests, typecheck and diff check, +inspected installed React Native 0.81.5 list internals, the runtime harness and +both screenshots. Verdict: **PASS code/static compact-control UX; BLOCKED merge +readiness**. No remaining actionable correctness defect was established. + +## Blocked Gates + +- Mandatory vision CUA was actually attempted with Python 3.12 and the existing + runner against the isolated installed package. It exited 1 with: + `Set AZURE_OPENAI_API_KEY, AZURE_DEV_AI_API_KEY, OPENAI_API_KEY, XAI_API_KEY, or GEMINI_API_KEY`. + The documented `~/.env.d/azure-openai.env` file is absent. Deterministic ADB + assertions do not substitute for this gate. +- The documented live server's `/global/health` request timed out after 15 + seconds. No server/daemon changes or permission workarounds were attempted. +- Native rapid-tap callback ordering, live streaming/appends, and constrained + keyboard/composer layout remain incompletely validated. Pure regressions + cover rapid pending taps, growth/appends, resizing and manual cancellation. + +Do not merge or release until the mandatory CUA gate passes and remaining +runtime gaps have been checked. The focused PR remains draft while blocked. diff --git a/docs/qa/screenshots/issue-214-keyboard-navigation.png b/docs/qa/screenshots/issue-214-keyboard-navigation.png new file mode 100644 index 00000000..5c0b7d50 Binary files /dev/null and b/docs/qa/screenshots/issue-214-keyboard-navigation.png differ diff --git a/docs/qa/screenshots/issue-214-message-navigation.png b/docs/qa/screenshots/issue-214-message-navigation.png new file mode 100644 index 00000000..ed25c056 Binary files /dev/null and b/docs/qa/screenshots/issue-214-message-navigation.png differ diff --git a/src/components/chat/message-navigation-layout.regression.test.ts b/src/components/chat/message-navigation-layout.regression.test.ts new file mode 100644 index 00000000..bd4b1656 --- /dev/null +++ b/src/components/chat/message-navigation-layout.regression.test.ts @@ -0,0 +1,29 @@ +import { test } from "node:test" +import assert from "node:assert/strict" +import { readFileSync } from "node:fs" +import { runInNewContext } from "node:vm" + +// Follow the existing source-level RN regression pattern: node:test cannot +// render native layouts. This guard fails on the original overlapping layout; +// changed-head device/keyboard acceptance is still required. +const source = readFileSync(new URL("../../../app/session/[id].tsx", import.meta.url), "utf8") + +test("message content reserves a gutter for the floating navigation controls", () => { + assert.match(source, /contentContainerStyle=\{s\.messageList\}/) + assert.match(source, //) + const content = source.match(/messageList:\s*\{([^}]+)\}/)?.[1] ?? "" + const controls = source.match(/scrollControls:\s*\{([^}]+)\}/)?.[1] ?? "" + const button = source.match(/scrollBtn:\s*\{([^}]+)\}/)?.[1] ?? "" + const inset = Number(controls.match(/right:\s*(\d+)/)?.[1]) + const width = Number(button.match(/width:\s*(\d+)/)?.[1]) + const height = Number(button.match(/height:\s*(\d+)/)?.[1]) + + assert.ok(width >= 44 && height >= 44, "keep practical navigation hit targets") + for (const os of ["android", "ios"]) { + const style = runInNewContext(`({${content}})`, { Platform: { OS: os } }) + // RN 0.81's Android inverted list uses scale:-1 (both axes), while iOS + // uses scaleY:-1. Content padding is transformed; the overlay is not. + const padding = os === "android" ? style.paddingLeft ?? style.padding : style.paddingRight ?? style.padding + assert.ok(padding >= inset + width + 16, `${os}: physical-right text must clear the control rail and gap`) + } +}) diff --git a/src/lib/message-navigation.test.ts b/src/lib/message-navigation.test.ts new file mode 100644 index 00000000..88499e18 --- /dev/null +++ b/src/lib/message-navigation.test.ts @@ -0,0 +1,217 @@ +import { test } from "node:test" +import assert from "node:assert/strict" +import { MessageNavigation } from "./message-navigation.ts" + +function list() { + const nav = new MessageNavigation() + nav.sync(["new", "long", "old"]) + nav.height = 200 + nav.frames.set("new", { y: 0, height: 100 }) + nav.frames.set("long", { y: 100, height: 1000 }) + nav.frames.set("old", { y: 1100, height: 100 }) + return nav +} + +test("viewport top anchors a partially visible long message, not the newest visible row", () => { + const nav = list() + assert.equal(nav.current(), 1) + assert.equal(nav.move(1), 2) + assert.equal(nav.destination(), 1000) +}) + +test("exact message starts belong to that message, not its older neighbor", () => { + const nav = list() + nav.offset = 900 + assert.equal(nav.current(), 1) + assert.equal(nav.move(-1), 0) + assert.equal(nav.destination(), 0) +}) + +test("repeated taps advance before the native scroll callback arrives", () => { + const nav = list() + nav.offset = 1000 + assert.equal(nav.move(-1), 1) + assert.equal(nav.destination(), 900) + assert.equal(nav.move(-1), 0) + assert.equal(nav.move(-1), undefined) + assert.equal(nav.target, "new") +}) + +test("empty, unmeasured and oldest bounds are no-ops", () => { + const nav = new MessageNavigation() + assert.equal(nav.move(1), undefined) + nav.sync(["only"]) + assert.equal(nav.move(-1), undefined) + const measured = list() + measured.offset = 1000 + assert.equal(measured.move(1), undefined) +}) + +test("virtualized offscreen target resolves only after its actual layout arrives", () => { + const nav = list() + nav.frames.delete("old") + assert.equal(nav.move(1), 2) + assert.equal(nav.destination(), undefined) + nav.frames.set("old", { y: 1100, height: 500 }) + assert.equal(nav.destination(), 1400) +}) + +test("streaming growth and appends preserve pending target ID rather than stale index", () => { + const nav = list() + nav.move(1) + nav.sync(["appended", "new", "long", "old"]) + nav.frames.set("old", { y: 1500, height: 100 }) + assert.equal(nav.current(), 3) + assert.equal(nav.destination(), 1400) + assert.equal(nav.move(-1), 2) + assert.equal(nav.target, "long") +}) + +test("manual scroll discards tap anchor and derives the next target from viewport", () => { + const nav = list() + nav.move(1) + nav.manual() + nav.offset = 400 + assert.equal(nav.current(), 1) + assert.equal(nav.move(-1), 0) +}) + +test("revert/removal discards missing targets and layouts", () => { + const nav = list() + nav.move(1) + nav.sync(["new", "long"]) + assert.equal(nav.target, undefined) + assert.equal(nav.frames.has("old"), false) + assert.equal(nav.current(), 1) +}) + +test("layout-only zero-height rows cannot become viewport anchors", () => { + const nav = list() + nav.frames.set("long", { y: 100, height: 0 }) + assert.equal(nav.current(), 0) +}) + +test("viewport resizing changes the inverted top coordinate", () => { + const nav = list() + nav.height = 100 + assert.equal(nav.current(), 0) + nav.height = 200 + assert.equal(nav.current(), 1) +}) + +test("completed newest navigation does not top-pin subsequent streaming growth", () => { + const nav = list() + nav.move(-1) + nav.complete(nav.destination()!) + nav.frames.set("new", { y: 0, height: 500 }) + assert.equal(nav.destination(), undefined) + assert.equal(nav.target, undefined) + assert.equal(nav.current(), 0) +}) + +test("completed tap anchor is invalidated by keyboard resize and viewport scroll", () => { + const nav = list() + nav.move(1) + nav.complete(nav.destination()!) + nav.observe(1000, 200) + assert.equal(nav.current(), 2) + nav.observe(1000, 100) + assert.equal(nav.current(), 1) + assert.equal(nav.move(-1), 0) + nav.complete(0) + nav.observe(400, 200) + assert.equal(nav.current(), 1) +}) + +test("completed clamped short-message taps advance until a viewport change", () => { + const nav = list() + nav.move(-1) + nav.complete(0) + nav.observe(0, 200) + assert.equal(nav.current(), 0) + assert.equal(nav.move(-1), undefined) + nav.manual() + assert.equal(nav.current(), 1) +}) + +test("accepted virtualized estimates keep seeking, then settle to actual measured top", () => { + const nav = list() + nav.frames.delete("old") + const calls: unknown[] = [] + const driver = { + scrollToIndex(params: unknown) { calls.push(params) }, + scrollToOffset(params: { offset: number; animated: boolean }) { + calls.push(params) + nav.observe(params.offset, nav.height) + }, + } + nav.move(1) + assert.equal(nav.seek(driver, 0), true) + assert.deepEqual(calls[0], { index: 2, viewPosition: 1, animated: false }) + assert.equal(nav.seek(driver, 1), true) + assert.deepEqual(calls[1], { offset: 160, animated: false }) + nav.frames.set("old", { y: 1100, height: 500 }) + assert.equal(nav.seek(driver, 2), false) + assert.deepEqual(calls[2], { offset: 1400, animated: false }) + assert.equal(nav.target, undefined) + assert.equal(nav.current(), 2) +}) + +test("manual cancellation and retry exhaustion stop all seek side effects", () => { + const nav = list() + nav.frames.delete("old") + const driver = { + scrollToIndex() { assert.fail("cancelled seek must not scroll") }, + scrollToOffset() { assert.fail("cancelled seek must not scroll") }, + } + nav.move(1) + assert.equal(nav.seek(driver, 20), false) + assert.equal(nav.target, undefined) + nav.move(1) + nav.manual() + assert.equal(nav.seek(driver, 1), false) +}) + +test("delayed native offsets cannot discard the latest rapid-tap boundary", () => { + const nav = new MessageNavigation() + nav.sync(["reply3", "reply2", "reply1", "user3", "user2", "user1"]) + nav.observe(0, 200) + nav.ids.forEach((id, index) => nav.frames.set(id, { y: index * 600, height: 600 })) + const driver = { scrollToOffset() {}, scrollToIndex() {} } + assert.equal(nav.move(1), 1) + nav.seek(driver, 0) + assert.equal(nav.move(1), 2) + nav.seek(driver, 0) + // The callback for tap 1 arrives after tap 2 has issued its native scroll. + nav.observe(1000, 200) + assert.equal(nav.move(1), 3) + assert.equal(nav.target, "user3") +}) + +test("Latest resets the logical viewport before its native callback arrives", () => { + const nav = list() + nav.frames.set("new", { y: 0, height: 600 }) + nav.frames.set("long", { y: 600, height: 1000 }) + nav.frames.set("old", { y: 1600, height: 100 }) + nav.observe(1500, 200) + nav.manual(0) + nav.observe(1500, 200) // an older scroll event still queued during Latest + assert.equal(nav.offset, 0) + assert.equal(nav.move(1), 1) + assert.equal(nav.target, "long") +}) + +test("acknowledged navigation resumes scroll reanchoring and manual cancellation", () => { + const nav = list() + nav.move(1) + nav.complete(1000) + nav.observe(1000, 200) + nav.observe(400, 200) + assert.equal(nav.current(), 1) + nav.move(-1) + nav.complete(0) + nav.manual() + nav.observe(400, 200) + assert.equal(nav.current(), 1) + assert.equal(nav.expected, undefined) +}) diff --git a/src/lib/message-navigation.ts b/src/lib/message-navigation.ts new file mode 100644 index 00000000..02e87645 --- /dev/null +++ b/src/lib/message-navigation.ts @@ -0,0 +1,112 @@ +// Inverted lists measure from the newest end; a cell's logical end is its +// visual top. Keep targets by ID because streaming can change list indices. +export class MessageNavigation { + ids: string[] = [] + frames = new Map() + offset = 0 + height = 0 + expected: number | undefined + target: string | undefined + anchor: { id: string; offset: number; height: number } | undefined + + sync(ids: string[]) { + this.ids = ids + if (this.target && !ids.includes(this.target)) this.target = undefined + if (this.anchor && !ids.includes(this.anchor.id)) this.anchor = undefined + for (const id of this.frames.keys()) { + if (!ids.includes(id)) this.frames.delete(id) + } + } + + current() { + if (this.target) return this.ids.indexOf(this.target) + if (this.anchor) return this.ids.indexOf(this.anchor.id) + return this.visible() + } + + visible() { + const top = this.offset + this.height + // Include exact starts, but not the end of the older adjacent cell. + const visible = this.ids.findIndex((id) => { + const frame = this.frames.get(id) + return frame && frame.height > 0 && frame.y < top && frame.y + frame.height >= top - 1 + }) + if (visible >= 0) return visible + const nearest = this.ids.map((id, index) => ({ index, frame: this.frames.get(id) })) + .filter((item) => item.frame && item.frame.height > 0 && item.frame.y < top) + .sort((a, b) => b.frame!.y - a.frame!.y)[0] + return nearest?.index ?? -1 + } + + move(direction: 1 | -1) { + const current = this.current() + if (current < 0) return undefined + const index = current + direction + if (index < 0 || index >= this.ids.length) return undefined + this.target = this.ids[index] + return index + } + + destination() { + if (!this.target) return undefined + const frame = this.frames.get(this.target) + if (!frame || frame.height <= 0 || this.height <= 0) return undefined + return Math.max(0, frame.y + frame.height - this.height) + } + + manual(offset?: number) { + this.target = undefined + this.anchor = undefined + this.expected = offset + if (offset !== undefined) this.offset = offset + } + + observe(offset: number, height: number) { + if (this.height !== height) this.expected = undefined + // Earlier programmatic scroll callbacks can arrive after a newer tap. + // Keep its boundary until native scrolling acknowledges the latest command. + if (this.expected !== undefined && Math.abs(this.expected - offset) > 1) return + this.expected = undefined + this.offset = offset + this.height = height + if (this.anchor && (Math.abs(this.anchor.offset - offset) > 1 || this.anchor.height !== height)) { + this.anchor = undefined + } + } + + complete(offset: number) { + if (this.target) this.anchor = { id: this.target, offset, height: this.height } + this.expected = offset + this.offset = offset + this.target = undefined + } + + seek(list: { + scrollToOffset(params: { offset: number; animated: boolean }): void + scrollToIndex(params: { index: number; viewPosition: number; animated: boolean }): void + }, attempt: number) { + if (!this.target) return false + const index = this.ids.indexOf(this.target) + if (index < 0) return false + const offset = this.destination() + if (offset !== undefined) { + list.scrollToOffset({ offset, animated: false }) + this.complete(offset) + return false + } + if (attempt >= 20) { + this.manual() + return false + } + if (attempt === 0) { + list.scrollToIndex({ index, viewPosition: 1, animated: false }) + return true + } + // RN may accept estimated metrics without mounting the target. Advance + // the render window until a real target frame arrives, even without failure. + const visible = this.visible() + const direction = visible < 0 || index > visible ? 1 : -1 + list.scrollToOffset({ offset: Math.max(0, this.offset + direction * this.height * 0.8), animated: false }) + return true + } +}