Skip to content

Commit cf4b208

Browse files
authored
Allow selecting patch line (ranges) and linking to them (#49)
* Allow selecting patch line (ranges) and linking to them * This doesn't need to be async * Fix typo and cleanup parseLineRef * Initial drag selection impl * Adjust shift click selection to work with an anchor like dragging * Estimate patch height before fully loaded to reduce layout shift and improve linking consistency, add Go>Go to Selection menu action * Comment out jump to line delays for now * Handle selection & scroll pos in popstate navigation * Add missing state param and todo * Don't create history entries when new selection matches old * Fix history spam when loading diff from url (with selection) * Fix open diff dialog opening when loading gh diff from url * cleanup navigation logic on loading new diffs * Fix selection being lost from URL on load * Disable Go to Selection when there is no selection * Add indicator for when file/lines in file are selected to file tree * Fix FileHeader focus/selected styles overriding --tw-shadow * Set cursor-pointer on selectable lines * Fix selected indicator breaking sidebar checkbox, improve file/dir focused style * Ensure file is expanding when jumping to selected lines from menu * Fix selection not resolving when it should (navigating back to initial load state)
1 parent 35ce8ff commit cf4b208

File tree

15 files changed

+915
-245
lines changed

15 files changed

+915
-245
lines changed

bun.lock

Lines changed: 42 additions & 88 deletions
Large diffs are not rendered by default.

web-extension/package.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,17 +18,17 @@
1818
"devDependencies": {
1919
"@eslint/compat": "^2.0.0",
2020
"@eslint/js": "^9.39.1",
21-
"@types/bun": "^1.3.2",
21+
"@types/bun": "^1.3.3",
2222
"@types/webextension-polyfill": "^0.12.4",
23-
"chrome-types": "^0.1.387",
23+
"chrome-types": "^0.1.390",
2424
"eslint": "^9.39.1",
2525
"eslint-config-prettier": "^10.1.8",
2626
"globals": "^16.5.0",
2727
"prettier": "^3.6.2",
2828
"prettier-plugin-tailwindcss": "^0.7.1",
2929
"typescript": "^5.9.3",
30-
"typescript-eslint": "^8.46.3",
31-
"vite": "^7.2.2"
30+
"typescript-eslint": "^8.48.0",
31+
"vite": "^7.2.4"
3232
},
3333
"dependencies": {
3434
"@tailwindcss/vite": "^4.1.17",

web/package.json

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,12 @@
1919
"devDependencies": {
2020
"@eslint/compat": "^2.0.0",
2121
"@eslint/js": "^9.39.1",
22-
"@iconify-json/octicon": "^1.2.17",
22+
"@iconify-json/octicon": "^1.2.19",
2323
"@iconify/tailwind4": "^1.1.0",
2424
"@octokit/openapi-types": "^27.0.0",
2525
"@sveltejs/adapter-auto": "^7.0.0",
2626
"@sveltejs/adapter-cloudflare": "^7.2.4",
27-
"@sveltejs/kit": "^2.48.4",
27+
"@sveltejs/kit": "^2.49.0",
2828
"@sveltejs/vite-plugin-svelte": "^6.2.1",
2929
"@tailwindcss/vite": "^4.1.17",
3030
"@types/chroma-js": "^3.1.2",
@@ -37,18 +37,18 @@
3737
"prettier": "^3.6.2",
3838
"prettier-plugin-svelte": "^3.4.0",
3939
"prettier-plugin-tailwindcss": "^0.7.1",
40-
"svelte": "^5.43.4",
40+
"svelte": "^5.43.14",
4141
"svelte-adapter-bun": "^1.0.1",
42-
"svelte-check": "^4.3.3",
42+
"svelte-check": "^4.3.4",
4343
"tailwindcss": "^4.1.17",
4444
"tw-animate-css": "^1.4.0",
4545
"typescript": "^5.9.3",
46-
"typescript-eslint": "^8.46.3",
47-
"vite": "^7.2.2",
48-
"vitest": "^4.0.8"
46+
"typescript-eslint": "^8.48.0",
47+
"vite": "^7.2.4",
48+
"vitest": "^4.0.13"
4949
},
5050
"dependencies": {
51-
"bits-ui": "^2.14.2",
51+
"bits-ui": "^2.14.4",
5252
"chroma-js": "^3.1.2",
5353
"diff": "^8.0.2",
5454
"luxon": "^3.7.2",

web/src/app.d.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,19 @@ declare global {
55
// interface Error {}
66
// interface Locals {}
77
// interface PageData {}
8-
// interface PageState {}
8+
interface PageState {
9+
/**
10+
* whether this state entry loaded patches. when true, selection is likely to be unresolved,
11+
* and scrollOffset is likely to be 0.
12+
*/
13+
initialLoad?: boolean;
14+
scrollOffset?: number;
15+
selection?: {
16+
fileIdx: number;
17+
lines?: LineSelection;
18+
unresolvedLines?: UnresolvedLineSelection;
19+
};
20+
}
921
// interface Platform {}
1022
}
1123
}

web/src/lib/components/diff/ConciseDiffView.svelte

Lines changed: 117 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
import {
33
type ConciseDiffViewProps,
44
ConciseDiffViewState,
5+
type DiffViewerPatchHunk,
56
innerPatchLineTypeProps,
67
type InnerPatchLineTypeProps,
78
makeSearchSegments,
@@ -13,9 +14,9 @@
1314
type SearchSegment,
1415
} from "$lib/components/diff/concise-diff-view.svelte";
1516
import Spinner from "$lib/components/Spinner.svelte";
16-
import { onDestroy } from "svelte";
1717
import { type MutableValue } from "$lib/util";
1818
import { box } from "svelte-toolbelt";
19+
import { boolAttr } from "runed";
1920
2021
let {
2122
rawPatchContent,
@@ -28,10 +29,16 @@
2829
searchQuery,
2930
searchMatchingLines,
3031
activeSearchResult = -1,
32+
jumpToSearchResult = $bindable(false),
3133
cache,
3234
cacheKey,
35+
unresolvedSelection,
36+
selection = $bindable(),
37+
jumpToSelection = $bindable(false),
3338
}: ConciseDiffViewProps<K> = $props();
3439
40+
const uid = $props.id();
41+
3542
const parsedPatch = $derived.by(() => {
3643
if (rawPatchContent !== undefined) {
3744
return parseSinglePatch(rawPatchContent);
@@ -42,12 +49,20 @@
4249
});
4350
4451
const view = new ConciseDiffViewState({
52+
rootElementId: uid,
53+
4554
patch: box.with(() => parsedPatch),
4655
syntaxHighlighting: box.with(() => syntaxHighlighting),
4756
syntaxHighlightingTheme: box.with(() => syntaxHighlightingTheme),
4857
omitPatchHeaderOnlyHunks: box.with(() => omitPatchHeaderOnlyHunks),
4958
wordDiffs: box.with(() => wordDiffs),
5059
60+
unresolvedSelection: box.with(() => unresolvedSelection),
61+
selection: box.with(
62+
() => selection,
63+
(v) => (selection = v),
64+
),
65+
5166
cache: box.with(() => cache),
5267
cacheKey: box.with(() => cacheKey),
5368
});
@@ -60,39 +75,6 @@
6075
}
6176
}
6277
63-
let searchResultElements: HTMLSpanElement[] = $state([]);
64-
let didInitialJump = $state(false);
65-
let scheduledJump: ReturnType<typeof setTimeout> | undefined = undefined;
66-
$effect(() => {
67-
if (didInitialJump) {
68-
return;
69-
}
70-
if (activeSearchResult >= 0 && searchResultElements[activeSearchResult] !== undefined) {
71-
const element = searchResultElements[activeSearchResult];
72-
const anchorElement = element.closest("tr");
73-
// This is an exceptionally stupid and unreliable hack, but at least
74-
// jumping to a result in a not-yet-loaded file works most of the time with a delay
75-
// instead of never.
76-
scheduledJump = setTimeout(() => {
77-
if (scheduledJump !== undefined) {
78-
clearTimeout(scheduledJump);
79-
scheduledJump = undefined;
80-
}
81-
82-
if (anchorElement !== null) {
83-
anchorElement.scrollIntoView({ block: "center", inline: "center" });
84-
}
85-
}, 200);
86-
didInitialJump = true;
87-
}
88-
});
89-
onDestroy(() => {
90-
if (scheduledJump !== undefined) {
91-
clearTimeout(scheduledJump);
92-
scheduledJump = undefined;
93-
}
94-
});
95-
9678
let searchSegments: Promise<SearchSegment[][][]> = $derived.by(async () => {
9779
if (!searchQuery || !searchMatchingLines) {
9880
return [];
@@ -134,6 +116,21 @@
134116
}
135117
return segments;
136118
});
119+
120+
let selectionMidpoint = $derived.by(() => {
121+
if (!selection) return null;
122+
const startIdx = selection.start.idx;
123+
const endIdx = selection.end.idx;
124+
return Math.floor((startIdx + endIdx) / 2);
125+
});
126+
127+
let heightEstimateRem = $derived.by(() => {
128+
if (!parsedPatch) return 1.25;
129+
const rawLineCount = parsedPatch.hunks.reduce((sum, hunk) => sum + hunk.lines.length, 0);
130+
const headerAndSpacerLines = parsedPatch.hunks.length * 2;
131+
const totalLines = rawLineCount + headerAndSpacerLines;
132+
return totalLines * 1.25;
133+
});
137134
</script>
138135

139136
{#snippet lineContent(line: PatchLine, lineType: PatchLineTypeProps, innerLineType: InnerPatchLineTypeProps)}
@@ -165,7 +162,21 @@
165162
<span class="inline leading-[0.875rem]">
166163
{#each lineSearchSegments as searchSegment, index (index)}
167164
{#if searchSegment.highlighted}<span
168-
bind:this={searchResultElements[searchSegment.id ?? -1]}
165+
{@attach (element) => {
166+
if (jumpToSearchResult && searchSegment.id === activeSearchResult) {
167+
element.scrollIntoView({ block: "center", inline: "center" });
168+
jumpToSearchResult = false;
169+
// See similar code & comment below around jumping to selections
170+
//const scheduledJump = setTimeout(() => {
171+
// jumpToSearchResult = false;
172+
// element.scrollIntoView({ block: "center", inline: "center" });
173+
//}, 200);
174+
//return () => {
175+
// jumpToSearchResult = false;
176+
// clearTimeout(scheduledJump);
177+
//};
178+
}
179+
}}
169180
class={{
170181
"bg-[#d4a72c66]": searchSegment.id !== activeSearchResult,
171182
"bg-[#ff9632]": searchSegment.id === activeSearchResult,
@@ -186,30 +197,79 @@
186197
{/await}
187198
{/snippet}
188199

189-
{#snippet renderLine(line: PatchLine, hunkIndex: number, lineIndex: number)}
200+
{#snippet renderLine(line: PatchLine, hunk: DiffViewerPatchHunk, hunkIndex: number, lineIndex: number)}
190201
{@const lineType = patchLineTypeProps[line.type]}
191-
<div class="bg-[var(--hunk-header-bg)]">
202+
{@const lineTypeSelectable = line.type !== PatchLineType.HEADER && line.type !== PatchLineType.SPACER}
203+
<div
204+
class="bg-[var(--hunk-header-bg)] data-selectable:cursor-pointer"
205+
data-hunk-idx={hunkIndex}
206+
data-line-idx={lineIndex}
207+
data-selectable={boolAttr(lineTypeSelectable)}
208+
{@attach view.selectable(hunk, hunkIndex, line, lineIndex)}
209+
>
192210
<div class="line-number h-full px-2 select-none {lineType.lineNoClasses}">{getDisplayLineNo(line, line.oldLineNo)}</div>
193211
</div>
194-
<div class="bg-[var(--hunk-header-bg)]">
195-
<div class="line-number h-full px-2 select-none {lineType.lineNoClasses}">{getDisplayLineNo(line, line.newLineNo)}</div>
212+
<div
213+
class="bg-[var(--hunk-header-bg)] data-selectable:cursor-pointer"
214+
data-hunk-idx={hunkIndex}
215+
data-line-idx={lineIndex}
216+
data-selectable={boolAttr(lineTypeSelectable)}
217+
{@attach view.selectable(hunk, hunkIndex, line, lineIndex)}
218+
>
219+
<div
220+
class="selected-indicator line-number h-full px-2 select-none {lineType.lineNoClasses}"
221+
data-selected={boolAttr(view.isSelected(hunkIndex, lineIndex))}
222+
>
223+
{getDisplayLineNo(line, line.newLineNo)}
224+
</div>
196225
</div>
197-
<div class="w-full pl-[1rem] {lineType.classes}">
226+
<div
227+
class="selected-indicator w-full pl-[1rem] {lineType.classes}"
228+
data-hunk-idx={hunkIndex}
229+
data-line-idx={lineIndex}
230+
data-selection-start={boolAttr(view.isSelectionStart(hunkIndex, lineIndex))}
231+
data-selection-end={boolAttr(view.isSelectionEnd(hunkIndex, lineIndex))}
232+
{@attach (element) => {
233+
if (jumpToSelection && selection && selection.hunk === hunkIndex && selectionMidpoint === lineIndex) {
234+
element.scrollIntoView({ block: "center", inline: "center" });
235+
jumpToSelection = false;
236+
// Need to schedule because otherwise the vlist rendering surrounding elements may shift things
237+
// and cause the element to scroll to the wrong position
238+
// This is not 100% reliable but is good enough for now
239+
//const scheduledJump = setTimeout(() => {
240+
// jumpToSelection = false;
241+
// element.scrollIntoView({ block: "center", inline: "center" });
242+
//}, 200);
243+
//return () => {
244+
// if (scheduledJump) {
245+
// jumpToSelection = false;
246+
// clearTimeout(scheduledJump);
247+
// }
248+
//};
249+
}
250+
}}
251+
>
198252
{@render lineContentWrapper(line, hunkIndex, lineIndex, lineType, innerPatchLineTypeProps[line.innerPatchLineType])}
199253
</div>
200254
{/snippet}
201255

202256
{#await Promise.all([view.rootStyle, view.diffViewerPatch])}
203-
<div class="flex items-center justify-center bg-neutral-2 p-4"><Spinner /></div>
257+
<div class="relative bg-neutral-2" style="min-height: {heightEstimateRem}rem;">
258+
<!-- 2.25 rem for file header offset -->
259+
<div class="sticky top-[2.25rem] flex items-center justify-center p-4">
260+
<Spinner />
261+
</div>
262+
</div>
204263
{:then [rootStyle, diffViewerPatch]}
205264
<div
265+
id={uid}
206266
style={rootStyle}
207267
class="diff-content text-patch-line w-full bg-[var(--editor-bg)] font-mono text-xs leading-[1.25rem] text-[var(--editor-fg)] selection:bg-[var(--select-bg)]"
208268
data-wrap={lineWrap}
209269
>
210270
{#each diffViewerPatch.hunks as hunk, hunkIndex (hunkIndex)}
211271
{#each hunk.lines as line, lineIndex (lineIndex)}
212-
{@render renderLine(line, hunkIndex, lineIndex)}
272+
{@render renderLine(line, hunk, hunkIndex, lineIndex)}
213273
{/each}
214274
{/each}
215275
</div>
@@ -266,4 +326,19 @@
266326
left: -0.75rem;
267327
top: 0;
268328
}
329+
330+
.selected-indicator[data-selected] {
331+
box-shadow: inset -4px 0 0 0 var(--hunk-header-fg);
332+
}
333+
.selected-indicator[data-selection-start] {
334+
box-shadow: inset 0 1px 0 0 var(--hunk-header-fg);
335+
}
336+
.selected-indicator[data-selection-end] {
337+
box-shadow: inset 0 -1px 0 0 var(--hunk-header-fg);
338+
}
339+
.selected-indicator[data-selection-start][data-selection-end] {
340+
box-shadow:
341+
inset 0 1px 0 0 var(--hunk-header-fg),
342+
inset 0 -1px 0 0 var(--hunk-header-fg);
343+
}
269344
</style>

0 commit comments

Comments
 (0)