Skip to content

Commit be9ee16

Browse files
amadeusclaude
andauthored
Update diffs to latest 1.3.0-beta.9 (#116)
Ports things over to use thew new custom CodeView header api and also take advantage of the new hydration APIs. You'll see the history updates to 2 different versions, Fable noticed a bit of an issue that was worth resolving in a secondary update. Pretty much all this code was vibed, and since i don't have a strong understanding of the internals, unclear if this is the correct way forward on this stuff. If it sucks, blame Fable 🤣 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent f03ea44 commit be9ee16

12 files changed

Lines changed: 395 additions & 211 deletions

core/App.css

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1613,6 +1613,22 @@ html[data-codiff-platform='darwin'] .sidebar {
16131613
font-size: 14px;
16141614
}
16151615

1616+
/* The PR/commit description card renders in CodeView's non-virtualized header
1617+
region at the top of the scroll content; the host div inherits the
1618+
scroller's inline padding, so only vertical spacing is needed here. */
1619+
.code-view [data-diffs-code-view-header] .codiff-code-view-source-description {
1620+
margin-top: 11px;
1621+
}
1622+
1623+
/* The title bar sticks to the top of the scroller while the card is in view
1624+
and unsticks naturally as the card scrolls past (sticky cannot escape the
1625+
panel; `overflow: clip` does not create a scroll container). */
1626+
.code-view [data-diffs-code-view-header] .codiff-source-description-header {
1627+
position: sticky;
1628+
top: 0;
1629+
z-index: 3;
1630+
}
1631+
16161632
.codiff-source-description-panel-body {
16171633
background: var(--code-bg);
16181634
}

core/App.tsx

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import type { FileDiffLoadedFiles } from '@pierre/diffs';
12
import {
23
useCallback,
34
useEffect,
@@ -67,6 +68,7 @@ import {
6768
getItemId,
6869
isPatchOnlyDiffSection,
6970
shouldLoadDiffSectionContents,
71+
shouldPreloadSectionContentsForSearch,
7072
} from './lib/diff.ts';
7173
import { compactPath, fuzzyMatches, sortFiles } from './lib/files.ts';
7274
import { isNativeInputTarget } from './lib/keyboard.ts';
@@ -442,6 +444,36 @@ export default function App() {
442444
[bumpItemVersion],
443445
);
444446

447+
// Fetches full file contents for a patch-only section so the CodeView
448+
// `loadDiffFiles` option can hydrate the rendered diff in place. Unlike
449+
// `loadDiffSection`, this must not touch React state: replacing the section
450+
// would reset the hydrated diff object's identity.
451+
const loadDiffSectionContents = useCallback(
452+
async (file: ChangedFile, section: DiffSection): Promise<FileDiffLoadedFiles> => {
453+
const currentState = stateRef.current;
454+
if (!currentState || !supportsLazyDiffContent(currentState.source)) {
455+
throw new Error(`Cannot load diff contents for '${file.path}'.`);
456+
}
457+
458+
const loadedSection = await window.codiff.getDiffSectionContent({
459+
force: true,
460+
kind: section.kind,
461+
path: file.path,
462+
showWhitespace: preferencesRef.current.showWhitespace,
463+
source: currentState.source,
464+
});
465+
if (!loadedSection.newFile) {
466+
throw new Error(`No file contents available for '${file.path}'.`);
467+
}
468+
469+
return {
470+
newFile: loadedSection.newFile,
471+
oldFile: loadedSection.oldFile ?? null,
472+
};
473+
},
474+
[],
475+
);
476+
445477
const refreshMarkdownFile = useCallback(
446478
(file: ChangedFile, _section: DiffSection) => {
447479
const refresh = async () => {
@@ -854,7 +886,7 @@ export default function App() {
854886
fileHasVisibleDiff(file, preferences.showWhitespace),
855887
);
856888
const requests = searchableFiles.flatMap((file) =>
857-
file.sections.filter(shouldLoadDiffSectionContents).map((section) => ({
889+
file.sections.filter(shouldPreloadSectionContentsForSearch).map((section) => ({
858890
file,
859891
section,
860892
})),
@@ -2591,6 +2623,7 @@ export default function App() {
25912623
onDeleteComment: deleteComment,
25922624
onLoadImageContent: window.codiff.getDiffImageContent,
25932625
onLoadSection: loadDiffSection,
2626+
onLoadSectionContents: loadDiffSectionContents,
25942627
onOpenFile: openFile,
25952628
onRefreshMarkdown: refreshMarkdownFile,
25962629
onSaveCommentEdit: updateComment,

core/SharedWalkthroughApp.tsx

Lines changed: 0 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,6 @@ export type MergeRequestReviewAppProps = {
170170
>;
171171
settingsBar?: ReactNode;
172172
sourceDescriptionFooterAside?: ReactNode;
173-
sourceDescriptionFooterAsideKey?: string;
174173
state: RepositoryState;
175174
title: string;
176175
walkthrough: NarrativeWalkthrough | null;
@@ -1012,7 +1011,6 @@ type ReviewSurfaceProps = {
10121011
settingsBar?: ReactNode;
10131012
snapshot: SharedWalkthroughSnapshot;
10141013
sourceDescriptionFooterAside?: ReactNode;
1015-
sourceDescriptionFooterAsideKey?: string;
10161014
title?: string;
10171015
};
10181016

@@ -1026,7 +1024,6 @@ function ReviewSurface({
10261024
settingsBar,
10271025
snapshot,
10281026
sourceDescriptionFooterAside,
1029-
sourceDescriptionFooterAsideKey,
10301027
title,
10311028
}: ReviewSurfaceProps) {
10321029
const canComment = commenting?.canComment ?? Boolean(interactive);
@@ -1774,28 +1771,6 @@ function ReviewSurface({
17741771
) : (
17751772
(sourceDescriptionFooterMain ?? sourceDescriptionFooterAside)
17761773
);
1777-
const sourceDescriptionFooterKey =
1778-
interactive && sourceMergeState && !isTerminalMergeState
1779-
? [
1780-
'merge',
1781-
pullRequestMergeSubmitting ? 'submitting' : 'idle',
1782-
sourceMergeState.sha,
1783-
String(sourceMergeState.autoMergeEnabled),
1784-
String(sourceMergeState.canCancelAutoMerge),
1785-
String(sourceMergeState.canMerge),
1786-
String(sourceMergeState.canSetAutoMerge),
1787-
sourceMergeState.status,
1788-
sourceMergeState.statusLabel,
1789-
String(sourceMergeState.options.removeSourceBranch),
1790-
String(sourceMergeState.options.squash),
1791-
...sourceMergeState.checks.map(
1792-
(check) => `${check.status}:${check.label}:${check.detail ?? ''}:${check.url ?? ''}`,
1793-
),
1794-
sourceDescriptionFooterAsideKey ? `aside:${sourceDescriptionFooterAsideKey}` : '',
1795-
].join('|')
1796-
: sourceDescriptionFooter
1797-
? (sourceDescriptionFooterAsideKey ?? 'custom')
1798-
: '';
17991774
const sourceDescription =
18001775
source.type === 'pull-request' ? (
18011776
<PullRequestSourceDescription
@@ -1829,7 +1804,6 @@ function ReviewSurface({
18291804
showSourceDescription
18301805
sourceDescriptionActions={sourceDescriptionActions}
18311806
sourceDescriptionFooter={sourceDescriptionFooter}
1832-
sourceDescriptionFooterKey={sourceDescriptionFooterKey}
18331807
walkthroughNotes={emptyWalkthroughNotes}
18341808
/>
18351809
</div>
@@ -2065,7 +2039,6 @@ function ReviewSurface({
20652039
selectedPath={visibleSelectedPath}
20662040
sourceDescriptionActions={sourceDescriptionActions}
20672041
sourceDescriptionFooter={sourceDescriptionFooter}
2068-
sourceDescriptionFooterKey={sourceDescriptionFooterKey}
20692042
walkthroughNotes={emptyWalkthroughNotes}
20702043
/>
20712044
)
@@ -2150,7 +2123,6 @@ export function MergeRequestReviewApp({
21502123
preferences,
21512124
settingsBar,
21522125
sourceDescriptionFooterAside,
2153-
sourceDescriptionFooterAsideKey,
21542126
state,
21552127
title,
21562128
walkthrough,
@@ -2230,7 +2202,6 @@ export function MergeRequestReviewApp({
22302202
settingsBar={settingsBar}
22312203
snapshot={snapshot}
22322204
sourceDescriptionFooterAside={sourceDescriptionFooterAside}
2233-
sourceDescriptionFooterAsideKey={sourceDescriptionFooterAsideKey}
22342205
title={title}
22352206
/>
22362207
);

core/__tests__/App.test.tsx

Lines changed: 126 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,13 @@ import {
55
canRenderImagePreview,
66
getDiffLineCount,
77
getMarkdownPreviewContents,
8+
getSectionForFileDiff,
89
getTotalDiffLineCount,
910
getVisibleDiffSections,
1011
fileHasVisibleDiff,
12+
loadSectionContents,
1113
shouldLoadDiffSectionContents,
14+
shouldPreloadSectionContentsForSearch,
1215
} from '../lib/diff.ts';
1316
import { isDiffSearchShortcut } from '../lib/keyboard.ts';
1417
import { renderInlineMarkdown, sanitizeMarkdownImages } from '../lib/markdown.tsx';
@@ -70,17 +73,37 @@ test('pure renames are visible without content hunks', () => {
7073
expect(fileHasVisibleDiff(file, false)).toBe(true);
7174
});
7275

73-
test('patch-only text sections are loadable for full context expansion', () => {
76+
test('patch-only text sections hydrate lazily instead of eager loading', () => {
77+
const patchOnlySection = {
78+
binary: false,
79+
id: 'src/app.ts:unstaged',
80+
kind: 'unstaged',
81+
loadState: 'ready',
82+
patch: 'diff --git a/src/app.ts b/src/app.ts\n@@ -1 +1 @@\n-old\n+new\n',
83+
} as const;
84+
85+
// Patch-only sections expand via `loadDiffFiles` hydration, not the eager
86+
// Load flow, but diff search still preloads their full contents.
87+
expect(shouldLoadDiffSectionContents(patchOnlySection)).toBe(false);
88+
expect(shouldPreloadSectionContentsForSearch(patchOnlySection)).toBe(true);
89+
7490
expect(
7591
shouldLoadDiffSectionContents({
76-
binary: false,
77-
id: 'src/app.ts:unstaged',
78-
kind: 'unstaged',
79-
loadState: 'ready',
80-
patch: 'diff --git a/src/app.ts b/src/app.ts\n@@ -1 +1 @@\n-old\n+new\n',
92+
...patchOnlySection,
93+
loadState: 'deferred',
8194
}),
8295
).toBe(true);
8396

97+
expect(
98+
shouldPreloadSectionContentsForSearch({
99+
...patchOnlySection,
100+
summary: {
101+
canLoad: false,
102+
reason: 'Codiff could not load full file context.',
103+
},
104+
}),
105+
).toBe(false);
106+
84107
expect(
85108
shouldLoadDiffSectionContents({
86109
binary: false,
@@ -114,6 +137,103 @@ test('patch-only text sections are loadable for full context expansion', () => {
114137
).toBe(false);
115138
});
116139

140+
test('patch-only diffs are registered for lazy hydration and side-cached contents upgrade re-parses', async () => {
141+
const section = {
142+
binary: false,
143+
id: 'src/lazy.ts:unstaged',
144+
kind: 'unstaged',
145+
loadState: 'ready',
146+
patch:
147+
'diff --git a/src/lazy.ts b/src/lazy.ts\n--- a/src/lazy.ts\n+++ b/src/lazy.ts\n@@ -2,3 +2,3 @@\n b\n-c\n+C\n d\n',
148+
} as const;
149+
const file = {
150+
fingerprint: 'lazy-hydration',
151+
path: 'src/lazy.ts',
152+
sections: [section],
153+
status: 'modified',
154+
} satisfies ChangedFile;
155+
156+
const { fileDiff } = getVisibleDiffSections(file, false)[0];
157+
expect(fileDiff.isPartial).toBe(true);
158+
// Stable identity across re-parses: hydration mutates this object in place.
159+
expect(getVisibleDiffSections(file, false)[0].fileDiff).toBe(fileDiff);
160+
161+
const target = getSectionForFileDiff(fileDiff);
162+
expect(target?.file).toBe(file);
163+
expect(target?.section).toBe(section);
164+
165+
let loadCount = 0;
166+
const load = async () => {
167+
loadCount += 1;
168+
return {
169+
newFile: { contents: 'a\nb\nC\nd\ne\n', name: 'src/lazy.ts' },
170+
oldFile: { contents: 'a\nb\nc\nd\ne\n', name: 'src/lazy.ts' },
171+
};
172+
};
173+
174+
// Concurrent loads dedupe; later calls resolve from the cache.
175+
const [first, second] = await Promise.all([
176+
loadSectionContents(file, section, load),
177+
loadSectionContents(file, section, load),
178+
]);
179+
const third = await loadSectionContents(file, section, load);
180+
expect(loadCount).toBe(1);
181+
expect(second).toBe(first);
182+
expect(third).toBe(first);
183+
184+
// CodeView hydrates the cached object in place (as of 1.3.0-beta.9), so a
185+
// re-parse under the same cache key keeps returning the same object. A
186+
// re-parse under a different key (e.g. the whitespace toggle) starts from a
187+
// fresh patch parse and is hydrated from the cached contents instead of
188+
// resetting to a partial diff.
189+
expect(getVisibleDiffSections(file, false)[0].fileDiff).toBe(fileDiff);
190+
const reparsedFlippedFlag = getVisibleDiffSections(file, true)[0].fileDiff;
191+
expect(reparsedFlippedFlag.isPartial).toBe(false);
192+
expect(reparsedFlippedFlag).not.toBe(fileDiff);
193+
});
194+
195+
test('non-loadable and placeholder diffs are not registered for hydration', () => {
196+
const nonLoadableFile = {
197+
fingerprint: 'non-loadable',
198+
path: 'src/locked.ts',
199+
sections: [
200+
{
201+
binary: false,
202+
id: 'src/locked.ts:unstaged',
203+
kind: 'unstaged',
204+
loadState: 'ready',
205+
patch:
206+
'diff --git a/src/locked.ts b/src/locked.ts\n--- a/src/locked.ts\n+++ b/src/locked.ts\n@@ -1 +1 @@\n-old\n+new\n',
207+
summary: {
208+
canLoad: false,
209+
reason: 'Codiff could not load full file context.',
210+
},
211+
},
212+
],
213+
status: 'modified',
214+
} satisfies ChangedFile;
215+
216+
const binaryFile = {
217+
fingerprint: 'binary-placeholder',
218+
path: 'assets/logo.bin',
219+
sections: [
220+
{
221+
binary: true,
222+
id: 'assets/logo.bin:unstaged',
223+
kind: 'unstaged',
224+
patch: '',
225+
},
226+
],
227+
status: 'modified',
228+
} satisfies ChangedFile;
229+
230+
for (const file of [nonLoadableFile, binaryFile]) {
231+
const { fileDiff } = getVisibleDiffSections(file, false)[0];
232+
expect(fileDiff.isPartial).toBe(true);
233+
expect(getSectionForFileDiff(fileDiff)).toBeUndefined();
234+
}
235+
});
236+
117237
test('empty patch-only sections are not visible or countable', () => {
118238
const file = {
119239
fingerprint: 'empty-patch-only',

core/__tests__/MergeRequestReviewApp.test.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,9 @@ test('merge request reviews expose navigation, actions, and lazy walkthrough gen
194194
expect(tabs[0]?.getAttribute('aria-selected')).toBe('true');
195195
expect(view.container.querySelector('.codiff-web-source-description')).toBeNull();
196196
await waitFor(() => {
197-
const sourceDescription = view.container.querySelector('.codiff-source-description-item');
197+
const sourceDescription = view.container.querySelector(
198+
'[data-diffs-code-view-header] .codiff-code-view-source-description',
199+
);
198200
expect(sourceDescription).not.toBeNull();
199201
expect(sourceDescription?.closest('.code-view')).not.toBeNull();
200202
expect(sourceDescription?.textContent).toContain('Review in Codiff');
@@ -1183,7 +1185,9 @@ test('merge request walkthrough shows the merge request description before the f
11831185

11841186
await waitFor(() => {
11851187
const surface = view.container.querySelector('.wt-diff-surface');
1186-
const descriptionPanel = surface?.querySelector('.codiff-source-description-item');
1188+
const descriptionPanel = surface?.querySelector(
1189+
'[data-diffs-code-view-header] .codiff-code-view-source-description',
1190+
);
11871191
expect(descriptionPanel).not.toBeNull();
11881192
expect(descriptionPanel?.querySelector('.codiff-file-header')).not.toBeNull();
11891193
expect(surface?.textContent?.indexOf('Review in Codiff')).toBeLessThan(

core/__tests__/ReviewCodeView-scroll.test.tsx

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -955,28 +955,26 @@ test('read-only markdown previews trigger CodeView layout remeasurement after he
955955

956956
try {
957957
const markdownItemId = `diff:${markdownSectionId}`;
958-
const sourceItemId = 'source-description:github:https://github.com/nkzw-tech/codiff/pull/12';
959958
const initialMarkdownVersion = getCodeViewItemVersion(markdownItemId);
960-
const initialSourceVersion = getCodeViewItemVersion(sourceItemId);
961959

962960
const markdownPreview = app.container.querySelector<HTMLElement>(
963961
'[aria-label="Preview plan.md"]',
964962
);
963+
// The source description renders in CodeView's header region, where height
964+
// changes are observed by the viewer directly instead of item versions.
965965
const sourceDescription = app.container.querySelector<HTMLElement>(
966-
'[aria-label="Preview source description"]',
966+
'[data-diffs-code-view-header] [aria-label="Preview source description"]',
967967
);
968968

969969
expect(markdownPreview).not.toBeNull();
970970
expect(sourceDescription).not.toBeNull();
971971

972972
await act(async () => {
973973
markdownPreview?.dispatchEvent(new MouseEvent('dblclick', { bubbles: true }));
974-
sourceDescription?.dispatchEvent(new MouseEvent('dblclick', { bubbles: true }));
975974
});
976975

977976
await waitFor(() => {
978977
expect(getCodeViewItemVersion(markdownItemId)).not.toBe(initialMarkdownVersion);
979-
expect(getCodeViewItemVersion(sourceItemId)).not.toBe(initialSourceVersion);
980978
});
981979
} finally {
982980
await app.cleanup();

0 commit comments

Comments
 (0)