From 770648327f7744e1d76ba81c37ebbb0700782ac1 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 10 Sep 2026 14:00:09 +0200 Subject: [PATCH] fix(app): finish desktop content search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reopening the same occurrence left the pane where the reader had scrolled it. A file target is identity-stable, so the second activation reused the tab and looked like a no-op; the existing openers avoid this by asking for navigation after opening, which the search callback had assembled around. The per-tab navigation revision now lives with the tabs in the layout store, one open-a-workspace-file command carries the whole recipe, and the explorer, filename search and content search all go through it. The editor instance, its dirty buffer and its history are untouched. Result rows now read as three things rather than one. The match is foreground on the highlight, the rest of the matched line is its context, and the location is context for that. Rows are 44px instead of 56px, so more occurrences fit without shrinking the text. Row identity was the worst of it: a flat occurrence list is full of repeated base names, and an end-truncated path threw away exactly the parents that tell them apart, so four different index.tsx rows looked identical. The row keeps the file name and its closest parent, marks the elided head, and pins the name so only the parent may shrink. The desktop preview drops its header. The selected row names the file and Enter opens it, so a repeated title and an Open button were duplicate chrome. The compact flow pages away from the list and keeps both. The daemon end-to-end assertion expected an empty result without the per-file ceiling the host reports, so it failed before reaching its file-read step. Fixtures that can only exist on POSIX — a file named with a backslash, a PATH built from which and symlinks — are skipped on Windows, and toWorkspaceRelativePath gets separator coverage that runs everywhere. Removes the native row-geometry estimator added earlier in this branch. It predicted platform text layout from character counts and never agreed with what Android drew. Native source returns to the behaviour it had before. --- ...orkspace-content-search-dependency.spec.ts | 4 + .../browser/workspace-content-search.spec.ts | 15 +++ .../helpers/workspace-content-search.ts | 51 ++++++++ .../workspace-content-search/index.tsx | 87 +++++++++----- .../internal/result-path.test.ts | 30 +++++ .../internal/result-path.ts | 25 ++++ .../command-center/workspace-file-search.ts | 7 +- .../compact-explorer-sidebar-host.tsx | 17 ++- .../src/file-pane/source/row-layout.test.ts | 63 ---------- .../app/src/file-pane/source/row-layout.ts | 55 --------- .../app/src/file-pane/source/view.native.tsx | 109 ++++-------------- .../workspace-file-open-command.test.ts | 31 ++++- .../workspace/workspace-file-open-command.ts | 23 +++- .../screens/workspace/workspace-screen.tsx | 18 ++- .../app/src/stores/workspace-layout-store.ts | 18 +++ .../workspace-content-search.e2e.test.ts | 3 + packages/server/src/server/path-utils.test.ts | 18 +++ .../workspace-content-search/index.test.ts | 19 ++- 18 files changed, 322 insertions(+), 271 deletions(-) create mode 100644 packages/app/src/command-center/workspace-content-search/internal/result-path.test.ts create mode 100644 packages/app/src/command-center/workspace-content-search/internal/result-path.ts delete mode 100644 packages/app/src/file-pane/source/row-layout.test.ts delete mode 100644 packages/app/src/file-pane/source/row-layout.ts create mode 100644 packages/server/src/server/path-utils.test.ts diff --git a/packages/app/e2e/browser/workspace-content-search-dependency.spec.ts b/packages/app/e2e/browser/workspace-content-search-dependency.spec.ts index cad874c72e..c52e7ef017 100644 --- a/packages/app/e2e/browser/workspace-content-search-dependency.spec.ts +++ b/packages/app/e2e/browser/workspace-content-search-dependency.spec.ts @@ -5,6 +5,10 @@ import path from "node:path"; import { test as base, expect } from "../support/fixtures"; import { prepareContentSearch } from "../support/helpers/workspace-content-search"; +// The missing-rg fixture builds a PATH from `which` and POSIX symlinks. Windows needs its own +// PATH construction, which belongs to the CI gate rather than to this journey. +base.skip(process.platform === "win32", "the missing-ripgrep PATH fixture is POSIX-shaped"); + const test = base.extend<{}, { searchHostPath: string }>({ searchHostPath: [ async ({ browserName }, provide) => { diff --git a/packages/app/e2e/browser/workspace-content-search.spec.ts b/packages/app/e2e/browser/workspace-content-search.spec.ts index 4fbe9f76e3..02c73b7b74 100644 --- a/packages/app/e2e/browser/workspace-content-search.spec.ts +++ b/packages/app/e2e/browser/workspace-content-search.spec.ts @@ -144,9 +144,24 @@ test("an empty result says which files the host never opened", async ({ page, wi await reportSkippedLargeFiles(page, await withWorkspace()); }); +// "a\b.txt" is one file on POSIX and a nested path on Windows, so the fixture only exists here. +// Windows separator handling is covered by resolveWorkspaceFilePaths and toWorkspaceRelativePath. +test.skip( + process.platform === "win32", + "a file name containing a backslash is not a valid Windows fixture", +); test("a file name containing a backslash opens itself, not a same-named nested path", async ({ page, withWorkspace, }) => { await openLiteralBackslashFile(page, await withWorkspace()); }); + +import { reopenSameOccurrenceAfterMovingAway } from "../support/helpers/workspace-content-search"; + +test("opening the same occurrence again navigates the pane it is already showing", async ({ + page, + withWorkspace, +}) => { + await reopenSameOccurrenceAfterMovingAway(page, await withWorkspace()); +}); diff --git a/packages/app/e2e/support/helpers/workspace-content-search.ts b/packages/app/e2e/support/helpers/workspace-content-search.ts index e3671bfd62..a0d4c3131d 100644 --- a/packages/app/e2e/support/helpers/workspace-content-search.ts +++ b/packages/app/e2e/support/helpers/workspace-content-search.ts @@ -190,3 +190,54 @@ export async function openLiteralBackslashFile(page: Page, workspace: CreatedWor await expect(source).not.toContainText("THIS IS THE OTHER FILE"); await expect(page.getByRole("status").filter({ hasText: "File changed" })).toHaveCount(0); } + +/** + * Opening the same occurrence twice has to navigate the pane both times. The tab target is + * deliberately identity-stable, so the second activation looks like a no-op unless it asks the + * file-opening owner to navigate again. + */ +export async function reopenSameOccurrenceAfterMovingAway(page: Page, workspace: CreatedWorkspace) { + const lines = Array.from( + { length: 1200 }, + (_value, index) => `export const filler${index} = ${index};`, + ); + lines.push('const marker = "REOPEN_TARGET";'); + await writeFile(path.join(workspace.repoPath, "reopen.ts"), `${lines.join("\n")}\n`); + await workspace.navigateTo(); + + const scroller = page.getByTestId("file-source-editor").locator(".cm-scroller"); + const offset = () => scroller.evaluate((element) => element.scrollTop); + + await openReopenTarget(page); + const revealed = await offset(); + expect(revealed).toBeGreaterThan(0); + await expect( + page.getByTestId("file-source-editor").locator(".cm-selectionBackground"), + ).toBeInViewport(); + + // Move the caret to the top of the same file, inside the same editor instance. + await page.getByTestId("file-source-editor").locator(".cm-content").click(); + await page.keyboard.press("ControlOrMeta+Home"); + await expect.poll(offset).toBeLessThan(revealed / 2); + + await openReopenTarget(page); + await expect + .poll(offset, { message: "reopening the same occurrence must navigate the pane again" }) + .toBeGreaterThan(revealed / 2); + await expect( + page.getByTestId("file-source-editor").locator(".cm-selectionBackground"), + ).toBeInViewport(); + // The editor instance is reused, so its history survives the second navigation. + await expect(page.getByTestId("file-source-editor")).toContainText("REOPEN_TARGET"); +} + +async function openReopenTarget(page: Page) { + await page.keyboard.press("Meta+Shift+F"); + const panel = page.getByTestId("command-center-panel"); + await page + .getByRole("textbox", { name: "Search saved file contents...", exact: true }) + .fill("REOPEN_TARGET"); + await expect(panel.getByRole("button", { name: /reopen\.ts:1201:/ })).toBeVisible(); + await page.keyboard.press("Enter"); + await expect(panel).toBeHidden(); +} diff --git a/packages/app/src/command-center/workspace-content-search/index.tsx b/packages/app/src/command-center/workspace-content-search/index.tsx index 554fb6596c..c294532772 100644 --- a/packages/app/src/command-center/workspace-content-search/index.tsx +++ b/packages/app/src/command-center/workspace-content-search/index.tsx @@ -11,12 +11,12 @@ import { useWorkspaceDirectory } from "@/stores/session-store-hooks"; import { useSessionStore } from "@/stores/session-store"; import { useWorkspaceLayoutStore } from "@/stores/workspace-layout-store"; import { usePanelStore } from "@/stores/panel-store"; -import { createWorkspaceFileTabTarget } from "@/workspace/file-open"; import { buildWorkspaceTabPersistenceKey } from "@/workspace-tabs/model"; +import { openWorkspaceFileInFocusedPane } from "@/screens/workspace/workspace-file-open-command"; import { clearCommandCenterFocusRestoreElement } from "@/utils/command-center-focus-restore"; import { WorkspaceFileSource } from "@/file-pane/source"; import { formatFileSize } from "@/utils/format-file-size"; -import { describeWorkspaceFilePath } from "../workspace-file-search-model"; +import { describeResultPath } from "./internal/result-path"; import { WorkspaceContentSearchModel, type ContentSearchSnapshot, @@ -104,16 +104,17 @@ function ContentSearch({ transport, open(location) { clearCommandCenterFocusRestoreElement(); - const workspaceKey = buildWorkspaceTabPersistenceKey({ serverId, workspaceId }); - if (!workspaceKey) return; const layout = useWorkspaceLayoutStore.getState(); - const tabId = layout.openTab({ - workspaceKey, - target: createWorkspaceFileTabTarget(location), - intent: "reveal", + openWorkspaceFileInFocusedPane({ + location, + persistenceKey: buildWorkspaceTabPersistenceKey({ serverId, workspaceId }), + closeExplorerAfterOpen: true, + showMobileAgent: usePanelStore.getState().showMobileAgent, + openWorkspaceTabInFocusedPane: (workspaceKey, target, placement) => + layout.openTab({ workspaceKey, target, intent: "reveal", placement }), + focusWorkspaceTab: layout.focusTab, + requestFileNavigation: layout.requestFileNavigation, }); - if (tabId) layout.focusTab(workspaceKey, tabId); - usePanelStore.getState().showMobileAgent(); closeRef.current(); }, }), @@ -230,7 +231,7 @@ function ResultRow({ [selected], ); const line = useMemo(() => describeMatchLine(item), [item]); - const file = useMemo(() => describeWorkspaceFilePath(item.path), [item.path]); + const file = useMemo(() => describeResultPath(item.path), [item.path]); return ( {line.match} {line.after} - - + {/* The file name and its coordinates are pinned; only the parent context may shrink, so a + deep path loses its middle rather than the identity the reader is scanning for. */} + + {file.directory ? ( + + {file.directory} + + ) : null} + {file.name}:{item.line}:{item.columnStart} - {file.directory ? {file.directory} : null} - + ); } @@ -346,13 +353,13 @@ function ContentPreview({ ); return ( - {match ? ( + {/* On desktop the selected row already names the file and Enter opens it, so the preview + carries no header. The compact flow pages away from the list and still needs both. */} + {compact && match ? ( - {compact ? ( - - ) : null} + {match.path} @@ -391,8 +398,9 @@ function ContentPreview({ ); } -// Matches the Command Center's own two-line result row. -const RESULT_ROW_HEIGHT = 56; +// Denser than the Command Center's own two-line row: these results are scanned, not read, and a +// search wants as many occurrences on screen as stay legible. +const RESULT_ROW_HEIGHT = 44; // Characters of the matched line kept ahead of the match itself. const SNIPPET_LEAD = 24; @@ -413,22 +421,37 @@ const styles = StyleSheet.create((theme) => ({ height: RESULT_ROW_HEIGHT, justifyContent: "center", paddingHorizontal: theme.spacing[4], - paddingVertical: theme.spacing[2], + paddingVertical: theme.spacing[1], }, hovered: { backgroundColor: theme.colors.surface1 }, selected: { backgroundColor: theme.colors.surface2 }, - // The matched line leads, in the code type scale; the location is its subtitle. + // Three levels: the match is what the reader is looking for, the rest of the line is the + // context it sits in, and the location is context for that. snippet: { - color: theme.colors.foreground, + color: theme.colors.foregroundMuted, fontSize: theme.fontSize.code, - lineHeight: 18, + lineHeight: 17, fontFamily: theme.fontFamily.mono, }, - match: { backgroundColor: theme.colors.terminal.selectionBackground }, - // The file name and coordinates lead so that truncation eats the directory, not the identity. - location: { fontSize: theme.fontSize.sm, lineHeight: 16 }, - locationFile: { color: theme.colors.foreground }, - locationDirectory: { color: theme.colors.foregroundMuted }, + match: { + color: theme.colors.foreground, + fontWeight: "600", + backgroundColor: theme.colors.terminal.selectionBackground, + }, + location: { flexDirection: "row", alignItems: "baseline" }, + locationFile: { + flexShrink: 0, + color: theme.colors.foregroundMuted, + fontSize: theme.fontSize.sm, + lineHeight: 15, + }, + locationDirectory: { + flexShrink: 1, + minWidth: 0, + color: theme.colors.foregroundExtraMuted, + fontSize: theme.fontSize.sm, + lineHeight: 15, + }, preview: { flex: 1, minWidth: 0, minHeight: 0 }, previewHeader: { flexDirection: "row", diff --git a/packages/app/src/command-center/workspace-content-search/internal/result-path.test.ts b/packages/app/src/command-center/workspace-content-search/internal/result-path.test.ts new file mode 100644 index 0000000000..f13261230d --- /dev/null +++ b/packages/app/src/command-center/workspace-content-search/internal/result-path.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it } from "vitest"; +import { describeResultPath } from "./result-path"; + +describe("describeResultPath", () => { + it("keeps a root file whole", () => { + expect(describeResultPath("README.md")).toEqual({ directory: "", name: "README.md" }); + }); + + it("keeps a shallow path whole", () => { + expect(describeResultPath("docs/architecture.md")).toEqual({ + directory: "docs/", + name: "architecture.md", + }); + }); + + it("elides the head of a deep path and keeps the distinguishing parent", () => { + // These two rows are both "index.tsx"; the parents are the only thing telling them apart. + expect( + describeResultPath("packages/app/src/command-center/workspace-content-search/index.tsx"), + ).toEqual({ directory: "…/workspace-content-search/", name: "index.tsx" }); + expect(describeResultPath("packages/app/src/file-pane/source/index.tsx")).toEqual({ + directory: "…/source/", + name: "index.tsx", + }); + }); + + it("keeps a literal backslash in a file name out of the directory", () => { + expect(describeResultPath("a\\b.txt")).toEqual({ directory: "", name: "a\\b.txt" }); + }); +}); diff --git a/packages/app/src/command-center/workspace-content-search/internal/result-path.ts b/packages/app/src/command-center/workspace-content-search/internal/result-path.ts new file mode 100644 index 0000000000..c7cb8cef8e --- /dev/null +++ b/packages/app/src/command-center/workspace-content-search/internal/result-path.ts @@ -0,0 +1,25 @@ +/** + * How a result row names its file. + * + * A flat list of occurrences is full of repeated base names — four `index.tsx` rows tell the + * reader nothing. The nearest directories are what distinguish them, and they are also what an + * end-truncated path throws away first. So the row keeps the file name and the closest parents, + * and marks the elided head with a leading ellipsis. + */ +export interface ResultPathParts { + /** Directory context immediately above the file, already elided at the head when deeper. */ + directory: string; + /** File name, which is never truncated away. */ + name: string; +} + +const PARENTS_KEPT = 1; + +export function describeResultPath(path: string): ResultPathParts { + const segments = path.split("/"); + const name = segments.pop() ?? path; + if (segments.length === 0) return { directory: "", name }; + const kept = segments.slice(-PARENTS_KEPT); + const elided = segments.length > kept.length; + return { directory: `${elided ? "…/" : ""}${kept.join("/")}/`, name }; +} diff --git a/packages/app/src/command-center/workspace-file-search.ts b/packages/app/src/command-center/workspace-file-search.ts index 1008604bec..2971b611ec 100644 --- a/packages/app/src/command-center/workspace-file-search.ts +++ b/packages/app/src/command-center/workspace-file-search.ts @@ -1,5 +1,5 @@ import { useCallback, useEffect, useMemo, useState } from "react"; -import { openWorkspaceFileFromExplorer } from "@/screens/workspace/workspace-file-open-command"; +import { openWorkspaceFileInFocusedPane } from "@/screens/workspace/workspace-file-open-command"; import { useActiveWorkspaceSelection } from "@/stores/navigation-active-workspace-store"; import { usePanelStore } from "@/stores/panel-store"; import { useSessionStore } from "@/stores/session-store"; @@ -128,8 +128,8 @@ export function useWorkspaceFileSearch(input: { enabled: boolean; query: string (path: string) => { if (!serverId || !workspaceId) return; clearCommandCenterFocusRestoreElement(); - openWorkspaceFileFromExplorer({ - filePath: path, + openWorkspaceFileInFocusedPane({ + location: { path }, persistenceKey: buildWorkspaceTabPersistenceKey({ serverId, workspaceId }), closeExplorerAfterOpen: true, showMobileAgent: usePanelStore.getState().showMobileAgent, @@ -141,6 +141,7 @@ export function useWorkspaceFileSearch(input: { enabled: boolean; query: string placement, }), focusWorkspaceTab: useWorkspaceLayoutStore.getState().focusTab, + requestFileNavigation: useWorkspaceLayoutStore.getState().requestFileNavigation, }); }, [serverId, workspaceId], diff --git a/packages/app/src/components/compact-explorer-sidebar-host.tsx b/packages/app/src/components/compact-explorer-sidebar-host.tsx index 2dae4b9042..6018b89888 100644 --- a/packages/app/src/components/compact-explorer-sidebar-host.tsx +++ b/packages/app/src/components/compact-explorer-sidebar-host.tsx @@ -14,7 +14,7 @@ import { usePanelStore } from "@/stores/panel-store"; import { useWorkspaceLayoutStore } from "@/stores/workspace-layout-store"; import type { WorkspaceTabTarget } from "@/workspace-tabs/model"; import { useWorkspaceCheckoutStatus } from "@/screens/workspace/use-workspace-checkout-status"; -import { openWorkspaceFileFromExplorer } from "@/screens/workspace/workspace-file-open-command"; +import { openWorkspaceFileInFocusedPane } from "@/screens/workspace/workspace-file-open-command"; import { isWeb } from "@/constants/platform"; import { DiffDocumentWorkspaceCacheProvider } from "@/git/diff-document/workspace-cache"; import { @@ -119,6 +119,7 @@ export function CompactExplorerSidebarHost({ [openTab], ); const focusWorkspaceTab = useWorkspaceLayoutStore((state) => state.focusTab); + const requestFileNavigation = useWorkspaceLayoutStore((state) => state.requestFileNavigation); const handleOpenExplorer = useCallback(() => { if (!model?.workspaceRoot) { @@ -136,16 +137,24 @@ export function CompactExplorerSidebarHost({ if (!model) { return; } - openWorkspaceFileFromExplorer({ - filePath, + openWorkspaceFileInFocusedPane({ + location: { path: filePath }, persistenceKey: model.persistenceKey, closeExplorerAfterOpen: presentation === "overlay", showMobileAgent, openWorkspaceTabInFocusedPane, focusWorkspaceTab, + requestFileNavigation, }); }, - [focusWorkspaceTab, model, openWorkspaceTabInFocusedPane, presentation, showMobileAgent], + [ + focusWorkspaceTab, + model, + openWorkspaceTabInFocusedPane, + presentation, + requestFileNavigation, + showMobileAgent, + ], ); const handleContainerLayout = useCallback((event: LayoutChangeEvent) => { diff --git a/packages/app/src/file-pane/source/row-layout.test.ts b/packages/app/src/file-pane/source/row-layout.test.ts deleted file mode 100644 index e1b12ddb1f..0000000000 --- a/packages/app/src/file-pane/source/row-layout.test.ts +++ /dev/null @@ -1,63 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { sourceRowOffsets, visualLineCount } from "./row-layout"; - -// Measured on a Pixel-class Android device at the default code size: one visual line is 17.5234dp -// after the device rounds it, and 49 monospace characters fit beside the gutter. -const VISUAL_LINE_HEIGHT = 17.5234375; -const COLUMNS = 49; - -// An ordinary TypeScript line, 119 characters, of the kind that fills a real source file. -const ORDINARY_LINE = - "export function handlerNumber1(input: Record, options: { retries: number }): Promise { /* 1 */ }"; - -describe("visualLineCount", () => { - it("keeps a line that fits on one visual line", () => { - expect(visualLineCount("const a = 1;", COLUMNS)).toBe(1); - expect(visualLineCount("", COLUMNS)).toBe(1); - }); - - it("counts the visual lines an ordinary source line occupies", () => { - expect(visualLineCount(ORDINARY_LINE, COLUMNS)).toBe(3); - }); - - it("breaks early at a space, where dividing by the column count would not", () => { - // A larger code size narrows the columns. Character division says four lines; the platform - // cannot split "handlerNumber1(input:" across a space that is not there, so it needs five. - expect(Math.ceil(ORDINARY_LINE.length / 33)).toBe(4); - expect(visualLineCount(ORDINARY_LINE, 33)).toBe(5); - }); - - it("breaks a single word that is wider than the line", () => { - expect(visualLineCount("x".repeat(COLUMNS * 3), COLUMNS)).toBe(3); - }); - - it("assumes one visual line until the column width is measured", () => { - expect(visualLineCount(ORDINARY_LINE, 0)).toBe(1); - }); -}); - -describe("sourceRowOffsets", () => { - it("stacks rows by the visual lines they really occupy", () => { - const offsets = sourceRowOffsets([{ text: "const a = 1;" }, { text: ORDINARY_LINE }], { - columns: COLUMNS, - visualLineHeight: VISUAL_LINE_HEIGHT, - }); - expect(Array.from(offsets)).toEqual([0, VISUAL_LINE_HEIGHT, VISUAL_LINE_HEIGHT * 4]); - }); - - it("puts a match after a thousand ordinary lines where it is really drawn", () => { - // The regression: 1,200 ordinary lines, match on 1,201. Claiming one visual line per row - // reported 1200 * 17.52 = 21,028, which is about line 1,100 on screen — the match was roughly - // 90 lines below the fold and never revealed. - const lines = [ - ...Array.from({ length: 1200 }, () => ({ text: ORDINARY_LINE })), - { text: 'const target = "needle";' }, - ]; - const offsets = sourceRowOffsets(lines, { - columns: COLUMNS, - visualLineHeight: VISUAL_LINE_HEIGHT, - }); - expect(offsets[1200]).toBeCloseTo(1200 * 3 * VISUAL_LINE_HEIGHT, 5); - expect(offsets[1200]).toBeGreaterThan(1200 * VISUAL_LINE_HEIGHT * 2.9); - }); -}); diff --git a/packages/app/src/file-pane/source/row-layout.ts b/packages/app/src/file-pane/source/row-layout.ts deleted file mode 100644 index 9e3294b5e9..0000000000 --- a/packages/app/src/file-pane/source/row-layout.ts +++ /dev/null @@ -1,55 +0,0 @@ -/** - * Where each source row sits in the list. - * - * A row is one visual line of code tall, and a line too long for the available columns wraps onto - * more of them. The virtualized list is told these offsets so it can jump straight to a match - * thousands of lines in; if they claim one visual line per row, an ordinary source file — whose - * lines routinely wrap on a phone — puts the match hundreds of lines off screen. - */ -export interface SourceRowGeometry { - /** Monospace characters that fit beside the gutter; 0 before the first measurement. */ - columns: number; - /** Height of one visual line, already rounded the way the device rounds it. */ - visualLineHeight: number; -} - -/** - * Visual lines a line of code occupies, wrapped the way the platform wraps it: greedily at spaces, - * and mid-word only for a word too long to fit a line of its own. Dividing the character count by - * the column count instead under-counts every line that has to break early at a space, and the - * error compounds over a long file. One line until the column width has been measured. - */ -export function visualLineCount(text: string, columns: number): number { - if (columns <= 0) return 1; - let rows = 1; - let used = 0; - for (const word of text.split(" ")) { - const gap = used === 0 ? 0 : 1; - if (used + gap + word.length <= columns) { - used += gap + word.length; - continue; - } - if (used > 0) rows += 1; - if (word.length > columns) { - rows += Math.ceil(word.length / columns) - 1; - used = word.length % columns || columns; - continue; - } - used = word.length; - } - return rows; -} - -/** Cumulative top edge of every row, with one extra entry for the end of the last row. */ -export function sourceRowOffsets( - lines: ReadonlyArray<{ text: string }>, - geometry: SourceRowGeometry, -): Float64Array { - const stops = new Float64Array(lines.length + 1); - for (let index = 0; index < lines.length; index += 1) { - stops[index + 1] = - stops[index] + - visualLineCount(lines[index].text, geometry.columns) * geometry.visualLineHeight; - } - return stops; -} diff --git a/packages/app/src/file-pane/source/view.native.tsx b/packages/app/src/file-pane/source/view.native.tsx index 571cc195d7..3ef40c93d6 100644 --- a/packages/app/src/file-pane/source/view.native.tsx +++ b/packages/app/src/file-pane/source/view.native.tsx @@ -1,13 +1,12 @@ import { useTranslation } from "react-i18next"; -import { useCallback, useEffect, useMemo, useRef, useState } from "react"; -import { FlatList, PixelRatio, Text, View, type LayoutChangeEvent } from "react-native"; +import { useCallback, useEffect, useMemo, useRef } from "react"; +import { FlatList, Text, View } from "react-native"; import { StyleSheet } from "react-native-unistyles"; import { highlightCode, type HighlightToken } from "@getpaseo/highlight"; import { syntaxTokenStyleFor } from "@/styles/syntax-token-styles"; import type { WorkspaceFileLocation } from "@/workspace/file-open"; import type { EditorVisualTheme } from "../editor/extensions.web"; import { selectSourcePresentation } from "./presentation"; -import { sourceRowOffsets } from "./row-layout"; interface FileSourceViewProps { content: string; @@ -22,8 +21,6 @@ interface FileSourceViewProps { interface SourceLine { number: number; tokens: HighlightToken[]; - /** The line's own text, which decides how many visual lines its row occupies. */ - text: string; } export function FileSourceView({ @@ -32,7 +29,6 @@ export function FileSourceView({ location, navigationRevision, size, - theme, tooLargeMessage, }: FileSourceViewProps) { const presentation = selectSourcePresentation({ size, platform: "native" }); @@ -50,7 +46,6 @@ export function FileSourceView({ location={location} navigationRevision={navigationRevision} presentation={presentation} - codeFontSize={theme.codeFontSize} /> ); } @@ -61,10 +56,8 @@ function VirtualizedSource({ location, navigationRevision, presentation, - codeFontSize, }: Omit & { presentation: "highlighted" | "plain"; - codeFontSize: number; }) { const { t } = useTranslation(); const listRef = useRef>(null); @@ -74,57 +67,19 @@ function VirtualizedSource({ return highlightCode(source, filename).map((tokens, index) => ({ number: index + 1, tokens, - text: tokens.map((token) => token.text).join(""), })); - return source.split("\n").map((text, index) => ({ - number: index + 1, - tokens: [{ text, style: null }], - text, - })); + return source + .split("\n") + .map((text, index) => ({ number: index + 1, tokens: [{ text, style: null }] })); }, [content, filename, presentation]); - // Jumping to a match thousands of lines in only works if the list is told the truth about row - // heights. A row is one visual line at the reader's own code size, rounded the way the device - // rounds it, times however many visual lines the text wraps onto. Assuming one line per row put - // a match in an ordinary source file hundreds of lines off screen, because ordinary lines wrap. - const [columnWidth, setColumnWidth] = useState(0); - const [listWidth, setListWidth] = useState(0); - const visualLineHeight = PixelRatio.roundToNearestPixel(sourceRowHeight(codeFontSize)); - const columns = - columnWidth > 0 && listWidth > 0 - ? Math.max(1, Math.floor((listWidth - GUTTER_WIDTH) / columnWidth)) - : 0; - const offsets = useMemo( - () => sourceRowOffsets(lines, { columns, visualLineHeight }), - [columns, lines, visualLineHeight], - ); - const itemLayout = useCallback( - (_data: ArrayLike | null | undefined, index: number) => ({ - length: offsets[index + 1] - offsets[index], - offset: offsets[index], - index, - }), - [offsets], - ); - const measureColumn = useCallback( - (event: LayoutChangeEvent) => - setColumnWidth(event.nativeEvent.layout.width / COLUMN_PROBE.length), - [], - ); - const measureList = useCallback( - (event: LayoutChangeEvent) => setListWidth(event.nativeEvent.layout.width), - [], - ); useEffect(() => { if (!location.lineStart) return; listRef.current?.scrollToIndex({ index: Math.min(location.lineStart - 1, lines.length - 1), animated: false, - // Near the top with a little context above, rather than centred: centring is measured - // against the list's own box, which inside a sheet is taller than the part on screen. - viewPosition: 0, - viewOffset: visualLineHeight * CONTEXT_LINES_ABOVE_MATCH, + viewPosition: 0.5, }); - }, [lines.length, location.lineStart, navigationRevision, offsets, visualLineHeight]); + }, [lines.length, location.lineStart, navigationRevision]); const selectedLine = lines[(location.lineStart ?? 1) - 1]?.tokens .map((token) => token.text) .join(""); @@ -143,27 +98,22 @@ function VirtualizedSource({ [changed, location], ); return ( - - - {COLUMN_PROBE} - + {changed ? ( {t("shell.commandCenter.contentChanged")} ) : null} - {columns > 0 ? ( - - ) : null} + ); } @@ -196,17 +146,10 @@ function sourceLineKey(line: SourceLine): string { return String(line.number); } -/** One visual line of code; the stylesheet below builds its row from the same ratio. */ -function sourceRowHeight(codeFontSize: number): number { - return codeFontSize * SOURCE_LINE_RATIO; +function sourceLineLayout(_data: ArrayLike | null | undefined, index: number) { + return { length: 20, offset: index * 20, index }; } -const SOURCE_LINE_RATIO = 1.45; -const GUTTER_WIDTH = 56; -/** Measured once to learn how wide one monospace character is at the reader's code size. */ -const COLUMN_PROBE = "0".repeat(10); -const CONTEXT_LINES_ABOVE_MATCH = 2; - const styles = StyleSheet.create((theme) => ({ root: { flex: 1 }, notice: { @@ -222,25 +165,19 @@ const styles = StyleSheet.create((theme) => ({ padding: theme.spacing[4], }, unsupportedText: { color: theme.colors.foregroundMuted, textAlign: "center" }, - line: { flexDirection: "row", minHeight: sourceRowHeight(theme.fontSize.code) }, + line: { flexDirection: "row", minHeight: theme.fontSize.code * 1.45 }, gutter: { - width: GUTTER_WIDTH, + width: 56, paddingRight: theme.spacing[3], textAlign: "right", color: theme.colors.foregroundMuted, fontFamily: theme.fontFamily.mono, fontSize: theme.fontSize.code, }, - columnProbe: { - position: "absolute", - opacity: 0, - fontFamily: theme.fontFamily.mono, - fontSize: theme.fontSize.code, - }, text: { flex: 1, fontFamily: theme.fontFamily.mono, fontSize: theme.fontSize.code, - lineHeight: sourceRowHeight(theme.fontSize.code), + lineHeight: theme.fontSize.code * 1.45, }, })); diff --git a/packages/app/src/screens/workspace/workspace-file-open-command.test.ts b/packages/app/src/screens/workspace/workspace-file-open-command.test.ts index d1551c7a8c..90cc160865 100644 --- a/packages/app/src/screens/workspace/workspace-file-open-command.test.ts +++ b/packages/app/src/screens/workspace/workspace-file-open-command.test.ts @@ -1,22 +1,24 @@ import { describe, expect, it, vi } from "vitest"; -import { openWorkspaceFileFromExplorer } from "@/screens/workspace/workspace-file-open-command"; +import { openWorkspaceFileInFocusedPane } from "@/screens/workspace/workspace-file-open-command"; +import type { WorkspaceFileLocation } from "@/workspace/file-open"; function createInput(closeExplorerAfterOpen: boolean) { return { - filePath: "src/app.tsx", + location: { path: "src/app.tsx" } as WorkspaceFileLocation, persistenceKey: "server:workspace", closeExplorerAfterOpen, showMobileAgent: vi.fn(), openWorkspaceTabInFocusedPane: vi.fn(() => "file-tab"), focusWorkspaceTab: vi.fn(), + requestFileNavigation: vi.fn(), }; } -describe("openWorkspaceFileFromExplorer", () => { +describe("openWorkspaceFileInFocusedPane", () => { it("closes the phone overlay after opening a file", () => { const input = createInput(true); - openWorkspaceFileFromExplorer(input); + openWorkspaceFileInFocusedPane(input); expect(input.showMobileAgent).toHaveBeenCalledOnce(); }); @@ -24,10 +26,29 @@ describe("openWorkspaceFileFromExplorer", () => { it("keeps the tablet dock open after opening a file", () => { const input = createInput(false); - openWorkspaceFileFromExplorer(input); + openWorkspaceFileInFocusedPane(input); expect(input.showMobileAgent).not.toHaveBeenCalled(); expect(input.openWorkspaceTabInFocusedPane).toHaveBeenCalledOnce(); expect(input.focusWorkspaceTab).toHaveBeenCalledWith("server:workspace", "file-tab"); }); + + it("asks for navigation every time, so reopening the same occurrence recentres its pane", () => { + const input = createInput(false); + input.location = { path: "src/app.tsx", lineStart: 1201, columnStart: 17 }; + + openWorkspaceFileInFocusedPane(input); + openWorkspaceFileInFocusedPane(input); + + expect(input.requestFileNavigation).toHaveBeenCalledTimes(2); + expect(input.requestFileNavigation).toHaveBeenLastCalledWith("file-tab"); + }); + + it("does not navigate when no tab could be opened", () => { + const input = { ...createInput(false), openWorkspaceTabInFocusedPane: vi.fn(() => null) }; + + openWorkspaceFileInFocusedPane(input); + + expect(input.requestFileNavigation).not.toHaveBeenCalled(); + }); }); diff --git a/packages/app/src/screens/workspace/workspace-file-open-command.ts b/packages/app/src/screens/workspace/workspace-file-open-command.ts index 0d990f3557..b024fdf016 100644 --- a/packages/app/src/screens/workspace/workspace-file-open-command.ts +++ b/packages/app/src/screens/workspace/workspace-file-open-command.ts @@ -1,6 +1,7 @@ import { createWorkspaceFileTabTarget, normalizeWorkspaceFileLocation, + type WorkspaceFileLocation, } from "@/workspace/file-open"; import { FOCUSED_PANE_PLACEMENT, @@ -8,8 +9,8 @@ import { } from "@/stores/workspace-layout-store"; import type { WorkspaceTabTarget } from "@/workspace-tabs/model"; -interface OpenWorkspaceFileFromExplorerInput { - filePath: string; +interface OpenWorkspaceFileInput { + location: WorkspaceFileLocation; persistenceKey: string | null; closeExplorerAfterOpen: boolean; showMobileAgent: () => void; @@ -19,16 +20,24 @@ interface OpenWorkspaceFileFromExplorerInput { placement?: WorkspaceTabPlacement, ) => string | null; focusWorkspaceTab: (workspaceKey: string, tabId: string) => void; + requestFileNavigation: (tabId: string) => void; } -export function openWorkspaceFileFromExplorer(input: OpenWorkspaceFileFromExplorerInput): void { +/** + * Opens a workspace file in the focused pane and navigates to it. + * + * The navigation request is the part callers keep forgetting: a file target is identity-stable, so + * reopening the same path — or the same occurrence — reuses its tab and would otherwise leave the + * pane wherever the reader had scrolled it. Every explicit activation asks for a reveal. + */ +export function openWorkspaceFileInFocusedPane(input: OpenWorkspaceFileInput): void { if (input.closeExplorerAfterOpen) { input.showMobileAgent(); } if (!input.persistenceKey) { return; } - const location = normalizeWorkspaceFileLocation({ path: input.filePath }); + const location = normalizeWorkspaceFileLocation(input.location); if (!location) { return; } @@ -37,7 +46,9 @@ export function openWorkspaceFileFromExplorer(input: OpenWorkspaceFileFromExplor createWorkspaceFileTabTarget(location), FOCUSED_PANE_PLACEMENT, ); - if (tabId) { - input.focusWorkspaceTab(input.persistenceKey, tabId); + if (!tabId) { + return; } + input.focusWorkspaceTab(input.persistenceKey, tabId); + input.requestFileNavigation(tabId); } diff --git a/packages/app/src/screens/workspace/workspace-screen.tsx b/packages/app/src/screens/workspace/workspace-screen.tsx index d9510f517f..e0fb014b6e 100644 --- a/packages/app/src/screens/workspace/workspace-screen.tsx +++ b/packages/app/src/screens/workspace/workspace-screen.tsx @@ -1647,17 +1647,13 @@ function WorkspaceScreenContent({ ) => openTab({ workspaceKey, target, intent: "reveal", parentTabId, placement }), [openTab], ); - // File targets stay identity-stable so the same path reuses its tab. Keep navigation - // requests separate so clicking an unchanged path:line can still recenter the pane. - const [fileNavigationRevisionByTabId, setFileNavigationRevisionByTabId] = useState< - Record - >({}); - const requestFileNavigation = useCallback((tabId: string) => { - setFileNavigationRevisionByTabId((current) => ({ - ...current, - [tabId]: (current[tabId] ?? 0) + 1, - })); - }, []); + // File targets stay identity-stable so the same path reuses its tab. Navigation requests live + // with the tabs in the layout store, so an opener outside this screen — the Command Center — + // asks for the same reveal through the same capability. + const fileNavigationRevisionByTabId = useWorkspaceLayoutStore( + (state) => state.fileNavigationRevisionByTabId, + ); + const requestFileNavigation = useWorkspaceLayoutStore((state) => state.requestFileNavigation); const focusWorkspacePane = useWorkspaceLayoutStore((state) => state.focusPane); const hasHydratedWorkspaces = useSessionStore( (state) => state.sessions[normalizedServerId]?.hasHydratedWorkspaces ?? false, diff --git a/packages/app/src/stores/workspace-layout-store.ts b/packages/app/src/stores/workspace-layout-store.ts index 9281950eaf..1d904a98df 100644 --- a/packages/app/src/stores/workspace-layout-store.ts +++ b/packages/app/src/stores/workspace-layout-store.ts @@ -110,6 +110,12 @@ interface WorkspaceLayoutStore { pendingAgentIdsByWorkspace: Record>; hiddenAgentIdsByWorkspace: Record>; focusRestorationByWorkspace: Record; + /** + * Bumped whenever a file tab is explicitly activated. File targets are identity-stable so the + * same path reuses its tab; without this an activation of an unchanged location would look like + * a no-op and leave the pane wherever the reader had scrolled it. + */ + fileNavigationRevisionByTabId: Record; explorerSidebarPaneIdByWorkspace: Record; sidePaneIdByWorkspace: Record; openTab: (input: OpenWorkspaceTabInput) => string | null; @@ -120,6 +126,7 @@ interface WorkspaceLayoutStore { ensureSidePane: (workspaceKey: string) => string | null; closeTab: (workspaceKey: string, tabId: string) => void; focusTab: (workspaceKey: string, tabId: string) => void; + requestFileNavigation: (tabId: string) => void; selectTabInPane: (workspaceKey: string, paneId: string, tabId: string) => void; replaceTab: ( workspaceKey: string, @@ -770,6 +777,7 @@ export function createWorkspaceLayoutStore( pendingAgentIdsByWorkspace: {}, hiddenAgentIdsByWorkspace: {}, focusRestorationByWorkspace: {}, + fileNavigationRevisionByTabId: {}, explorerSidebarPaneIdByWorkspace: {}, sidePaneIdByWorkspace: {}, openTab: (input) => { @@ -1043,6 +1051,16 @@ export function createWorkspaceLayoutStore( }; }); }, + requestFileNavigation: (tabId) => { + const normalizedTabId = trimNonEmpty(tabId); + if (!normalizedTabId) return; + set((state) => ({ + fileNavigationRevisionByTabId: { + ...state.fileNavigationRevisionByTabId, + [normalizedTabId]: (state.fileNavigationRevisionByTabId[normalizedTabId] ?? 0) + 1, + }, + })); + }, focusTab: (workspaceKey, tabId) => { const normalizedWorkspaceKey = trimNonEmpty(workspaceKey); const normalizedTabId = trimNonEmpty(tabId); diff --git a/packages/server/src/server/daemon-e2e/workspace-content-search.e2e.test.ts b/packages/server/src/server/daemon-e2e/workspace-content-search.e2e.test.ts index 9f134fb581..d0d79cd0e4 100644 --- a/packages/server/src/server/daemon-e2e/workspace-content-search.e2e.test.ts +++ b/packages/server/src/server/daemon-e2e/workspace-content-search.e2e.test.ts @@ -24,10 +24,13 @@ test("real client searches saved contents, cancels host work and can search agai ); controller.abort(); expect(await searching).toMatchObject({ status: "error", code: "cancelled" }); + // A complete empty result also reports the per-file ceiling the host applied, so the client + // can say which files it never opened rather than claiming it read the whole workspace. expect(await client.searchWorkspaceContent({ cwd, query: "missing" })).toEqual({ status: "ok", matches: [], limited: false, + maxFileBytes: 1_048_576, }); const preview = await client.readFile(cwd, "a.ts", undefined, 1_048_576); expect(new TextDecoder().decode(preview.bytes)).toBe("const original = 'NEEDLE';\n"); diff --git a/packages/server/src/server/path-utils.test.ts b/packages/server/src/server/path-utils.test.ts new file mode 100644 index 0000000000..4857a97288 --- /dev/null +++ b/packages/server/src/server/path-utils.test.ts @@ -0,0 +1,18 @@ +import { sep } from "node:path"; +import { expect, test } from "vitest"; +import { toWorkspaceRelativePath } from "./path-utils.js"; + +// Workspace-relative identities cross the wire with "/" whatever the host separator is, so a +// client never has to guess which backslash was a separator and which was a file name character. +test("renders a host-relative path with forward slashes", () => { + expect(toWorkspaceRelativePath(["src", "app.ts"].join(sep))).toBe("src/app.ts"); + expect(toWorkspaceRelativePath("app.ts")).toBe("app.ts"); +}); + +test.skipIf(sep === "\\")("leaves a POSIX file name containing a backslash intact", () => { + expect(toWorkspaceRelativePath("a\\b.txt")).toBe("a\\b.txt"); +}); + +test.skipIf(sep !== "\\")("converts every Windows separator", () => { + expect(toWorkspaceRelativePath("src\\nested\\app.ts")).toBe("src/nested/app.ts"); +}); diff --git a/packages/server/src/server/workspace-content-search/index.test.ts b/packages/server/src/server/workspace-content-search/index.test.ts index eadd61c1c9..6bb96a4450 100644 --- a/packages/server/src/server/workspace-content-search/index.test.ts +++ b/packages/server/src/server/workspace-content-search/index.test.ts @@ -173,9 +173,16 @@ test("states the per-file ceiling so a search that skipped an oversized file is ).toEqual([["small.txt"], 1_048_576]); }); -test("keeps a literal backslash in a saved file name exactly as the host reported it", async () => { - // A backslash is an ordinary character in a Unix file name, never a separator. - const cwd = await workspace({ "a\\b.txt": "needle" }); - const result = await searchWorkspaceContent({ cwd, query: "needle" }); - expect(result.status === "ok" && result.matches.map((match) => match.path)).toEqual(["a\\b.txt"]); -}); +// A backslash is an ordinary character in a Unix file name but a separator on Windows, so this +// file name only exists on POSIX. Windows separator handling is covered by toWorkspaceRelativePath, +// which runs on every platform. +test.skipIf(process.platform === "win32")( + "keeps a literal backslash in a saved file name exactly as the host reported it", + async () => { + const cwd = await workspace({ "a\\b.txt": "needle" }); + const result = await searchWorkspaceContent({ cwd, query: "needle" }); + expect(result.status === "ok" && result.matches.map((match) => match.path)).toEqual([ + "a\\b.txt", + ]); + }, +);