Skip to content

Commit 5cdbb83

Browse files
watany-devwatany-dev
authored
Merge pull request #90 from watany-dev/review-codex
Fix stale TUI state races and make filtered pagination accurate
2 parents fc168b0 + 59ed03c commit 5cdbb83

7 files changed

Lines changed: 951 additions & 83 deletions

File tree

‎src/app.test.tsx‎

Lines changed: 317 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,16 @@ import {
5151

5252
const mockClient = {} as any;
5353

54+
function createDeferred<T>() {
55+
let resolve!: (value: T) => void;
56+
let reject!: (reason?: unknown) => void;
57+
const promise = new Promise<T>((res, rej) => {
58+
resolve = res;
59+
reject = rej;
60+
});
61+
return { promise, resolve, reject };
62+
}
63+
5464
describe("App", () => {
5565
beforeEach(() => {
5666
vi.resetAllMocks();
@@ -1168,6 +1178,130 @@ describe("App", () => {
11681178
});
11691179
});
11701180

1181+
it("ignores stale approval and reaction loads from a previous PR detail", async () => {
1182+
const approvalDeferred = createDeferred<any[]>();
1183+
const reactionDeferred = createDeferred<any>();
1184+
1185+
vi.mocked(listPullRequests).mockResolvedValue({
1186+
pullRequests: [
1187+
{
1188+
pullRequestId: "42",
1189+
title: "first pr",
1190+
authorArn: "arn:aws:iam::123456789012:user/watany",
1191+
creationDate: new Date("2026-02-13T10:00:00Z"),
1192+
status: "OPEN" as const,
1193+
},
1194+
],
1195+
});
1196+
vi.mocked(getPullRequestDetail)
1197+
.mockResolvedValueOnce({
1198+
pullRequest: {
1199+
pullRequestId: "42",
1200+
title: "first pr",
1201+
revisionId: "rev-1",
1202+
pullRequestTargets: [],
1203+
},
1204+
differences: [],
1205+
commentThreads: [
1206+
{
1207+
location: null,
1208+
comments: [
1209+
{
1210+
commentId: "shared-comment",
1211+
authorArn: "arn:aws:iam::123456789012:user/taro",
1212+
content: "stale comment",
1213+
},
1214+
],
1215+
},
1216+
],
1217+
})
1218+
.mockResolvedValueOnce({
1219+
pullRequest: {
1220+
pullRequestId: "43",
1221+
title: "second pr",
1222+
revisionId: "rev-2",
1223+
pullRequestTargets: [],
1224+
},
1225+
differences: [],
1226+
commentThreads: [
1227+
{
1228+
location: null,
1229+
comments: [
1230+
{
1231+
commentId: "shared-comment",
1232+
authorArn: "arn:aws:iam::123456789012:user/hanako",
1233+
content: "current comment",
1234+
},
1235+
],
1236+
},
1237+
],
1238+
});
1239+
vi.mocked(getApprovalStates)
1240+
.mockImplementationOnce(() => approvalDeferred.promise as Promise<any[]>)
1241+
.mockResolvedValueOnce([]);
1242+
vi.mocked(evaluateApprovalRules).mockResolvedValue(null);
1243+
vi.mocked(getReactionsForComments)
1244+
.mockImplementationOnce(() => reactionDeferred.promise as Promise<any>)
1245+
.mockResolvedValueOnce(new Map());
1246+
1247+
const { lastFrame, stdin } = render(<App client={mockClient} initialRepo="my-service" />);
1248+
1249+
await vi.waitFor(() => {
1250+
expect(lastFrame()).toContain("first pr");
1251+
});
1252+
1253+
stdin.write("\r");
1254+
await vi.waitFor(() => {
1255+
expect(lastFrame()).toContain("PR #42");
1256+
});
1257+
1258+
stdin.write("q");
1259+
await vi.waitFor(() => {
1260+
expect(lastFrame()).toContain("first pr");
1261+
});
1262+
1263+
await new Promise((resolve) => setTimeout(resolve, 0));
1264+
vi.mocked(listPullRequests).mockResolvedValueOnce({
1265+
pullRequests: [
1266+
{
1267+
pullRequestId: "43",
1268+
title: "second pr",
1269+
authorArn: "arn:aws:iam::123456789012:user/hanako",
1270+
creationDate: new Date("2026-02-14T10:00:00Z"),
1271+
status: "CLOSED" as const,
1272+
},
1273+
],
1274+
});
1275+
stdin.write("f");
1276+
await vi.waitFor(() => {
1277+
expect(lastFrame()).toContain("second pr");
1278+
expect(lastFrame()).toContain("[Closed]");
1279+
});
1280+
stdin.write("\r");
1281+
await vi.waitFor(() => {
1282+
expect(lastFrame()).toContain("PR #43");
1283+
expect(lastFrame()).toContain("current comment");
1284+
expect(lastFrame()).not.toContain("taro");
1285+
expect(lastFrame()).not.toContain("👍×1");
1286+
});
1287+
1288+
approvalDeferred.resolve([
1289+
{ userArn: "arn:aws:iam::123456789012:user/taro", approvalState: "APPROVE" },
1290+
]);
1291+
reactionDeferred.resolve(
1292+
new Map([
1293+
["shared-comment", [{ emoji: "👍", shortCode: ":thumbsup:", count: 1, userArns: [] }]],
1294+
]),
1295+
);
1296+
1297+
await vi.waitFor(() => {
1298+
expect(lastFrame()).toContain("PR #43");
1299+
expect(lastFrame()).toContain("current comment");
1300+
expect(lastFrame()).not.toContain("taro");
1301+
expect(lastFrame()).not.toContain("👍×1");
1302+
});
1303+
});
1304+
11711305
it("approves PR successfully and reloads approval states", async () => {
11721306
vi.mocked(listPullRequests).mockResolvedValue({
11731307
pullRequests: [
@@ -3479,6 +3613,105 @@ describe("App", () => {
34793613
});
34803614
});
34813615

3616+
it("ignores stale commit diff results when switching commits quickly", async () => {
3617+
const firstCommitDeferred = createDeferred<any[]>();
3618+
3619+
vi.mocked(listPullRequests).mockResolvedValue({
3620+
pullRequests: [
3621+
{
3622+
pullRequestId: "42",
3623+
title: "fix: login",
3624+
authorArn: "arn:aws:iam::123456789012:user/watany",
3625+
creationDate: new Date("2026-02-13T10:00:00Z"),
3626+
status: "OPEN" as const,
3627+
},
3628+
],
3629+
});
3630+
vi.mocked(getPullRequestDetail).mockResolvedValue({
3631+
pullRequest: {
3632+
pullRequestId: "42",
3633+
title: "fix: login",
3634+
pullRequestTargets: [
3635+
{
3636+
sourceCommit: "src123",
3637+
destinationCommit: "dest456",
3638+
mergeBase: "base789",
3639+
},
3640+
],
3641+
},
3642+
differences: [],
3643+
commentThreads: [],
3644+
});
3645+
vi.mocked(getCommitsForPR).mockResolvedValue([
3646+
{
3647+
commitId: "c1",
3648+
shortId: "c1short",
3649+
message: "First commit",
3650+
authorName: "watany",
3651+
authorDate: new Date("2026-02-13T09:00:00Z"),
3652+
parentIds: ["base789"],
3653+
},
3654+
{
3655+
commitId: "c2",
3656+
shortId: "c2short",
3657+
message: "Second commit",
3658+
authorName: "watany",
3659+
authorDate: new Date("2026-02-13T10:00:00Z"),
3660+
parentIds: ["c1"],
3661+
},
3662+
]);
3663+
vi.mocked(getCommitDifferences)
3664+
.mockImplementationOnce(() => firstCommitDeferred.promise as Promise<any[]>)
3665+
.mockResolvedValueOnce([
3666+
{
3667+
beforeBlob: { blobId: "commit2-before", path: "src/commit2.ts" },
3668+
afterBlob: { blobId: "commit2-after", path: "src/commit2.ts" },
3669+
},
3670+
]);
3671+
vi.mocked(getBlobContent).mockImplementation(async (_client, _repo, blobId) => {
3672+
if (blobId === "commit1-before") return "old first";
3673+
if (blobId === "commit1-after") return "new first";
3674+
if (blobId === "commit2-before") return "old second";
3675+
if (blobId === "commit2-after") return "new second";
3676+
return "";
3677+
});
3678+
3679+
const { lastFrame, stdin } = render(<App client={mockClient} initialRepo="my-service" />);
3680+
await vi.waitFor(() => {
3681+
expect(lastFrame()).toContain("fix: login");
3682+
});
3683+
stdin.write("\r");
3684+
await vi.waitFor(() => {
3685+
expect(lastFrame()).toContain("PR #42");
3686+
});
3687+
3688+
stdin.write("\t");
3689+
await vi.waitFor(() => {
3690+
expect(lastFrame()).toContain("[Commit 1/2]");
3691+
});
3692+
3693+
stdin.write("\t");
3694+
await vi.waitFor(() => {
3695+
expect(lastFrame()).toContain("[Commit 2/2]");
3696+
});
3697+
await vi.waitFor(() => {
3698+
expect(lastFrame()).toContain("+new second");
3699+
});
3700+
3701+
firstCommitDeferred.resolve([
3702+
{
3703+
beforeBlob: { blobId: "commit1-before", path: "src/commit1.ts" },
3704+
afterBlob: { blobId: "commit1-after", path: "src/commit1.ts" },
3705+
},
3706+
]);
3707+
3708+
await vi.waitFor(() => {
3709+
expect(lastFrame()).toContain("[Commit 2/2]");
3710+
expect(lastFrame()).toContain("+new second");
3711+
expect(lastFrame()).not.toContain("+new first");
3712+
});
3713+
});
3714+
34823715
it("reloads comments without commit IDs when target has no commits", async () => {
34833716
vi.mocked(listPullRequests).mockResolvedValue({
34843717
pullRequests: [
@@ -4207,6 +4440,90 @@ describe("App", () => {
42074440
});
42084441
});
42094442

4443+
it("fills MERGED results from later CLOSED pages", async () => {
4444+
const openPage = {
4445+
pullRequests: [
4446+
{
4447+
pullRequestId: "42",
4448+
title: "fix: login",
4449+
authorArn: "arn:aws:iam::123456789012:user/watany",
4450+
creationDate: new Date("2026-02-13T10:00:00Z"),
4451+
status: "OPEN" as const,
4452+
},
4453+
],
4454+
};
4455+
const closedPage = {
4456+
pullRequests: [
4457+
{
4458+
pullRequestId: "35",
4459+
title: "fix: typos",
4460+
authorArn: "arn:aws:iam::123456789012:user/hanako",
4461+
creationDate: new Date("2026-02-09T10:00:00Z"),
4462+
status: "CLOSED" as const,
4463+
},
4464+
],
4465+
nextToken: "page-2",
4466+
};
4467+
const mergedPage = {
4468+
pullRequests: [
4469+
{
4470+
pullRequestId: "40",
4471+
title: "feat: auth",
4472+
authorArn: "arn:aws:iam::123456789012:user/watany",
4473+
creationDate: new Date("2026-02-11T10:00:00Z"),
4474+
status: "MERGED" as const,
4475+
},
4476+
],
4477+
};
4478+
let phase: "initial" | "closed-filter" | "merged-filter" = "initial";
4479+
4480+
vi.mocked(listPullRequests).mockImplementation(async (_client, _repo, token, apiStatus) => {
4481+
if (apiStatus === "OPEN") {
4482+
return openPage;
4483+
}
4484+
if (phase === "closed-filter") {
4485+
phase = "merged-filter";
4486+
return closedPage;
4487+
}
4488+
if (phase === "merged-filter" && token === undefined) {
4489+
return closedPage;
4490+
}
4491+
if (phase === "merged-filter" && token === "page-2") {
4492+
return mergedPage;
4493+
}
4494+
return { pullRequests: [] };
4495+
});
4496+
4497+
const { lastFrame, stdin } = render(<App client={mockClient} initialRepo="my-service" />);
4498+
await vi.waitFor(() => {
4499+
expect(lastFrame()).toContain("fix: login");
4500+
});
4501+
4502+
phase = "closed-filter";
4503+
stdin.write("f");
4504+
await vi.waitFor(() => {
4505+
expect(lastFrame()).toContain("[Closed]");
4506+
});
4507+
4508+
stdin.write("f");
4509+
await vi.waitFor(() => {
4510+
expect(lastFrame()).toContain("[Merged]");
4511+
expect(lastFrame()).toContain("feat: auth");
4512+
expect(lastFrame()).not.toContain("fix: typos");
4513+
});
4514+
expect(
4515+
vi
4516+
.mocked(listPullRequests)
4517+
.mock.calls.some(
4518+
([calledClient, repoName, pageToken, apiStatus]) =>
4519+
calledClient === mockClient &&
4520+
repoName === "my-service" &&
4521+
pageToken === "page-2" &&
4522+
apiStatus === "CLOSED",
4523+
),
4524+
).toBe(true);
4525+
});
4526+
42104527
// v0.8: Pagination integration tests
42114528
it("navigates to next page with n key", async () => {
42124529
vi.mocked(listPullRequests).mockResolvedValueOnce({

0 commit comments

Comments
 (0)