Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
45 changes: 23 additions & 22 deletions apps/web/src/components/pullRequest/PullRequestSummaryTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,11 @@ import { PullRequestReviewerPicker } from "./PullRequestReviewerPicker";
import { PullRequestActivityUnavailableState } from "./PullRequestActivityUnavailableState";
import {
latestPullRequestReviewOutcomes,
orderPullRequestComments,
pullRequestFindingKey,
pullRequestHiddenCommentsActionLabel,
pullRequestReviewOutcome,
visibleBody,
windowPullRequestComments,
type PullRequestFinding,
} from "./pullRequestDetail.logic";
import {
Expand Down Expand Up @@ -401,12 +402,26 @@ export function PullRequestSummaryTab({
// rather than wherever the last one had been read back to.
const [shown, setShown] = useState({ url: detail.url, count: COMMENT_PAGE });
const shownComments = shown.url === detail.url ? shown.count : COMMENT_PAGE;
// Windowed by recency regardless of display order: expanding always reaches further back in
// time, whether the newest comment currently reads first or last.
const recentComments = detail.comments.slice(Math.max(0, detail.comments.length - shownComments));
const hiddenCommentCount = detail.comments.length - recentComments.length;
const [commentOrder, setCommentOrder] = useState<"newest" | "oldest">("newest");
const visibleComments = orderPullRequestComments(recentComments, commentOrder);
const {
visible: visibleComments,
hiddenCount: hiddenCommentCount,
expandFrom,
} = windowPullRequestComments(detail.comments, shownComments, commentOrder);
// Hundreds of comments are hundreds of markdown renders, and the ones worth opening a pull
// request for are the recent ones. The rest are one press away and stay rendered once asked
// for. Newest-first puts that press after the visible list, where the oldest comments belong.
const showOldestCommentsButton =
hiddenCommentCount > 0 ? (
<Button
size="sm"
variant="outline"
className="w-full"
onClick={() => setShown({ url: detail.url, count: shownComments + COMMENT_PAGE })}
>
{pullRequestHiddenCommentsActionLabel(hiddenCommentCount, COMMENT_PAGE)}
</Button>
) : null;
// Read from the whole conversation, not the window shown below it: a verdict older than the
// last thirty comments still stands.
const reviewOutcomes = latestPullRequestReviewOutcomes(detail.comments, detail.commits);
Expand Down Expand Up @@ -782,22 +797,7 @@ export function PullRequestSummaryTab({
<p className="py-2 text-xs text-muted-foreground">No comments yet.</p>
) : (
<div className="space-y-3">
{hiddenCommentCount > 0 ? (
// Hundreds of comments are hundreds of markdown renders, and the ones worth
// opening a pull request for are the recent ones. The rest are one press away and
// stay rendered once asked for.
<Button
size="sm"
variant="outline"
className="w-full"
onClick={() =>
setShown({ url: detail.url, count: shownComments + COMMENT_PAGE })
}
>
Show {Math.min(hiddenCommentCount, COMMENT_PAGE)} earlier{" "}
{hiddenCommentCount === 1 ? "comment" : "comments"}
</Button>
) : null}
{expandFrom === "start" ? showOldestCommentsButton : null}
{visibleComments.map((comment) => {
const thread = threadByCommentId.get(comment.id);
const body = visibleBody(comment.body);
Expand Down Expand Up @@ -909,6 +909,7 @@ export function PullRequestSummaryTab({
</article>
);
})}
{expandFrom === "end" ? showOldestCommentsButton : null}
</div>
)}
</>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ import {
latestPullRequestReviewOutcomes,
newestPullRequestCommitAt,
mergePullRequestThreadComments,
windowPullRequestComments,
orderPullRequestComments,
pullRequestHiddenCommentsActionLabel,
pullRequestActionMenuHasGroup,
pullRequestActionNeedsHostRefresh,
pullRequestCheckoutCommand,
Expand Down Expand Up @@ -235,6 +237,40 @@ describe("ordering comments", () => {
// The source array is chronological input, not a mutation target.
expect(comments).toEqual([{ createdAt: "a" }, { createdAt: "b" }, { createdAt: "c" }]);
});

it("opens on the most recent comments and expands the oldest remainder after them when newest-first", () => {
const comments = [
{ createdAt: "1" },
{ createdAt: "2" },
{ createdAt: "3" },
{ createdAt: "4" },
];
expect(windowPullRequestComments(comments, 2, "newest")).toEqual({
visible: [{ createdAt: "4" }, { createdAt: "3" }],
hiddenCount: 2,
expandFrom: "end",
});
});

it("expands the same oldest remainder from the start when oldest-first", () => {
const comments = [
{ createdAt: "1" },
{ createdAt: "2" },
{ createdAt: "3" },
{ createdAt: "4" },
];
expect(windowPullRequestComments(comments, 2, "oldest")).toEqual({
visible: [{ createdAt: "3" }, { createdAt: "4" }],
hiddenCount: 2,
expandFrom: "start",
});
});

it("names the hidden remainder as oldest comments, not earlier ones", () => {
expect(pullRequestHiddenCommentsActionLabel(18, 30)).toBe("Show 18 oldest comments");
expect(pullRequestHiddenCommentsActionLabel(40, 30)).toBe("Show 30 oldest comments");
expect(pullRequestHiddenCommentsActionLabel(1, 30)).toBe("Show 1 oldest comment");
});
});

describe("review verdicts", () => {
Expand Down
32 changes: 32 additions & 0 deletions apps/web/src/components/pullRequest/pullRequestDetail.logic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,38 @@ export function orderPullRequestComments<T extends { readonly createdAt: string
return order === "newest" ? comments.toReversed() : comments;
}

/**
* A long conversation still opens on what happened last. The window is always the most recent
* comments; display order only decides whether those read newest-first or oldest-first, and which
* end of the list the remaining oldest comments expand from.
*/
export function windowPullRequestComments<T extends { readonly createdAt: string }>(
comments: ReadonlyArray<T>,
shownCount: number,
order: "newest" | "oldest",
): {
readonly visible: ReadonlyArray<T>;
readonly hiddenCount: number;
readonly expandFrom: "start" | "end";
} {
const recent = comments.slice(Math.max(0, comments.length - shownCount));
return {
visible: orderPullRequestComments(recent, order),
hiddenCount: comments.length - recent.length,
// Newest-first puts the oldest remainder after the visible list; oldest-first puts it before.
expandFrom: order === "newest" ? "end" : "start",
};
}

/** How many of the remaining oldest comments the next press reveals. */
export function pullRequestHiddenCommentsActionLabel(
hiddenCount: number,
pageSize: number,
): string {
const count = Math.min(hiddenCount, pageSize);
return `Show ${count} oldest ${count === 1 ? "comment" : "comments"}`;
}

/** A review that says something about the change itself, rather than only carrying remarks. */
export type PullRequestReviewOutcome = "approved" | "changes-requested" | "dismissed";

Expand Down
Loading