Skip to content

Commit aac3266

Browse files
enif-leeenif-lee
committed
fix: block Diff on empty-commit (0-file) PRs
Gate expandDiff, header toggle, Opt+., session/deep-link restore when changedFiles is known 0. Fixture PR #17 for e2e; control PR still opens Diff.
1 parent c65b5a9 commit aac3266

8 files changed

Lines changed: 392 additions & 16 deletions

File tree

‎src/modal/app/PrModalApp.impl.tsx‎

Lines changed: 60 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,12 @@ import { canUpdateBranch, coerceMergeMethod } from '../lib/merge-box-status';
3434
import { ConversationView } from '../views/conversation/ConversationView';
3535
import { DiffWorkspace } from '../views/pr-modal/DiffWorkspace';
3636
import { ShellResizers } from '../views/pr-modal/ShellResizers';
37-
import { LAYOUT_CENTERED, LAYOUT_DIFF, layoutClassName } from '../lib/layout-mode';
37+
import {
38+
LAYOUT_CENTERED,
39+
LAYOUT_DIFF,
40+
layoutClassName,
41+
isDiffUnavailable,
42+
} from '../lib/layout-mode';
3843
import {
3944
compareCacheKey,
4045
isAllCommitsFilter,
@@ -2121,7 +2126,8 @@ export function PrModalApp({
21212126
*/
21222127
if (searchHitHasRowIndex(hit)) {
21232128
if (layoutMode !== LAYOUT_DIFF) {
2124-
setLayoutMode(LAYOUT_DIFF);
2129+
expandDiff();
2130+
if (useModalStore.getState().layoutMode !== LAYOUT_DIFF) return;
21252131
}
21262132
const j = searchJumpRef.current;
21272133
const top = scrollTopForIndex(
@@ -2507,7 +2513,10 @@ export function PrModalApp({
25072513
line?: number | null;
25082514
side?: string | null;
25092515
}) => {
2510-
if (layoutMode !== LAYOUT_DIFF) setLayoutMode(LAYOUT_DIFF);
2516+
if (layoutMode !== LAYOUT_DIFF) {
2517+
expandDiff();
2518+
if (useModalStore.getState().layoutMode !== LAYOUT_DIFF) return;
2519+
}
25112520
// Thread jump is a focus change — clear any line selection island.
25122521
clearLineSelectionForNav();
25132522

@@ -4300,7 +4309,13 @@ export function PrModalApp({
43004309
// stacked PRs (do not clobber with the target PR's stored session layout).
43014310
const routePage = normalizePage(initialRoute?.page);
43024311
if (routePage) {
4303-
setLayoutMode(routePage === 'diff' ? LAYOUT_DIFF : LAYOUT_CENTERED);
4312+
const wantDiff = routePage === 'diff';
4313+
const emptyDiff =
4314+
typeof isDiffUnavailable === 'function' &&
4315+
isDiffUnavailable(detail);
4316+
setLayoutMode(
4317+
wantDiff && !emptyDiff ? LAYOUT_DIFF : LAYOUT_CENTERED
4318+
);
43044319
}
43054320

43064321
// Effective page for session gate (URI page, else stored page)
@@ -4330,8 +4345,13 @@ export function PrModalApp({
43304345
!routePage &&
43314346
(stored.layoutMode === 'diff' || stored.layoutMode === 'centered')
43324347
) {
4348+
const emptyDiff =
4349+
typeof isDiffUnavailable === 'function' &&
4350+
isDiffUnavailable(detail);
43334351
setLayoutMode(
4334-
stored.layoutMode === 'diff' ? LAYOUT_DIFF : LAYOUT_CENTERED
4352+
stored.layoutMode === 'diff' && !emptyDiff
4353+
? LAYOUT_DIFF
4354+
: LAYOUT_CENTERED
43354355
);
43364356
}
43374357
if (stored.diffMode === 'split' || stored.diffMode === 'unified') {
@@ -4446,7 +4466,10 @@ export function PrModalApp({
44464466

44474467
ghSelectionAppliedRef.current = applyKey;
44484468
setActiveFilePath(path);
4449-
if (layoutMode !== LAYOUT_DIFF) setLayoutMode(LAYOUT_DIFF);
4469+
if (layoutMode !== LAYOUT_DIFF) {
4470+
expandDiff();
4471+
if (useModalStore.getState().layoutMode !== LAYOUT_DIFF) return;
4472+
}
44504473

44514474
if (startLine != null && Number(startLine) >= 1) {
44524475
const end =
@@ -5054,6 +5077,15 @@ export function PrModalApp({
50545077

50555078
/** Instant layout swap — keep-alive panels, no fade/scale on Diff ↔ Conversation. */
50565079
function expandDiff(after?: any) {
5080+
// Empty-commit PRs (0 files) have no Diff surface — stay on Conversation.
5081+
const liveDetail =
5082+
useModalStore.getState().localDetail || detail || null;
5083+
if (
5084+
typeof isDiffUnavailable === 'function' &&
5085+
isDiffUnavailable(liveDetail)
5086+
) {
5087+
return;
5088+
}
50575089
setAnimClass('');
50585090
setLayoutMode(LAYOUT_DIFF);
50595091
after?.();
@@ -5069,10 +5101,30 @@ export function PrModalApp({
50695101
// peer-opt → runPaletteCommand paths (monitor fired but layout stuck).
50705102
const live =
50715103
useModalStore.getState().layoutMode || layoutMode;
5072-
if (live === LAYOUT_DIFF) collapseDiff();
5073-
else expandDiff();
5104+
if (live === LAYOUT_DIFF) {
5105+
collapseDiff();
5106+
return;
5107+
}
5108+
expandDiff();
50745109
}
50755110

5111+
// If meta settles to 0 files while Diff is open (or deep-link forced Diff
5112+
// before meta), leave Diff — empty-commit PRs have no file surface.
5113+
useEffect(() => {
5114+
if (!open) return;
5115+
const liveDetail =
5116+
useModalStore.getState().localDetail || detail || null;
5117+
if (
5118+
typeof isDiffUnavailable !== 'function' ||
5119+
!isDiffUnavailable(liveDetail)
5120+
) {
5121+
return;
5122+
}
5123+
if (useModalStore.getState().layoutMode === LAYOUT_DIFF) {
5124+
setLayoutMode(LAYOUT_CENTERED);
5125+
}
5126+
}, [open, detail?.changedFiles, detail?.additions, detail?.deletions, detail?.files, setLayoutMode]);
5127+
50765128
/** Play exit animation, then notify host to unmount (modal + side sheet). */
50775129
const requestClose = useCallback(() => {
50785130
// Embed has no exit chrome — ignore close (Escape stays no-op for shell).

‎src/modal/lib/layout-mode.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,3 +29,34 @@ export function layoutClassName(mode) {
2929
}
3030
return 'prp-modal prp-modal--centered';
3131
}
32+
33+
/**
34+
* True when Diff has nothing to show: PR meta is known and there are zero
35+
* file changes (empty-commit-only PR). Returns false when `changedFiles` is
36+
* still unknown/null so progressive load does not false-disable Diff.
37+
*
38+
* Prefer explicit `changedFiles === 0`. Fallback only when files array is
39+
* present and empty AND additions/deletions are both explicitly 0 (not null).
40+
*
41+
* @param {any} detail
42+
* @returns {boolean}
43+
*/
44+
export function isDiffUnavailable(detail) {
45+
if (!detail || typeof detail !== 'object') return false;
46+
const cf = detail.changedFiles;
47+
if (cf != null && cf !== '') {
48+
const n = Number(cf);
49+
if (Number.isFinite(n)) return n === 0;
50+
}
51+
// Unknown changedFiles: do not gate on empty files alone (may still be loading).
52+
// Only when both stats are known 0 and files list is an empty array.
53+
const files = detail.files;
54+
if (!Array.isArray(files) || files.length > 0) return false;
55+
const add = detail.additions;
56+
const del = detail.deletions;
57+
if (add == null || del == null || add === '' || del === '') return false;
58+
const a = Number(add);
59+
const d = Number(del);
60+
if (!Number.isFinite(a) || !Number.isFinite(d)) return false;
61+
return a === 0 && d === 0;
62+
}

‎src/modal/views/chrome/Header.tsx‎

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import {
2626
IconConversation,
2727
} from '@common/icons';
2828
import { EMBED_RESTORE_SHORTCUT } from '@lib/page-embed';
29-
import { LAYOUT_DIFF } from '@lib/layout-mode';
29+
import { LAYOUT_DIFF, isDiffUnavailable } from '@lib/layout-mode';
3030
import { headerReviewCompact } from '@lib/header-layout';
3131
import { useT } from '@lib/locale-context';
3232
import { branchRefCopyText, copyTextToClipboard } from '@lib/copy-to-clipboard';
@@ -402,6 +402,11 @@ export function Header(props: any) {
402402
const canReopen = detail.state === 'closed' && !detail.merged;
403403
const fileCount = detail.changedFiles ?? (detail.files || []).length;
404404
const subscribed = detail.subscribed === true;
405+
/** Empty-commit PR: block entry to Diff (leaving Diff → Conversation still OK). */
406+
const diffUnavailable =
407+
typeof isDiffUnavailable === 'function' && isDiffUnavailable(detail);
408+
const layoutToggleDisabled =
409+
diffUnavailable && effectiveLayout !== LAYOUT_DIFF;
405410

406411
return (
407412
<header
@@ -846,10 +851,20 @@ export function Header(props: any) {
846851
type="button"
847852
className="prp-header__icon-btn prp-header__icon-btn--layout prp-has-tip prp-opt-hint-host"
848853
onClick={onToggleDiff}
854+
disabled={layoutToggleDisabled}
855+
aria-disabled={layoutToggleDisabled ? 'true' : undefined}
856+
data-prp-diff-unavailable={diffUnavailable ? '1' : '0'}
849857
aria-label={
850-
effectiveLayout === LAYOUT_DIFF
851-
? t('header_show_conversation')
852-
: t('cta_show_file_diff')
858+
layoutToggleDisabled
859+
? t('aside_no_files')
860+
: effectiveLayout === LAYOUT_DIFF
861+
? t('header_show_conversation')
862+
: t('cta_show_file_diff')
863+
}
864+
title={
865+
layoutToggleDisabled
866+
? t('aside_no_files')
867+
: undefined
853868
}
854869
data-layout={
855870
effectiveLayout === LAYOUT_DIFF ? 'diff' : 'conversation'
@@ -863,11 +878,13 @@ export function Header(props: any) {
863878
)}
864879
<TipPopover
865880
title={
866-
effectiveLayout === LAYOUT_DIFF
867-
? t('tab_conversation')
868-
: t('cta_show_file_diff')
881+
layoutToggleDisabled
882+
? t('aside_no_files')
883+
: effectiveLayout === LAYOUT_DIFF
884+
? t('tab_conversation')
885+
: t('cta_show_file_diff')
869886
}
870-
shortcut="⌥."
887+
shortcut={layoutToggleDisabled ? undefined : '⌥.'}
871888
/>
872889
</button>
873890
{canClose ? (

‎tests/e2e/features/empty-diff.mjs‎

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
/**
2+
* Empty-commit PR (0 changed files): Diff must not open via header or ⌥.
3+
* Control: normal PR still reaches Diff.
4+
*/
5+
import {
6+
DEMO_PR,
7+
EMPTY_DIFF_PR,
8+
assert,
9+
evalInPage,
10+
holdChord,
11+
layout,
12+
log,
13+
modalProbe,
14+
openPr,
15+
press,
16+
setLayout,
17+
waitDetailReady,
18+
waitMs,
19+
} from '../lib/harness.mjs';
20+
21+
export { EMPTY_DIFF_PR };
22+
23+
function layoutToggleProbe() {
24+
return evalInPage(`
25+
(() => {
26+
const btn = document.querySelector(
27+
'.prp-header__icon-btn--layout, button[data-prp-diff-unavailable]'
28+
);
29+
if (!btn) return { ok: false, reason: 'no-toggle' };
30+
return {
31+
ok: true,
32+
disabled: !!(btn.disabled || btn.getAttribute('aria-disabled') === 'true'),
33+
unavailable: btn.getAttribute('data-prp-diff-unavailable') === '1',
34+
dataLayout: btn.getAttribute('data-layout') || null,
35+
aria: btn.getAttribute('aria-label') || '',
36+
};
37+
})()
38+
`);
39+
}
40+
41+
/**
42+
* @returns {import('../lib/e2e-register.ts').E2eStep[]}
43+
*/
44+
export function getSteps() {
45+
/** @type {{ name: string, fn: () => unknown | Promise<unknown> }[]} */
46+
const steps = [];
47+
const run = (name, fn) => {
48+
steps.push({ name, fn });
49+
};
50+
51+
run(`ED.1 open empty PR #${EMPTY_DIFF_PR}`, () => {
52+
openPr(EMPTY_DIFF_PR);
53+
waitDetailReady(`ED.1 empty PR #${EMPTY_DIFF_PR}`);
54+
waitMs(400);
55+
const p = modalProbe();
56+
assert(p.overlay, 'overlay missing');
57+
assert(
58+
p.layout === 'conversation' || layout() === 'conversation',
59+
`expected conversation on empty PR, layout=${p.layout || layout()}`
60+
);
61+
});
62+
63+
run('ED.2 header Diff toggle disabled / unavailable', () => {
64+
// Wait for meta (changedFiles: 0) to paint on header control
65+
let probe = layoutToggleProbe();
66+
for (let i = 0; i < 40 && !(probe?.unavailable || probe?.disabled); i++) {
67+
waitMs(150);
68+
probe = layoutToggleProbe();
69+
}
70+
log(` layout toggle: ${JSON.stringify(probe)}`);
71+
assert(probe?.ok, `layout toggle missing: ${JSON.stringify(probe)}`);
72+
assert(
73+
probe.unavailable || probe.disabled,
74+
`expected Diff unavailable on empty PR: ${JSON.stringify(probe)}`
75+
);
76+
assert(
77+
probe.disabled,
78+
`Diff toggle must be disabled: ${JSON.stringify(probe)}`
79+
);
80+
});
81+
82+
run('ED.3 header click and force-toggle stay Conversation', () => {
83+
// Do not use setLayout('diff') — it asserts success; we expect no-op.
84+
evalInPage(`
85+
(() => {
86+
const btn = document.querySelector('.prp-header__icon-btn--layout');
87+
if (btn) {
88+
btn.disabled = false; // even if re-enabled, product must no-op
89+
btn.click();
90+
}
91+
return true;
92+
})()
93+
`);
94+
waitMs(250);
95+
assert(
96+
layout() === 'conversation',
97+
`click toggle must not enter Diff, got ${layout()}`
98+
);
99+
// Direct store poke path is not e2e; product entry is onToggleDiff / expandDiff.
100+
press('Alt+.');
101+
waitMs(250);
102+
assert(
103+
layout() === 'conversation',
104+
`⌥. after click must still be conversation, got ${layout()}`
105+
);
106+
});
107+
108+
run('ED.4 ⌥. does not enter Diff', () => {
109+
// Blur composer so global Opt+. can fire
110+
evalInPage(`
111+
(() => {
112+
const ae = document.activeElement;
113+
if (ae && ae !== document.body) ae.blur?.();
114+
document.body?.focus?.();
115+
return true;
116+
})()
117+
`);
118+
waitMs(100);
119+
holdChord('Alt+.', { holdMs: 200, repeatMs: 80 });
120+
waitMs(250);
121+
press('Alt+.');
122+
waitMs(250);
123+
assert(
124+
layout() === 'conversation',
125+
`⌥. must not open Diff on empty PR, got ${layout()}`
126+
);
127+
});
128+
129+
run(`ED.5 control PR #${DEMO_PR} still opens Diff`, () => {
130+
openPr(DEMO_PR);
131+
waitDetailReady(`ED.5 control PR #${DEMO_PR}`);
132+
waitMs(300);
133+
setLayout('diff');
134+
assert(layout() === 'diff', `control PR must enter Diff, got ${layout()}`);
135+
const probe = layoutToggleProbe();
136+
log(` control toggle: ${JSON.stringify(probe)}`);
137+
assert(
138+
!probe?.unavailable,
139+
`control PR must not stamp diff-unavailable: ${JSON.stringify(probe)}`
140+
);
141+
assert(
142+
!probe?.disabled,
143+
`control PR Diff toggle must be enabled: ${JSON.stringify(probe)}`
144+
);
145+
});
146+
147+
return steps;
148+
}
149+
150+
export async function runEmptyDiff(ctx) {
151+
const { run } = ctx;
152+
for (const step of getSteps()) {
153+
await run(step.name, step.fn);
154+
}
155+
}

‎tests/e2e/features/empty-diff.rstest.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
/**
2+
* E2E group: empty-diff
3+
* Run alone: rstest run -c rstest.e2e.config.ts empty-diff
4+
*/
5+
import { registerE2eFeature } from '../lib/e2e-register';
6+
import { getSteps } from './empty-diff.mjs';
7+
8+
registerE2eFeature({
9+
title: 'e2e / empty-diff',
10+
steps: getSteps(),
11+
});

0 commit comments

Comments
 (0)