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

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ jobs:
packages/review-editor/edit/discardRestoreRender.test.tsx
packages/review-editor/edit/selectionActionPopover.test.ts
packages/review-editor/components/AllFilesCodeView.lifecycle.test.tsx
packages/review-editor/components/DiffViewer.compactTouchSelection.test.tsx
packages/guide-viewer/GuideSectionCard.test.tsx
packages/guide-viewer/GuideView.test.tsx
packages/guide-viewer/GuideViewportManager.test.tsx
Expand Down
243 changes: 243 additions & 0 deletions adr/research/SPIKE-mobile-touch-range-selection-20260816.md

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions packages/core/guide-viewer-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
import type { GuideViewerAssets } from "./guide-format";

export const GUIDE_VIEWER_MANIFEST: Omit<GuideViewerAssets, "baseUrl"> = {
js: "viewer.sFtOnb1i.js",
js: "viewer.BhU6ea2v.js",
css: "viewer.Dtx2gmOp.css",
jsIntegrity: "sha384-HRAx6JL8MwurSiEBSingzilCiL9O4o93I9G45am9wtXscw419eSvXAontQL/TkHd",
jsIntegrity: "sha384-gtNYVWIHVgo9erfFEu8RspdmcaRUrVhz0McgJacIXvD0ZvUOu4QWsDr8Nv5uSCdy",
cssIntegrity: "sha384-jbTLuO1urA+7R65nS8yRrbSr13BuYyfeSZBvNMTAJuJCcmJVWxw6F3R0C+735ihs",
langs: {
"astro": "chunks/astro.BykyiR6i.js",
Expand Down
104 changes: 102 additions & 2 deletions packages/review-editor/components/AllFilesCodeView.lifecycle.test.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import { afterAll, afterEach, describe, expect, mock, test } from 'bun:test';
import React, { act, useCallback, useEffect, useImperativeHandle, useRef } from 'react';
import React, { act, useCallback, useEffect, useImperativeHandle, useRef, useState } from 'react';
import { createRoot, type Root } from 'react-dom/client';
import type { SelectedLineRange } from '@plannotator/ui/types';
import type { DiffFile } from '../types';

let codeViewMounts = 0;
let codeViewUnmounts = 0;
let scrollTargets: Array<Record<string, unknown>> = [];
let lastCodeViewProps: Record<string, unknown> | null = null;
let toolbarSelections: Array<SelectedLineRange | null> = [];

// Captured BEFORE the mocks below replace the specifiers, so this file can put
// the real modules back when it is done. `mock.module` is process global and
Expand Down Expand Up @@ -103,7 +105,15 @@ mock.module('@pierre/diffs/react', () => ({
}));

mock.module('./ToolbarHost', () => ({
ToolbarHost: React.forwardRef(function MockToolbarHost() {
ToolbarHost: React.forwardRef(function MockToolbarHost(_props, ref) {
useImperativeHandle(ref, () => ({
handleLineSelectionEnd: (range: SelectedLineRange | null) => {
toolbarSelections.push(range);
},
openLineAnnotation: () => {},
handleTokenClick: () => {},
startEdit: () => {},
}));
return null;
}),
}));
Expand Down Expand Up @@ -159,6 +169,7 @@ afterEach(async () => {
codeViewUnmounts = 0;
scrollTargets = [];
lastCodeViewProps = null;
toolbarSelections = [];
});

// Hand the real @pierre/diffs back to the process. Only the two library
Expand Down Expand Up @@ -226,6 +237,95 @@ describe('AllFilesCodeView guide mount state', () => {

});

describe('AllFilesCodeView compact-touch line selection', () => {
// Stands in for App: a published range comes straight back down as
// `pendingSelection`. That loop is load-bearing — CodeView's selection is
// controlled, and the reconcile effect clears the highlight whenever
// pendingSelection is null, so a statically-null prop would wipe the range
// the preserve branch just painted.
function Harness({ compactTouchLayout, onSelection }: {
compactTouchLayout: boolean;
onSelection?: (range: SelectedLineRange | null) => void;
}) {
const [pendingSelection, setPendingSelection] = useState<SelectedLineRange | null>(null);
return view({
compactTouchLayout,
pendingSelection,
onLineSelection: (range) => {
onSelection?.(range);
setPendingSelection(range);
},
});
}

async function mount(
compactTouchLayout: boolean,
onSelection?: (range: SelectedLineRange | null) => void,
) {
host = document.createElement('div');
host.style.height = '400px';
document.body.appendChild(host);
root = createRoot(host);
await act(async () => {
root!.render(<Harness compactTouchLayout={compactTouchLayout} onSelection={onSelection} />);
await new Promise((resolve) => setTimeout(resolve, 25));
});
}

function getSelectionCallbacks() {
const options = lastCodeViewProps?.options as {
onLineSelectionEnd?: (
range: SelectedLineRange | null,
context: { item: { id: string; type: 'diff' } },
) => void;
onGutterUtilityClick?: (
range: SelectedLineRange,
context: { item: { id: string; type: 'diff' } },
) => void;
};
const item = (lastCodeViewProps?.initialItems as Array<{ id: string; type: 'diff' }>)[0];
return { options, item };
}

test.skipIf(!hasDom)('preserves a dragged range, then opens the composer from the gutter action', async () => {
const observedSelections: Array<SelectedLineRange | null> = [];
const range: SelectedLineRange = { start: 4, end: 8, side: 'additions' };
await mount(true, (selection) => observedSelections.push(selection));
const { options, item } = getSelectionCallbacks();

await act(async () => {
options.onLineSelectionEnd?.(range, { item });
});

expect(observedSelections.at(-1)).toEqual(range);
expect(toolbarSelections).toEqual([]);
// Publishing the range upward is only half of "preserved": CodeView's
// selection is controlled here, so the highlight only survives if the range
// is also handed back down. Without this the composer would stay shut on a
// range nothing paints.
expect(lastCodeViewProps?.selectedLines).toEqual({ id: item.id, range });

await act(async () => {
options.onGutterUtilityClick?.(range, { item });
});

expect(toolbarSelections).toEqual([range]);
});

test.skipIf(!hasDom)('keeps the incumbent desktop selection-to-composer transition', async () => {
const range: SelectedLineRange = { start: 4, end: 8, side: 'additions' };
await mount(false);
const { options, item } = getSelectionCallbacks();

await act(async () => {
options.onLineSelectionEnd?.(range, { item });
await new Promise((resolve) => setTimeout(resolve, 0));
});

expect(toolbarSelections).toEqual([range]);
});
});

describe('AllFilesCodeView readOnly (portable guide host)', () => {
// The portable Guided Review viewer renders this component with no server
// and no review state behind it (decision record D2/D4). These guard the
Expand Down
41 changes: 33 additions & 8 deletions packages/review-editor/components/AllFilesCodeView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ import { buildCodeNavRequest } from '../utils/buildCodeNavRequest';
import { getDiffSelection, getLineNumberFromNode, getSideFromNode } from '../utils/diffSelection';
import { isContentConsistentWithPatch } from '../utils/patchConsistency';
import { hashString } from '../utils/hashString';
import {
resolveLineSelectionBehavior,
type LineSelectionSource,
} from '../utils/lineSelectionBehavior';
import { isContentlessBinaryPatch, isOversizedReviewStubPatch } from '@plannotator/shared/diff-paths';
import { OversizedFileNotice } from './OversizedFileNotice';
import { ToolbarHost, type ToolbarHostHandle } from './ToolbarHost';
Expand Down Expand Up @@ -560,7 +564,12 @@ export const AllFilesCodeView: React.FC<AllFilesCodeViewProps> = ({
// header-custom slot, no [data-title] element), so that rule is moot either
// way — we keep `true` to be explicit that the built-in title is irrelevant
// here (our FileHeader owns all header chrome).
const pierreTheme = usePierreTheme({ fontFamily, fontSize, showFileHeader: true });
const pierreTheme = usePierreTheme({
fontFamily,
fontSize,
showFileHeader: true,
compactTouchLayout,
});
// Worker-pool highlighting: wait for the pool so the first tokenization
// wave runs in workers (not a main-thread fallback), and keep the pool's
// theme pair in step with the UI theme.
Expand Down Expand Up @@ -1820,25 +1829,41 @@ export const AllFilesCodeView: React.FC<AllFilesCodeViewProps> = ({
}
}, [activeFilePath, pendingSelection, filePathToItemId]);

const handleLineSelectionEnd = useStableCallback(
(range: SelectedLineRange | null, item: CodeViewItem<DiffAnnotationMetadata>) => {
const handleLineSelectionInteraction = useStableCallback(
(
source: LineSelectionSource,
range: SelectedLineRange | null,
item: CodeViewItem<DiffAnnotationMetadata>,
) => {
if (range == null || item.type !== 'diff') return;
// The file being edited owns its pointer interactions — opening the
// annotation toolbar over an active editor would fight its focus.
if (item.id === editSession.editingItemIdRef.current) return;
const filePath = itemIdToFilePath.get(item.id);
if (filePath == null) return;
if (resolveLineSelectionBehavior({
source,
compactTouchLayout: compactTouchLayout === true,
}) === 'preserve-selection') {
pendingToolbarRange.current = null;
setActiveFilePath(filePath);
setSelectedLines({ id: item.id, range });
onLineSelection(range);
return;
}
routeSelectionToToolbar(range, filePath);
},
);

const handleLineSelectionEnd = useStableCallback(
(range: SelectedLineRange | null, item: CodeViewItem<DiffAnnotationMetadata>) => {
handleLineSelectionInteraction('range-gesture', range, item);
},
);

const handleGutterUtilityClick = useStableCallback(
(range: SelectedLineRange, item: CodeViewItem<DiffAnnotationMetadata>) => {
if (item.type !== 'diff') return;
if (item.id === editSession.editingItemIdRef.current) return;
const filePath = itemIdToFilePath.get(item.id);
if (filePath == null) return;
routeSelectionToToolbar(range, filePath);
handleLineSelectionInteraction('gutter-comment-action', range, item);
},
);

Expand Down
Loading