Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions apps/code/src/main/di/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,11 @@ container
getWorkspace: (taskId) => workspace().getWorkspace(taskId),
linkBranch: (taskId, branch, source) =>
workspace().linkBranch(taskId, branch, source),
accumulatePrUrl: (taskId, prUrl) => {
void ctx
.get<IGitPrStatus>(GIT_PR_STATUS_PROVIDER)
.accumulatePrUrl(taskId, prUrl);
},
Comment on lines +587 to +591

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 PR persistence is fire-and-forget

When the app shuts down after PR creation while getPrDetailsByUrl is still pending, this discarded promise reaches the closed database and rejects without being handled. The create-PR flow has already reported success, but the new PR URL is never persisted and disappears from the task after restart.

Rule Used: Always wrap asynchronous calls that may fail, such... (source)

Learned From
PostHog/posthog#32098

Knowledge Base Used:

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/di/container.ts
Line: 587-591

Comment:
**PR persistence is fire-and-forget**

When the app shuts down after PR creation while `getPrDetailsByUrl` is still pending, this discarded promise reaches the closed database and rejects without being handled. The create-PR flow has already reported success, but the new PR URL is never persisted and disappears from the task after restart.

**Rule Used:** Always wrap asynchronous calls that may fail, such... ([source](https://app.greptile.com/posthog-org-19734/-/custom-context?memory=df81f021-48c3-45e4-981f-7348133f5f7a))

**Learned From**
[PostHog/posthog#32098](https://github.com/PostHog/posthog/pull/32098)

**Knowledge Base Used:**
- [PostHog Code Desktop App (`apps/code`)](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/code/-/docs/apps-code-desktop.md)
- [workspace-server](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/code/-/docs/workspace-server.md)

How can I resolve this? If you propose a fix, please make it concise.

};
});
container.load(gitHostModule);
Expand Down
5 changes: 5 additions & 0 deletions packages/core/src/git-pr/git-pr.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ function makeHost(over: Partial<CreatePrHost> = {}): CreatePrHost {
prUrl: "https://github.com/o/r/pull/1",
}),
linkBranch: vi.fn(),
accumulatePrUrl: vi.fn(),
getPrState: vi.fn().mockResolvedValue({ prStatus: "open" }),
...over,
};
Expand Down Expand Up @@ -163,6 +164,10 @@ describe("GitPrService.createPr", () => {
});
expect(host.push).toHaveBeenCalledWith("/repo", undefined);
expect(host.linkBranch).toHaveBeenCalledWith("task-1", "feature", "user");
expect(host.accumulatePrUrl).toHaveBeenCalledWith(
"task-1",
"https://github.com/o/r/pull/1",
);
expect(onProgress).toHaveBeenLastCalledWith(
"complete",
"Pull request created",
Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/git-pr/git-pr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,9 @@ Rules:
if (linkedBranch) {
host.linkBranch(input.taskId, linkedBranch, "user");
}
if (result.data.prUrl) {
host.accumulatePrUrl(input.taskId, result.data.prUrl);
}
}

onProgress(
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/git-pr/identifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,5 +100,6 @@ export interface CreatePrHost {
env?: Record<string, string>,
): Promise<{ success: boolean; message: string; prUrl: string | null }>;
linkBranch(taskId: string, branch: string, source: "user"): void;
accumulatePrUrl(taskId: string, prUrl: string): void;
getPrState(directoryPath: string): Promise<unknown>;
}
2 changes: 2 additions & 0 deletions packages/core/src/git/git-host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,8 @@ export class GitHostService extends TypedEventEmitter<GitServiceEvents> {
}),
linkBranch: (taskId, branch, source) =>
this.workspaceLookup.linkBranch(taskId, branch, source),
accumulatePrUrl: (taskId, prUrl) =>
this.workspaceLookup.accumulatePrUrl(taskId, prUrl),
getPrState: (dir) => this.getPrStateSnapshot(dir),
};
}
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/git/host-git.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,5 @@ export interface GitPrWorkspaceInfo {
export interface GitWorkspaceLookup {
getWorkspace(taskId: string): Promise<GitPrWorkspaceInfo | null>;
linkBranch(taskId: string, branch: string, source: "user"): void;
accumulatePrUrl(taskId: string, prUrl: string): void;
}
1 change: 1 addition & 0 deletions packages/host-router/src/ports/git-pr-status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,5 @@ export interface IGitPrStatus {
): Promise<TaskPrStatus>;
getCachedPrUrl(taskId: string): CachedPrUrlOutput;
setPrimaryPrUrl(taskId: string, prUrl: string): Promise<void>;
accumulatePrUrl(taskId: string, prUrl: string): Promise<void>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ describe("resolveTaskPrUrls", () => {
{ cloudUrls: [PR_1], cachedUrls: [PR_1, PR_2], currentBranchUrl: PR_1 },
{ primaryUrl: PR_1, otherUrls: [PR_2] },
],
[
"two accumulated local PRs surface an Other PRs entry",
{ cloudUrls: [], cachedUrls: [PR_1, PR_2], currentBranchUrl: PR_2 },
{ primaryUrl: PR_1, otherUrls: [PR_2] },
],
[
"dedupes across sources preserving cloud order",
{
Expand Down
2 changes: 1 addition & 1 deletion packages/workspace-server/src/services/agent/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2228,7 +2228,7 @@ For git operations while detached:
this.fetchGhLogin(session.repoPath),
]);
if (!wasCreatedRecently(attribution.createdAt, Date.now())) return;
if (!wasCreatedByLogin(attribution.author, ghLogin)) return;
if (ghLogin && !wasCreatedByLogin(attribution.author, ghLogin)) return;

this.log.info("Detected PR URL created during run", { taskRunId, prUrl });

Expand Down
86 changes: 86 additions & 0 deletions packages/workspace-server/src/services/git/task-pr-status.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -274,3 +274,89 @@ describe("TaskPrStatusService.setPrimaryPrUrl", () => {
});
});
});

describe("TaskPrStatusService.accumulatePrUrl", () => {
const PR_A = "https://github.com/acme/repo/pull/1";
const PR_B = "https://github.com/acme/repo/pull/2";

function makeService(existingUrls: string[]) {
const urls = [...existingUrls];
const gitService = {
getPrDetailsByUrl: vi
.fn()
.mockResolvedValue({ state: "open", merged: false, draft: false }),
} as unknown as GitService;
const workspaceService = { emit: vi.fn() };
const workspaceRepo = {
findByTaskId: vi.fn().mockReturnValue({ id: "ws-1" }),
updatePrCache: vi.fn((_taskId, update) => {
if (update.prUrl && update.accumulate && !urls.includes(update.prUrl)) {
urls.push(update.prUrl);
}
}),
getPrUrls: vi.fn(() => [...urls]),
};
const service = new TaskPrStatusService(
gitService,
workspaceRepo as unknown as IWorkspaceRepository,
workspaceService as unknown as WorkspaceService,
);
return { service, gitService, workspaceService, workspaceRepo };
}

it("records the first PR as a fresh accumulate and emits it", async () => {
const { service, workspaceService, workspaceRepo } = makeService([]);

await service.accumulatePrUrl("task-1", PR_A);

expect(workspaceRepo.updatePrCache).toHaveBeenCalledWith("task-1", {
prUrl: PR_A,
prState: "open",
accumulate: true,
});
expect(workspaceService.emit).toHaveBeenCalledWith("taskPrInfoChanged", {
taskId: "task-1",
prUrl: PR_A,
prUrls: [PR_A],
prState: "open",
});
});

it("appends a second distinct PR so both survive", async () => {
const { service, workspaceService } = makeService([PR_A]);

await service.accumulatePrUrl("task-1", PR_B);

expect(workspaceService.emit).toHaveBeenCalledWith("taskPrInfoChanged", {
taskId: "task-1",
prUrl: PR_B,
prUrls: [PR_A, PR_B],
prState: "open",
});
});

it("is idempotent when re-accumulating an existing PR", async () => {
const { service, workspaceService } = makeService([PR_A]);

await service.accumulatePrUrl("task-1", PR_A);

expect(workspaceService.emit).toHaveBeenCalledWith("taskPrInfoChanged", {
taskId: "task-1",
prUrl: PR_A,
prUrls: [PR_A],
prState: "open",
});
});

it("does nothing when the task has no workspace row", async () => {
const { service, gitService, workspaceService, workspaceRepo } =
makeService([]);
workspaceRepo.findByTaskId.mockReturnValue(null);

await service.accumulatePrUrl("task-1", PR_A);

expect(gitService.getPrDetailsByUrl).not.toHaveBeenCalled();
expect(workspaceRepo.updatePrCache).not.toHaveBeenCalled();
expect(workspaceService.emit).not.toHaveBeenCalled();
});
});
21 changes: 21 additions & 0 deletions packages/workspace-server/src/services/git/task-pr-status.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,27 @@ export class TaskPrStatusService {
});
}

async accumulatePrUrl(taskId: string, prUrl: string): Promise<void> {
if (!this.workspaceRepo.findByTaskId(taskId)) return;
const details = await this.gitService
.getPrDetailsByUrl(prUrl)
.catch(() => null);
Comment on lines +74 to +76

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 PR creation order is lost

When two PRs are created for the same task before either details lookup finishes, their URLs are appended in lookup-completion order rather than creation order. Since the UI treats prUrls[0] as primary, the second-created PR is displayed as the primary badge when its lookup resolves first.

Knowledge Base Used: workspace-server

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/workspace-server/src/services/git/task-pr-status.ts
Line: 74-76

Comment:
**PR creation order is lost**

When two PRs are created for the same task before either details lookup finishes, their URLs are appended in lookup-completion order rather than creation order. Since the UI treats `prUrls[0]` as primary, the second-created PR is displayed as the primary badge when its lookup resolves first.

**Knowledge Base Used:** [workspace-server](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/code/-/docs/workspace-server.md)

How can I resolve this? If you propose a fix, please make it concise.

const prState: SidebarPrState = details
? mapPrState(details.state, details.merged, details.draft)
: null;
this.workspaceRepo.updatePrCache(taskId, {
prUrl,
prState,
accumulate: true,
});
this.workspaceService.emit("taskPrInfoChanged", {
taskId,
prUrl,
prUrls: this.workspaceRepo.getPrUrls(taskId),
prState,
});
}

private async computeWorktreeHasDiff(taskId: string): Promise<boolean> {
const workspace = await this.workspaceService.getWorkspace(taskId);
if (
Expand Down
Loading