Skip to content
Merged
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
111 changes: 99 additions & 12 deletions apps/dashboard/src/components/pulls/detail/pull-detail-activity.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ import {
dismissPullReview,
getCommentPage,
getTimelineEventPage,
markPullReadyForReview,
mergePullRequest,
replyToReviewComment,
requestPullReviewers,
Expand Down Expand Up @@ -254,6 +255,8 @@ export function PullDetailActivitySection({
pullNumber={pullNumber}
prTitle={pr.title}
firstCommitMessage={commits?.[0]?.message}
isDraft={pr.isDraft}
pullId={pr.graphqlId}
/>
</div>
)}
Expand Down Expand Up @@ -328,13 +331,17 @@ function MergeStatusSection({
pullNumber,
prTitle,
firstCommitMessage,
isDraft,
pullId,
}: {
scope: GitHubQueryScope;
owner: string;
repo: string;
pullNumber: number;
prTitle: string;
firstCommitMessage?: string;
isDraft: boolean;
pullId: string | undefined;
}) {
const input = useMemo(
() => ({ owner, repo, pullNumber }),
Expand Down Expand Up @@ -389,6 +396,8 @@ function MergeStatusSection({
pullNumber={pullNumber}
prTitle={prTitle}
firstCommitMessage={firstCommitMessage}
isDraft={isDraft}
pullId={pullId}
/>
);
}
Expand Down Expand Up @@ -486,6 +495,8 @@ function MergeStatusCard({
pullNumber,
prTitle,
firstCommitMessage,
isDraft,
pullId,
}: {
scope: GitHubQueryScope;
status: PullStatus;
Expand All @@ -494,6 +505,8 @@ function MergeStatusCard({
pullNumber: number;
prTitle: string;
firstCommitMessage?: string;
isDraft: boolean;
pullId: string | undefined;
}) {
const {
checks,
Expand Down Expand Up @@ -585,18 +598,27 @@ function MergeStatusCard({
/>
)}

{/* Merge action footer */}
<MergeFooter
scope={scope}
isMergeBlocked={isMergeBlocked}
canMerge={canMerge}
bypass={bypass}
owner={owner}
repo={repo}
pullNumber={pullNumber}
prTitle={prTitle}
firstCommitMessage={firstCommitMessage}
/>
{/* Merge action footer (or "ready for review" CTA when draft) */}
{isDraft ? (
<ReadyForReviewFooter
owner={owner}
repo={repo}
pullNumber={pullNumber}
pullId={pullId}
/>
) : (
<MergeFooter
scope={scope}
isMergeBlocked={isMergeBlocked}
canMerge={canMerge}
bypass={bypass}
owner={owner}
repo={repo}
pullNumber={pullNumber}
prTitle={prTitle}
firstCommitMessage={firstCommitMessage}
/>
)}
</div>
);
}
Expand Down Expand Up @@ -1330,6 +1352,71 @@ function UpdateBranchButton({
);
}

// ── Ready for review footer ─────────────────────────────────────────

function ReadyForReviewFooter({
owner,
repo,
pullNumber,
pullId,
}: {
owner: string;
repo: string;
pullNumber: number;
pullId: string | undefined;
}) {
const [isMarking, setIsMarking] = useState(false);
const queryClient = useQueryClient();

const handleMarkReady = async () => {
if (!pullId) {
toast.error("Missing pull request id");
return;
}
setIsMarking(true);
try {
const result = await markPullReadyForReview({
data: { owner, repo, pullNumber, pullId },
});
if (result.ok) {
await queryClient.invalidateQueries({ queryKey: ["github"] });
} else {
toast.error(result.error);
checkPermissionWarning(result, `${owner}/${repo}`);
setIsMarking(false);
}
} catch {
toast.error("Failed to mark as ready for review");
setIsMarking(false);
}
};
Comment on lines +1328 to +1349
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.

⚠️ Potential issue | 🟠 Major

Reset loading state on success to prevent a permanently disabled CTA.

isMarking is never cleared on the success path. If the refetch/invalidation does not transition the view immediately, the button can stay disabled with a stuck spinner.

💡 Suggested fix
 const handleMarkReady = async () => {
   if (!pullId) {
     toast.error("Missing pull request id");
     return;
   }
   setIsMarking(true);
   try {
     const result = await markPullReadyForReview({
       data: { owner, repo, pullNumber, pullId },
     });
     if (result.ok) {
       await queryClient.invalidateQueries({ queryKey: ["github"] });
     } else {
       toast.error(result.error);
       checkPermissionWarning(result, `${owner}/${repo}`);
-      setIsMarking(false);
+      return;
     }
   } catch {
     toast.error("Failed to mark as ready for review");
-    setIsMarking(false);
+  } finally {
+    setIsMarking(false);
   }
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const handleMarkReady = async () => {
if (!pullId) {
toast.error("Missing pull request id");
return;
}
setIsMarking(true);
try {
const result = await markPullReadyForReview({
data: { owner, repo, pullNumber, pullId },
});
if (result.ok) {
await queryClient.invalidateQueries({ queryKey: ["github"] });
} else {
toast.error(result.error);
checkPermissionWarning(result, `${owner}/${repo}`);
setIsMarking(false);
}
} catch {
toast.error("Failed to mark as ready for review");
setIsMarking(false);
}
};
const handleMarkReady = async () => {
if (!pullId) {
toast.error("Missing pull request id");
return;
}
setIsMarking(true);
try {
const result = await markPullReadyForReview({
data: { owner, repo, pullNumber, pullId },
});
if (result.ok) {
await queryClient.invalidateQueries({ queryKey: ["github"] });
} else {
toast.error(result.error);
checkPermissionWarning(result, `${owner}/${repo}`);
return;
}
} catch {
toast.error("Failed to mark as ready for review");
} finally {
setIsMarking(false);
}
};
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/dashboard/src/components/pulls/detail/pull-detail-activity.tsx` around
lines 1328 - 1349, The loading flag is never cleared on success in
handleMarkReady, causing the CTA to remain disabled; update handleMarkReady to
reset the loading state (call setIsMarking(false)) after a successful
markPullReadyForReview and queryClient.invalidateQueries (or move the
setIsMarking(false) into a finally block) so the spinner/button is re-enabled
even if the refetch doesn't immediately update the view.


return (
<div className="flex items-center gap-3 px-4 py-3">
<Button
size="sm"
disabled={isMarking || !pullId}
onClick={() => {
void handleMarkReady();
}}
iconLeft={
isMarking ? (
<Spinner size={14} />
) : (
<GitPullRequestIcon size={14} strokeWidth={2} />
)
}
>
Ready for review
</Button>
<p className="text-xs text-muted-foreground">
This pull request is a draft. Mark it as ready for review to allow
merging.
</p>
</div>
);
}

// ── Merge footer ────────────────────────────────────────────────────

const MERGE_STRATEGIES = [
Expand Down
29 changes: 29 additions & 0 deletions apps/dashboard/src/lib/github.functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6402,6 +6402,35 @@ export const mergePullRequest = createServerFn({ method: "POST" })
}
});

export type ReadyForReviewInput = PullFromRepoInput & {
/** GraphQL node id of the pull request (e.g. `PR_kwD...`). */
pullId: string;
};

export const markPullReadyForReview = createServerFn({ method: "POST" })
.inputValidator(identityValidator<ReadyForReviewInput>)
.handler(async ({ data }): Promise<MutationResult> => {
const context = await getGitHubUserContextForRepository(data);
if (!context) {
return { ok: false, error: "Not authenticated" };
}

try {
await context.octokit.graphql(
`mutation($pullRequestId: ID!) {
markPullRequestReadyForReview(input: { pullRequestId: $pullRequestId }) {
pullRequest { isDraft }
}
}`,
{ pullRequestId: data.pullId },
);
await bustPullDetailCaches(context.session.user.id, data);
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.

⚠️ Potential issue | 🟠 Major

Bust pulls.pageData.graphql.v2 cache after marking ready-for-review.

At Line 6304, bustPullDetailCaches does not clear the cached pull page-data resource that contains detail.isDraft. This can leave stale draft state after the mutation and keep showing the draft CTA until cache expiry.

💡 Proposed fix
async function bustPullDetailCaches(userId: string, params: PullCacheParams) {
  await Promise.all([
+   bustGitHubCache(userId, "pulls.pageData.graphql.v2", params),
    bustGitHubCache(userId, "pulls.detail.raw", params),
    bustGitHubCache(userId, "pulls.status.raw", params),
    bustGitHubCache(userId, "pulls.status.v1", params),
    bustGitHubCache(userId, "pulls.status.v2", params),
    bustGitHubCache(userId, "pulls.status.v3", params),
  ]);
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/dashboard/src/lib/github.functions.ts` at line 6304, The
bustPullDetailCaches call currently misses invalidating the cached pull
page-data resource that contains detail.isDraft; update bustPullDetailCaches (or
add an explicit call right after await
bustPullDetailCaches(context.session.user.id, data)) to also clear the
pulls.pageData.graphql.v2 cache entry for the affected pull (use the same
identifier available in data — e.g., data.id or data.pullNumber and
context.session.user.id). Specifically, ensure the function invokes the cache
invalidation API used elsewhere (e.g.,
invalidateResource/cacheClient.invalidate/removeResource) for the resource key
"pulls.pageData.graphql.v2" scoped to that pull so the draft state is not served
stale.

return { ok: true };
} catch (error) {
return toMutationError("mark pull request as ready for review", error);
}
});

export const deleteBranch = createServerFn({ method: "POST" })
.inputValidator(
identityValidator<{
Expand Down
Loading