Skip to content

fix(review): list commit scopes newest first and keep the newest on huge prs #103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
xieziyu merged 1 commit into from
Sep 7, 2026
Merged

fix(review): list commit scopes newest first and keep the newest on huge prs #103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion docs/design/ui.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ review 屏原先把品牌、来源、模型、用量、状态、CTA、主题、
一个 PR 常常一半是重构、一半是新逻辑。整份 diff 一起审时,agent 的注意力被最大的那一块吃掉,而 reviewer 想问的往往是「**这一个提交**干了什么」。所以 review 屏里可以在**整个 PR** 与 **PR 里的某一个提交**之间来回切,两者各自是一份改动面、一套 findings。

- **切换器锚在顶栏那枚范围 chip 上**,与来源 chip、base chip 同一级 —— 范围回答的是「这一屏在看什么」,和它们是同一类上下文。否决的两个位置:放**左栏顶部**要与文件树抢那一列的注意力,而人在这一栏里干的是找文件;做成三栏之上的**常驻范围条**,等于为一件低频动作永久让出一行高度 —— 切完之后人是要长时间读 diff 的。chip 还有一个位置换不来的好处:它已经是「本次审的不是默认那份」的既有语言(base chip 就长这样),扩成可点开的下拉不需要教。
- **弹层里「整个 PR」永远是第一行**,提交按 GitHub commits 页的顺序(旧→新)——用户是照着那一页找「刚才那个提交」的,倒过来会让他数错位置。每行带机审状态(未机审 / 扫描中 / N findings / 已提交 N):这枚弹层是**唯一**看得见各范围进度的地方。
- **弹层里「整个 PR」永远是第一行**,提交**新→旧**,最近的一个紧挨着它。最初照 GitHub commits 页排成旧→新,理由是「用户照着那一页数位置」;实际用下来切过来找的几乎总是最近几个提交,旧→新把它们压到列表底下,每次都要滚。数位置这件事由 chip 上的 k/N 承担(k 仍按旧→新计,与首轮提示词告诉 agent 的位置同一口径),列表不必再为它保序;入口屏的范围选择器同向。每行带机审状态(未机审 / 扫描中 / N findings / 已提交 N):这枚弹层是**唯一**看得见各范围进度的地方。
- **`⌥↑ / ⌥↓` 不开弹层直接前后切**,到头不回环。为此提交列表在进屏时就拉一次(而不是等第一次打开弹层)——否则这个键位在按过一次 chip 之前是坏的,而它恰恰是给「逐个提交看过去」那种用法准备的。
- **切范围一律内联等待**,不弹浮层:这是「进屏之后一律内联」的延伸。占位只出现在左栏与中栏 —— 那两栏是新范围的答案会出现的地方;顶栏、tab、右栏一格不动,人才知道自己没有离开这个 PR。切换失败就地说一句并**留在原范围**:把三栏清空换成一句错误,等于连正在读的那份 diff 也一并收走。

Expand Down
80 changes: 68 additions & 12 deletions scripts/spike-scope.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { ReviewManager } from '../src/backend/review/review-manager';
import { setToolPath } from '../src/backend/config/tool-paths';
import { buildScanPrompt } from '../src/backend/prompt/scan-prompt';
import { isUnscanned, REVIEW_RETENTION_MS } from '../src/shared/domain';
import type { CommitPosition } from '../src/shared/source-discovery';
import { PR_COMMITS_CAP, type CommitPosition } from '../src/shared/source-discovery';

function log(msg: string) {
process.stdout.write(`[scope] ${msg}\n`);
Expand All @@ -31,30 +31,44 @@ const SHAS = [
'ccccccc3333333333333333333333333333cccc3',
];

/** 第 i 个(0 基)提交的 oid:前三个用 SHAS,再往后合成 —— 假 gh 与断言两边要算得一样。 */
const oidAt = (i: number): string => SHAS[i] ?? (i + 1).toString(16).padStart(40, '0');

/**
* 假 gh:认 `pr view` / `pulls/N/commits` / `commits/<sha>`(diff 媒体类型)三条路径。
* 假 gh:认 `pr view` / GraphQL 的 PR commits 连接 / `commits/<sha>`(diff 媒体类型)/ compare 几条路径。
* 写成 node 脚本而不是 shell —— 参数里带 `Accept: ...` 空格与冒号,shell 引用一错就变成静默走空分支。
*
* 提交总数由环境变量 SPIKE_COMMIT_TOTAL 定(缺省 3),GraphQL 侧按 `last` / `before` 真分页:
* 封顶截断那条路只有靠几百个提交才走得到。
*/
function fakeGh(): string {
const dir = mkdtempSync(path.join(tmpdir(), 'duetlens-spike-gh-'));
const bin = path.join(dir, 'gh');
const commits = SHAS.map((sha, i) => ({
sha,
commit: { message: `feat: step ${i + 1}\n\nbody`, committer: { date: '2026-09-01T00:00:00Z' }, author: { name: 'dev' } },
author: { login: 'dev' },
parents: [{}],
}));
writeFileSync(
bin,
`#!/usr/bin/env node
const a = process.argv.slice(2);
const joined = a.join(' ');
const SHAS = ${JSON.stringify(SHAS)};
const oidAt = (i) => SHAS[i] ?? (i + 1).toString(16).padStart(40, '0');
const total = Number(process.env.SPIKE_COMMIT_TOTAL || 3);
if (a[0] === 'pr' && a[1] === 'view') {
process.stdout.write(JSON.stringify({ title: 'streaming pipeline', number: 42, headRefOid: ${JSON.stringify(SHAS[2])}, url: 'https://github.com/acme/repo/pull/42' }));
process.exit(0);
}
if (/pulls\\/42\\/commits/.test(joined)) {
process.stdout.write(/page=1(&|$)/.test(joined) ? ${JSON.stringify(JSON.stringify(commits))} : '[]');
if (a[0] === 'api' && a[1] === 'graphql' && /commits\\(last/.test(joined)) {
const opt = (k) => { const t = a.find((x) => x.startsWith(k + '=')); return t ? t.slice(k.length + 1) : null; };
const page = Number(opt('page'));
const before = opt('before');
const end = before ? Number(before.slice(1)) : total;
const start = Math.max(0, end - page);
const nodes = [];
for (let i = start; i < end; i++) nodes.push({ commit: { oid: oidAt(i), messageHeadline: 'feat: step ' + (i + 1), committedDate: '2026-09-01T00:00:00Z', author: { name: 'dev', user: { login: 'dev' } }, parents: { totalCount: 1 } } });
process.stdout.write(JSON.stringify({ data: { repository: { pullRequest: { commits: { totalCount: total, pageInfo: { hasPreviousPage: start > 0, startCursor: 'c' + start }, nodes } } } } }));
process.exit(0);
}
if (/compare\\//.test(joined)) {
process.stdout.write(JSON.stringify({ status: 'ahead', base_commit: { commit: { message: 'feat: step 2\\n\\nbody' } } }));
process.exit(0);
}
if (/repos\\/[^ ]+\\/commits\\//.test(joined)) {
Expand Down Expand Up @@ -234,6 +248,19 @@ async function main() {
assert.equal(scopes.capped, false);
log('listScopes:⋈ 子行 / 顺序 / 未建出的为 null ok');

// 提交列表有缓存:PR 那头多了一个提交,TTL 内再开切换器看到的还是旧列表;清掉缓存才重拉
const scopeCache = (manager as unknown as { scopeCommits: Map<string, unknown> }).scopeCommits;
process.env.SPIKE_COMMIT_TOTAL = '4';
try {
assert.equal((await manager.listScopes(container.id)).commits.length, 3, 'TTL 内命中缓存');
scopeCache.clear();
assert.equal((await manager.listScopes(container.id)).commits.length, 4, '失效后重拉');
} finally {
scopeCache.clear();
delete process.env.SPIKE_COMMIT_TOTAL;
}
log('listScopes 提交列表缓存 ok');

// 非 github-pr 与子行都不该有范围
const local = store.createReview({ source: 'local-branch', sourceRef: 'feat/x' });
await assert.rejects(() => manager.listScopes(local.id), /GitHub PR/);
Expand Down Expand Up @@ -281,7 +308,6 @@ async function main() {
headline: 'feat: step 2',
index: 2,
total: 3,
capped: false,
prevHeadline: 'feat: step 1',
nextHeadline: 'feat: step 3',
};
Expand All @@ -299,7 +325,7 @@ async function main() {
// 列表被截断时说不出第几个,但仍要交代这是单个提交
const cappedPrompt = buildScanPrompt({
pr: null,
position: { ...position, index: null, total: 250, capped: true, prevHeadline: null, nextHeadline: null },
position: { ...position, index: null, total: 260, prevHeadline: null, nextHeadline: null },
})!;
assert.ok(cappedPrompt.includes('## 本次审核范围'));
assert.ok(!cappedPrompt.includes('第 2/3'), '拿不到位置就别编一个');
Expand All @@ -316,6 +342,36 @@ async function main() {
await src.dispose();
log('prepare 回带 position ok');

// ---- 8b. 几百个提交:列表从最新一页倒着翻、封顶后截掉的是最早那段,位置序号把截掉的算上 ----
const { fetchPrCommits } = await import('../src/backend/source/github-pr-source');
process.env.SPIKE_COMMIT_TOTAL = '260';
try {
const big = await fetchPrCommits('acme/repo', '42');
assert.equal(big.total, 260);
assert.equal(big.commits.length, PR_COMMITS_CAP, '凑够封顶值就停');
assert.equal(big.commits[0].headline, 'feat: step 11', '截掉的是最早的 10 个');
assert.equal(big.commits[PR_COMMITS_CAP - 1].headline, 'feat: step 260', '最新的一个在末尾');
assert.equal(big.commits[0].oid, oidAt(10));
// 钉在列表里的第一条(全 PR 第 11 个):序号按总数算,前一个提交在截掉的那段里、给不出
const pinnedIn = new source({ source: 'github-pr', ref: 'acme/repo#42', repoPath: '', headRef: oidAt(10) });
const inList = await pinnedIn.prepare();
assert.equal(inList.position?.index, 11);
assert.equal(inList.position?.total, 260);
assert.equal(inList.position?.prevHeadline, null);
assert.equal(inList.position?.nextHeadline, 'feat: step 12');
await pinnedIn.dispose();
// 钉在被截掉那段里的提交:不在列表里不算 force-push 失效,走 compare 兜底,拿得到标题、给不出序号
const pinnedOut = new source({ source: 'github-pr', ref: 'acme/repo#42', repoPath: '', headRef: SHAS[1] });
const outList = await pinnedOut.prepare();
assert.equal(outList.position?.index, null);
assert.equal(outList.position?.headline, 'feat: step 2');
assert.equal(outList.position?.total, 260);
await pinnedOut.dispose();
} finally {
delete process.env.SPIKE_COMMIT_TOTAL;
}
log('封顶截断走最新一段 ok');

// ---- 9. 删 PR:子范围的会话也要拆 ----
//
// 库里的行随父级联删,但会话是进程内的活物,级联碰不到它。桩打在 teardown 下面那一层
Expand Down
4 changes: 1 addition & 3 deletions src/backend/prompt/scan-prompt.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@ export interface ScanPromptInput {
* 只讲位置,不注入 PR 级 findings:那些锚在整个 PR 的行号上,与这一份 diff 不是同一套基准。
*/
function scopeSection(p: CommitPosition): string[] {
// 截断时总数只是下界,`+` 两支都要带:index 拿得到也不代表分母是真的
const total = p.capped ? `${p.total}+` : `${p.total}`;
const at = p.index ? `第 ${p.index}/${total} 个提交` : `其中一个提交(共 ${total} 个)`;
const at = p.index ? `第 ${p.index}/${p.total} 个提交` : `其中一个提交(共 ${p.total} 个)`;
const out = [
'## 本次审核范围',
`本次只审 PR 的${at} \`${p.sha.slice(0, 7)}\` · ${p.headline};下面这份 diff 是它相对其父提交的改动,不是整个 PR 的。`,
Expand Down
22 changes: 21 additions & 1 deletion src/backend/review/review-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,9 @@ export function describeRoundFailure(cause: unknown): {
* main 侧 review 编排入口:持久化 + 活跃 ReviewSession,把领域事件归一成 IPC ReviewEvent 外发。
* IPC 层订阅本类 'review-event' 转发到 renderer(见 backend/ipc)。
*/
/** 容器提交列表的缓存时限;屏幕开着期间 PR 有新 push,最多这么久后切换器能看到。 */
const SCOPE_COMMITS_TTL_MS = 60_000;

export class ReviewManager extends EventEmitter {
/** 活跃会话;按 Map 插入序当 LRU,访问时 touch 到队尾。 */
private readonly sessions = new Map<string, ReviewSession>();
Expand All @@ -211,6 +214,12 @@ export class ReviewManager extends EventEmitter {
* 见 {@link teardownEpoch}。不清理:代次归零会让在途的旧代次重新对上,反而放它登记回来。
*/
private readonly teardowns = new Map<string, number>();
/**
* 各 PR 容器的提交列表缓存。切换器每次打开都要重算各范围的机审状态(本地库,毫秒级),
* 但提交列表走 gh 要 1~3 次往返,而它只在 PR 被 push 时才变 —— 容器重跑机审时清掉
* (那是唯一会拿到新 head 的路径),再加 TTL 兜住屏幕开着期间的新 push。
*/
private readonly scopeCommits = new Map<string, { commits: PrCommit[]; at: number }>();
private readonly maxLiveSessions: number;
/** 已预留、会话还没建出来的位子数,见 {@link reserveCapacity}。 */
private pendingSessions = 0;
Expand Down Expand Up @@ -777,6 +786,7 @@ export class ReviewManager extends EventEmitter {
const ids = [...this.store.listChildren(reviewId).map((c) => c.id), reviewId];
for (const id of ids) await this.teardown(id);
this.store.deleteReview(reviewId);
this.scopeCommits.delete(reviewId);
// 第一次 teardown 释放会话要 await,那期间来的追问读到的还是删除前的行,会照常续接上来。
// 再拆一次收掉它:此后新来的续接第一步就查不到 review,这条路到此为止。
for (const id of ids) await this.teardown(id);
Expand Down Expand Up @@ -1031,7 +1041,7 @@ export class ReviewManager extends EventEmitter {
*/
async listScopes(parentId: string): Promise<ReviewScopes> {
const parent = this.requireContainer(parentId);
const commits = await listPrCommits(parent.sourceRef, parent.repoPath ?? undefined);
const commits = await this.prCommitsOf(parent);
const byHead = new Map(this.store.listChildren(parentId).map((c) => [c.headRef ?? '', c]));
return {
pr: this.scopeStateOf(parent),
Expand All @@ -1055,6 +1065,14 @@ export class ReviewManager extends EventEmitter {
};
}

private async prCommitsOf(parent: Review): Promise<PrCommit[]> {
const hit = this.scopeCommits.get(parent.id);
if (hit && Date.now() - hit.at < SCOPE_COMMITS_TTL_MS) return hit.commits;
const commits = await listPrCommits(parent.sourceRef, parent.repoPath ?? undefined);
this.scopeCommits.set(parent.id, { commits, at: Date.now() });
return commits;
}

/**
* 切到 PR 里某个 commit 的范围。已建过就直接返回;没有则现拉它相对父提交的 diff 建一条子 review。
*
Expand Down Expand Up @@ -1263,6 +1281,8 @@ export class ReviewManager extends EventEmitter {
// 每轮新 thread:先彻底释放上一轮的会话、MCP 与 source,再重建。
opts.onStage?.('resolve');
await this.teardown(reviewId);
// 重跑会按 PR 当前 head 重新 prepare,提交列表可能已经不是缓存里那份
this.scopeCommits.delete(reviewId);
// 代次在 teardown 之后取:本轮要作废的是这次拆完之后又来的释放/删除。
const epoch = this.teardownEpoch(reviewId);
// 刚腾出的位子立刻占住:下面还要 await 好几步拉取,期间被别的发起抢走的话,
Expand Down
Loading