fix: keep PR actions from wrapping the title (#2670)
This commit is contained in:
@@ -159,16 +159,20 @@ export function ChangeRequestDetailHeader({
|
||||
const state = detail.draft && detail.state === "open" ? "draft" : detail.state;
|
||||
return (
|
||||
<div className="space-y-2 p-3">
|
||||
<div className="flex items-start gap-2">
|
||||
<div className="flex flex-wrap items-start gap-2">
|
||||
<a
|
||||
data-testid="change-request-detail-title"
|
||||
href={detail.url}
|
||||
target="_blank"
|
||||
rel="noopener noreferrer"
|
||||
className="min-w-0 flex-1 truncate text-sm font-medium hover:underline"
|
||||
className="min-w-0 grow basis-auto break-words text-sm font-medium hover:underline"
|
||||
>
|
||||
{detail.title}
|
||||
</a>
|
||||
<div className="flex flex-wrap justify-end gap-1.5">
|
||||
<div
|
||||
data-testid="change-request-detail-actions"
|
||||
className="flex min-w-0 max-w-full shrink flex-wrap justify-start gap-1.5"
|
||||
>
|
||||
{actions.map((action) => (
|
||||
<ChangeRequestActionButton
|
||||
key={action.id}
|
||||
|
||||
@@ -1,5 +1,14 @@
|
||||
import { expect, type Locator } from "@playwright/test";
|
||||
|
||||
export type ElementBox = { x: number; y: number; width: number; height: number };
|
||||
|
||||
export async function requireBox(locator: Locator, label: string): Promise<ElementBox> {
|
||||
const box = await locator.boundingBox();
|
||||
expect(box, `${label}: locator has no bounding box`).not.toBeNull();
|
||||
if (!box) throw new Error(`${label}: locator has no bounding box`);
|
||||
return box;
|
||||
}
|
||||
|
||||
// Shared layout assertions for mobile / responsive specs. Extracted because
|
||||
// both onboarding mobile specs need the same overflow + padding checks and
|
||||
// duplicating the DOM walk caused review churn.
|
||||
@@ -135,6 +144,48 @@ export async function assertNoElementHorizontalOverflow(
|
||||
).toBeLessThanOrEqual(widths.client + 1);
|
||||
}
|
||||
|
||||
/**
|
||||
* Asserts that text occupies multiple rendered lines without overflowing its
|
||||
* own horizontal content box. Useful for titles whose complete text must stay
|
||||
* readable instead of being clipped with an ellipsis.
|
||||
*/
|
||||
export async function assertTextWrapsNaturallyWithoutHorizontalOverflow(
|
||||
locator: Locator,
|
||||
label = "text",
|
||||
): Promise<void> {
|
||||
const metrics = await locator.evaluate((node) => {
|
||||
const style = getComputedStyle(node);
|
||||
const range = document.createRange();
|
||||
range.selectNodeContents(node);
|
||||
const lineWidths = Array.from(range.getClientRects(), (rect) => rect.width).filter(
|
||||
(width) => width > 0,
|
||||
);
|
||||
return {
|
||||
height: node.getBoundingClientRect().height,
|
||||
lineHeight: Number.parseFloat(style.lineHeight),
|
||||
scrollWidth: node.scrollWidth,
|
||||
clientWidth: node.clientWidth,
|
||||
lineWidths,
|
||||
};
|
||||
});
|
||||
expect(
|
||||
metrics.height,
|
||||
`${label}: height (${metrics.height}) does not span multiple ${metrics.lineHeight}px lines`,
|
||||
).toBeGreaterThan(metrics.lineHeight * 1.5);
|
||||
expect(
|
||||
metrics.scrollWidth,
|
||||
`${label}: scrollWidth (${metrics.scrollWidth}) exceeds clientWidth (${metrics.clientWidth})`,
|
||||
).toBeLessThanOrEqual(metrics.clientWidth + 1);
|
||||
const [firstLineWidth] = metrics.lineWidths;
|
||||
if (firstLineWidth === undefined) {
|
||||
throw new Error(`${label}: text has no rendered line rectangles`);
|
||||
}
|
||||
expect(
|
||||
firstLineWidth,
|
||||
`${label}: first line should consume available width before wrapping`,
|
||||
).toBeGreaterThan(metrics.clientWidth * 0.8);
|
||||
}
|
||||
|
||||
/**
|
||||
* Asserts that a visible locator fits inside the viewport horizontally.
|
||||
* Useful for popovers that portal outside their dialog/container.
|
||||
|
||||
@@ -2,6 +2,8 @@ import { test, expect } from "../../fixtures/test-base";
|
||||
import {
|
||||
assertLocatorWithinViewportX,
|
||||
assertNoDocumentHorizontalOverflow,
|
||||
assertTextWrapsNaturallyWithoutHorizontalOverflow,
|
||||
requireBox,
|
||||
} from "../../helpers/layout-assertions";
|
||||
import { SessionPage } from "../../pages/session-page";
|
||||
|
||||
@@ -9,6 +11,8 @@ const OWNER = "testorg";
|
||||
const REPO = "testrepo";
|
||||
const PR_NUMBER = 418;
|
||||
const REVIEWER = "reviewer-with-a-near-maximum-practical-github-login";
|
||||
const MOBILE_PR_TITLE =
|
||||
"Keep this long pull request title visible while review and merge controls wrap below it at narrow widths";
|
||||
const SWITCH_PR_NUMBER = 420;
|
||||
const SWITCH_SECOND_PR_NUMBER = 421;
|
||||
|
||||
@@ -33,23 +37,39 @@ test.describe("mobile PR re-request review", () => {
|
||||
repository_ids: [seedData.repositoryId],
|
||||
},
|
||||
);
|
||||
await apiClient.mockGitHubAssociateTaskPR({
|
||||
const mergeReadyPR = {
|
||||
task_id: task.id,
|
||||
owner: OWNER,
|
||||
repo: REPO,
|
||||
pr_number: PR_NUMBER,
|
||||
pr_url: `https://github.com/${OWNER}/${REPO}/pull/${PR_NUMBER}`,
|
||||
pr_title: "Mobile re-request dismissed review",
|
||||
pr_title: MOBILE_PR_TITLE,
|
||||
head_branch: "feat/mobile-rerequest-review",
|
||||
base_branch: "main",
|
||||
author_login: "another-user",
|
||||
state: "open",
|
||||
});
|
||||
review_state: "approved",
|
||||
checks_state: "success",
|
||||
mergeable_state: "clean",
|
||||
review_count: 1,
|
||||
pending_review_count: 0,
|
||||
required_reviews: 1,
|
||||
checks_total: 1,
|
||||
checks_passing: 1,
|
||||
};
|
||||
await apiClient.mockGitHubAssociateTaskPR(mergeReadyPR);
|
||||
await apiClient.mockGitHubSeedPRFeedback({
|
||||
owner: OWNER,
|
||||
repo: REPO,
|
||||
pr_number: PR_NUMBER,
|
||||
checks: [{ name: "Frontend tests", status: "completed", conclusion: "success" }],
|
||||
reviews: [
|
||||
{
|
||||
id: 2,
|
||||
author: "code-owner",
|
||||
state: "APPROVED",
|
||||
created_at: "2026-07-23T09:00:00Z",
|
||||
},
|
||||
{
|
||||
id: 1,
|
||||
author: REVIEWER,
|
||||
@@ -73,14 +93,56 @@ test.describe("mobile PR re-request review", () => {
|
||||
await expect(panel).toBeVisible({ timeout: 15_000 });
|
||||
await assertLocatorWithinViewportX(panel, "mobile PR review panel");
|
||||
|
||||
const detail = panel.getByTestId("change-request-detail");
|
||||
const title = detail.getByTestId("change-request-detail-title");
|
||||
const actions = detail.getByTestId("change-request-detail-actions");
|
||||
const approve = detail.getByTestId("pr-approve-button");
|
||||
const merge = detail.getByTestId("pr-merge-button");
|
||||
const action = session.prReRequestReviewButton(REVIEWER);
|
||||
await expect(title).toBeVisible();
|
||||
await expect(approve).toBeVisible();
|
||||
await expect(action).toBeVisible();
|
||||
const box = await action.boundingBox();
|
||||
expect(box, "re-request review action has no bounding box").not.toBeNull();
|
||||
if (box) {
|
||||
expect(box.width, "re-request review action width").toBeGreaterThanOrEqual(44);
|
||||
expect(box.height, "re-request review action height").toBeGreaterThanOrEqual(44);
|
||||
}
|
||||
// Feedback correctly marks a dismissed reviewer as pending. Re-pin the
|
||||
// stored projection so this regression fixture exposes every header action
|
||||
// while retaining the dismissed-review flow below it.
|
||||
await apiClient.mockGitHubAssociateTaskPR(mergeReadyPR);
|
||||
await expect(merge).toBeVisible({ timeout: 15_000 });
|
||||
const [detailBox, titleBox, actionsBox, approveBox, mergeBox] = await Promise.all([
|
||||
requireBox(detail, "mobile detail"),
|
||||
requireBox(title, "mobile title"),
|
||||
requireBox(actions, "mobile action cluster"),
|
||||
requireBox(approve, "mobile approve action"),
|
||||
requireBox(merge, "mobile merge action"),
|
||||
]);
|
||||
expect(actionsBox.y, "mobile action cluster should sit below the title").toBeGreaterThanOrEqual(
|
||||
titleBox.y + titleBox.height,
|
||||
);
|
||||
expect(approveBox.y, "mobile approve action should sit below the title").toBeGreaterThanOrEqual(
|
||||
titleBox.y + titleBox.height,
|
||||
);
|
||||
expect(mergeBox.y, "mobile merge action should follow approval").toBeGreaterThanOrEqual(
|
||||
approveBox.y,
|
||||
);
|
||||
expect(
|
||||
mergeBox.y > approveBox.y || mergeBox.x > approveBox.x,
|
||||
"mobile merge action should preserve visual action order",
|
||||
).toBe(true);
|
||||
expect(titleBox.width, "mobile title should own the full padded row").toBeGreaterThanOrEqual(
|
||||
detailBox.width - 25,
|
||||
);
|
||||
expect(approveBox.x, "mobile actions should share the title's leading edge").toBeCloseTo(
|
||||
titleBox.x,
|
||||
0,
|
||||
);
|
||||
expect(approveBox.height, "mobile approve action height").toBeGreaterThanOrEqual(44);
|
||||
expect(mergeBox.height, "mobile merge action height").toBeGreaterThanOrEqual(44);
|
||||
await assertTextWrapsNaturallyWithoutHorizontalOverflow(title, "mobile PR title");
|
||||
await assertLocatorWithinViewportX(approve, "mobile approve action");
|
||||
await assertLocatorWithinViewportX(merge, "mobile merge action");
|
||||
|
||||
const box = await requireBox(action, "re-request review action");
|
||||
expect(box.width, "re-request review action width").toBeGreaterThanOrEqual(44);
|
||||
expect(box.height, "re-request review action height").toBeGreaterThanOrEqual(44);
|
||||
await assertLocatorWithinViewportX(action, "mobile re-request review action");
|
||||
const reRequestResponse = testPage.waitForResponse(
|
||||
(response) =>
|
||||
|
||||
@@ -1,10 +1,20 @@
|
||||
import { expect, type Page } from "@playwright/test";
|
||||
import { expect, type Locator, type Page } from "@playwright/test";
|
||||
import { test, type SeedData } from "../../fixtures/test-base";
|
||||
import type { ApiClient } from "../../helpers/api-client";
|
||||
import { getDockviewGroupWidth, resizeColumnViaSplitview } from "../../helpers/dockview-resize";
|
||||
import {
|
||||
assertTextWrapsNaturallyWithoutHorizontalOverflow,
|
||||
requireBox,
|
||||
type ElementBox,
|
||||
} from "../../helpers/layout-assertions";
|
||||
import { SessionPage } from "../../pages/session-page";
|
||||
|
||||
const DONE_STATES = ["COMPLETED", "WAITING_FOR_INPUT"];
|
||||
const PR_NUMBER = 701;
|
||||
const RESPONSIVE_PR_NUMBER = 702;
|
||||
const RESPONSIVE_PR_TITLE =
|
||||
"Keep this long pull request title visible while review and merge controls wrap below it at narrow widths";
|
||||
const RESPONSIVE_PR_URL = `https://github.com/testorg/testrepo/pull/${RESPONSIVE_PR_NUMBER}`;
|
||||
|
||||
type ReviewLayout = {
|
||||
canonicalGroupId: string | null;
|
||||
@@ -110,6 +120,59 @@ function sessionTabWrapper(page: Page, sessionId: string) {
|
||||
});
|
||||
}
|
||||
|
||||
function verticalOverlap(first: ElementBox, second: ElementBox): number {
|
||||
return Math.min(first.y + first.height, second.y + second.height) - Math.max(first.y, second.y);
|
||||
}
|
||||
|
||||
async function textLineCount(locator: Locator): Promise<number> {
|
||||
return locator.evaluate((element) => {
|
||||
const range = document.createRange();
|
||||
range.selectNodeContents(element);
|
||||
return Array.from(range.getClientRects()).filter((rect) => rect.width > 0).length;
|
||||
});
|
||||
}
|
||||
|
||||
async function singleLineTextWidth(locator: Locator, label: string): Promise<number> {
|
||||
const widths = await locator.evaluate((element) => {
|
||||
const range = document.createRange();
|
||||
range.selectNodeContents(element);
|
||||
return Array.from(range.getClientRects(), (rect) => rect.width).filter((width) => width > 0);
|
||||
});
|
||||
expect(widths, `${label} should render on exactly one line`).toHaveLength(1);
|
||||
const [width] = widths;
|
||||
if (width === undefined) throw new Error(`${label}: text has no rendered line rectangles`);
|
||||
return width;
|
||||
}
|
||||
|
||||
async function resizeReviewDetailTo(page: Page, targetWidth: number): Promise<void> {
|
||||
const detail = page.getByTestId("change-request-detail");
|
||||
const currentDetailWidth = await detail.evaluate((element) =>
|
||||
Math.round(element.getBoundingClientRect().width),
|
||||
);
|
||||
const currentRightWidth = await getDockviewGroupWidth(page, "files");
|
||||
await resizeColumnViaSplitview(
|
||||
page,
|
||||
"right",
|
||||
Math.round(currentRightWidth + currentDetailWidth - targetWidth),
|
||||
);
|
||||
await expect
|
||||
.poll(() => detail.evaluate((element) => Math.round(element.getBoundingClientRect().width)), {
|
||||
message: `change request detail did not settle at ${targetWidth}px`,
|
||||
})
|
||||
.toBe(targetWidth);
|
||||
}
|
||||
|
||||
async function expectCenterHit(locator: Locator, label: string): Promise<void> {
|
||||
await expect(
|
||||
locator.evaluate((element) => {
|
||||
const rect = element.getBoundingClientRect();
|
||||
const hit = document.elementFromPoint(rect.x + rect.width / 2, rect.y + rect.height / 2);
|
||||
return hit === element || (hit !== null && element.contains(hit));
|
||||
}),
|
||||
`${label} center is not a usable hit target`,
|
||||
).resolves.toBe(true);
|
||||
}
|
||||
|
||||
test.describe("PR Details layout panel", () => {
|
||||
test("adds linked review content beside Agent without changing focus", async ({
|
||||
testPage,
|
||||
@@ -235,4 +298,143 @@ test.describe("PR Details layout panel", () => {
|
||||
);
|
||||
await expect(agentTab).not.toHaveClass(/dv-active-tab/);
|
||||
});
|
||||
|
||||
test("moves PR actions below before they force the title to wrap", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
seedData,
|
||||
}) => {
|
||||
test.setTimeout(120_000);
|
||||
await testPage.setViewportSize({ width: 1600, height: 900 });
|
||||
await apiClient.mockGitHubReset();
|
||||
await apiClient.mockGitHubSetUser("test-user");
|
||||
const task = await createTaskWithSession(apiClient, seedData, "Responsive PR Details header");
|
||||
await apiClient.mockGitHubAssociateTaskPR({
|
||||
task_id: task.id,
|
||||
owner: "testorg",
|
||||
repo: "testrepo",
|
||||
pr_number: RESPONSIVE_PR_NUMBER,
|
||||
pr_url: RESPONSIVE_PR_URL,
|
||||
pr_title: RESPONSIVE_PR_TITLE,
|
||||
head_branch: "feat/responsive-pr-header",
|
||||
base_branch: "main",
|
||||
author_login: "another-user",
|
||||
state: "open",
|
||||
review_state: "approved",
|
||||
checks_state: "success",
|
||||
mergeable_state: "clean",
|
||||
review_count: 1,
|
||||
pending_review_count: 0,
|
||||
required_reviews: 1,
|
||||
checks_total: 1,
|
||||
checks_passing: 1,
|
||||
});
|
||||
await apiClient.mockGitHubSeedPRFeedback({
|
||||
owner: "testorg",
|
||||
repo: "testrepo",
|
||||
pr_number: RESPONSIVE_PR_NUMBER,
|
||||
checks: [
|
||||
{
|
||||
name: "Frontend tests",
|
||||
status: "completed",
|
||||
conclusion: "success",
|
||||
},
|
||||
],
|
||||
reviews: [
|
||||
{
|
||||
id: 1,
|
||||
author: "code-owner",
|
||||
state: "APPROVED",
|
||||
created_at: "2026-08-14T10:00:00Z",
|
||||
},
|
||||
],
|
||||
});
|
||||
await expect
|
||||
.poll(() => apiClient.getTaskPR(task.id), {
|
||||
message: "responsive PR fixture did not reach merge-ready state",
|
||||
})
|
||||
.toMatchObject({
|
||||
state: "open",
|
||||
review_state: "approved",
|
||||
checks_state: "success",
|
||||
mergeable_state: "clean",
|
||||
review_count: 1,
|
||||
required_reviews: 1,
|
||||
});
|
||||
|
||||
const session = await openTask(testPage, task.id);
|
||||
await expect(session.prDetailTab()).toBeVisible({ timeout: 15_000 });
|
||||
await session.prDetailTab().click();
|
||||
const detail = testPage.getByTestId("change-request-detail");
|
||||
const title = detail.getByTestId("change-request-detail-title");
|
||||
const actions = detail.getByTestId("change-request-detail-actions");
|
||||
const approve = detail.getByTestId("pr-approve-button");
|
||||
const merge = detail.getByTestId("pr-merge-button");
|
||||
const refresh = detail.getByRole("button", { name: "Refresh" });
|
||||
await expect(title).toBeVisible();
|
||||
await expect(approve).toBeVisible();
|
||||
await expect(merge).toBeVisible();
|
||||
await expect(refresh).toBeVisible();
|
||||
|
||||
await resizeReviewDetailTo(testPage, 1200);
|
||||
const [wideDetailBox, wideTitleBox, wideActionsBox, wideTitleTextWidth] = await Promise.all([
|
||||
requireBox(detail, "wide detail"),
|
||||
requireBox(title, "wide title"),
|
||||
requireBox(actions, "wide actions"),
|
||||
singleLineTextWidth(title, "wide title"),
|
||||
]);
|
||||
expect(await textLineCount(title), "wide title should fit on one line").toBe(1);
|
||||
expect(
|
||||
verticalOverlap(wideTitleBox, wideActionsBox),
|
||||
"actions may stay inline when the title remains one line",
|
||||
).toBeGreaterThan(0);
|
||||
|
||||
const inlineOverhead = wideDetailBox.width - wideTitleBox.width - wideActionsBox.width;
|
||||
const squeezedWidth = Math.ceil(wideTitleTextWidth + inlineOverhead + wideActionsBox.width / 2);
|
||||
await resizeReviewDetailTo(testPage, squeezedWidth);
|
||||
const [squeezedDetailBox, squeezedTitleBox, squeezedActionsBox] = await Promise.all([
|
||||
requireBox(detail, "squeezed detail"),
|
||||
requireBox(title, "squeezed title"),
|
||||
requireBox(actions, "squeezed actions"),
|
||||
]);
|
||||
expect(
|
||||
await textLineCount(title),
|
||||
"actions should move below before they force the title onto another line",
|
||||
).toBe(1);
|
||||
expect(
|
||||
squeezedActionsBox.y,
|
||||
"squeezed actions should sit below the title",
|
||||
).toBeGreaterThanOrEqual(squeezedTitleBox.y + squeezedTitleBox.height);
|
||||
expect(
|
||||
squeezedTitleBox.width,
|
||||
"squeezed title should recover the full row",
|
||||
).toBeGreaterThanOrEqual(squeezedDetailBox.width - 25);
|
||||
|
||||
await resizeReviewDetailTo(testPage, 600);
|
||||
const [detailBox, titleBox, approveBox, mergeBox, refreshBox] = await Promise.all([
|
||||
requireBox(detail, "narrow detail"),
|
||||
requireBox(title, "narrow title"),
|
||||
requireBox(approve, "narrow approve action"),
|
||||
requireBox(merge, "narrow merge action"),
|
||||
requireBox(refresh, "narrow refresh action"),
|
||||
]);
|
||||
expect(approveBox.y, "approval should start below the title").toBeGreaterThanOrEqual(
|
||||
titleBox.y + titleBox.height,
|
||||
);
|
||||
expect(mergeBox.y, "merge should share the action row").toBeCloseTo(approveBox.y, 0);
|
||||
expect(refreshBox.y, "refresh should share the action row").toBeCloseTo(approveBox.y, 0);
|
||||
expect(titleBox.width, "title should own the full padded row").toBeGreaterThanOrEqual(
|
||||
detailBox.width - 25,
|
||||
);
|
||||
await assertTextWrapsNaturallyWithoutHorizontalOverflow(title, "narrow PR title");
|
||||
expect(approveBox.x, "narrow actions should share the title's leading edge").toBeCloseTo(
|
||||
titleBox.x,
|
||||
0,
|
||||
);
|
||||
expect(approveBox.x).toBeLessThan(mergeBox.x);
|
||||
expect(mergeBox.x).toBeLessThan(refreshBox.x);
|
||||
expect(refreshBox.x + refreshBox.width).toBeLessThanOrEqual(detailBox.x + detailBox.width);
|
||||
await expectCenterHit(approve, "approve action");
|
||||
await expectCenterHit(merge, "merge action");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -0,0 +1,157 @@
|
||||
---
|
||||
spec: docs/specs/pr-detail-header-width/spec.md
|
||||
created: 2026-08-14
|
||||
status: done
|
||||
---
|
||||
|
||||
# Implementation Plan: Responsive PR Detail Header
|
||||
|
||||
## Overview
|
||||
|
||||
Make the provider-neutral change-request header respond to its own content and
|
||||
rendered width instead of allowing its action cluster to consume the title.
|
||||
Use intrinsic flex wrapping so actions share the row only with a single-line
|
||||
title, and prove the behavior in desktop and mobile PR flows.
|
||||
|
||||
## Confirmed root cause
|
||||
|
||||
`ChangeRequestDetailHeader` renders the title and every header action in one
|
||||
non-wrapping flex row. The title is the only shrinking item, so GitHub's
|
||||
**Approve as...**, **Squash and merge**, optional merge-method trigger, and
|
||||
refresh controls can reduce it to almost no width. Browser breakpoints cannot
|
||||
solve this because a Dockview PR Details panel can be narrow inside a wide
|
||||
desktop viewport.
|
||||
|
||||
The first title-wrapping follow-up used `text-wrap: balance` and kept the
|
||||
stacked action cluster right-aligned. At intermediate widths, balancing breaks
|
||||
the title early into similarly sized lines while the actions form a separate
|
||||
right-aligned island below it. The intended composition needs normal line flow
|
||||
and a shared leading edge for title and controls.
|
||||
|
||||
The subsequent fixed 640px container query still permits a long title to wrap
|
||||
beside the action cluster at any width above that threshold. Width alone cannot
|
||||
encode the invariant because title text, localization, authenticated actor, and
|
||||
visible action labels all change the combined intrinsic width.
|
||||
|
||||
## Frontend
|
||||
|
||||
### Responsive header composition
|
||||
|
||||
- In
|
||||
`apps/web/components/integrations/change-request-detail-header.tsx`, give the
|
||||
title and action cluster stable test IDs for rendered geometry checks.
|
||||
- Make the primary header row a wrapping flex row driven by intrinsic content
|
||||
width, with no fixed container or viewport breakpoint.
|
||||
- Keep the title's automatic flex basis so line collection measures its
|
||||
unwrapped content. Let it grow across a line when the action cluster moves to
|
||||
the next flex line.
|
||||
- Let the action cluster shrink only when it owns its flex line, so its controls
|
||||
can wrap internally on phones without forcing the title to shrink beside it.
|
||||
- Remove single-line truncation from the title. Let it wrap naturally, with
|
||||
normal greedy line filling and long-word protection so title text stays
|
||||
inside its content box. Do not balance title lines.
|
||||
- Move the complete existing action cluster together, including normalized
|
||||
provider actions, GitHub `headerActions`, and refresh. Preserve action order,
|
||||
handlers, pending and disabled states, accessible names, and button sizing.
|
||||
- Add no new copy, state, API calls, or provider-specific layout branch.
|
||||
|
||||
### Mobile design contract
|
||||
|
||||
- **Desktop outcome:** title/actions remain inline only when the complete title
|
||||
stays on one line; otherwise actions move below before the title loses width.
|
||||
- **Mobile entry point:** the existing **Review** bottom-navigation item opens
|
||||
`MobileReviewPanel`; navigation does not change.
|
||||
- **Nearest shipped exemplar:**
|
||||
`apps/web/components/task/mobile/mobile-review-panel.tsx` remains the focused,
|
||||
full-height Review surface and supplies the existing review selector.
|
||||
- **Hierarchy and primary actions:** review identity stays first; approval,
|
||||
merge, provider actions, and refresh remain in a wrapping cluster directly
|
||||
below it when narrow.
|
||||
- **Presentation rationale:** these are frequent, already-visible review
|
||||
controls, so an inline second row is clearer than an overflow drawer or new
|
||||
route.
|
||||
- **Geometry:** the header remains fixed above
|
||||
`change-request-detail-scroll`, that ScrollArea remains the only vertical
|
||||
scroll owner, safe-area handling is unchanged, and phone buttons retain their
|
||||
existing 44px minimum height.
|
||||
- **Shared logic:** provider data, eligibility, mutations, and selection remain
|
||||
shared; only header presentation responds to container width.
|
||||
- **Mobile proof:** extend the existing 320px mobile PR scenario to assert title
|
||||
wrapping and placement, action hit areas and containment, and zero
|
||||
document-level horizontal overflow.
|
||||
|
||||
## Tests
|
||||
|
||||
No unit test is appropriate for the new behavior because jsdom does not
|
||||
evaluate browser flex wrapping or rendered geometry. Existing
|
||||
`change-request-detail.test.tsx` interaction coverage remains unchanged; the
|
||||
responsive contract is tested in Chromium.
|
||||
|
||||
## E2E Tests
|
||||
|
||||
- **Scenario:** a wide desktop viewport shows title/actions inline when their
|
||||
intrinsic widths fit, moves actions below before they would wrap the title,
|
||||
then lets the full-width title wrap at a narrower panel width.
|
||||
- **File:** `apps/web/e2e/tests/pr/pr-detail-layout.spec.ts`
|
||||
- **Method:** seed a merge-ready PR authored by another user so both GitHub
|
||||
actions render, resize the center group indirectly through the existing
|
||||
`resizeColumnViaSplitview` right-column helper, and assert a single-line
|
||||
inline state, a squeezed single-line title with actions below, and a narrow
|
||||
full-width wrapping title with actions below.
|
||||
- **Scenario:** the 320px phone Review surface places approval and squash-merge
|
||||
actions below the title while preserving the existing re-request flow.
|
||||
- **File:**
|
||||
`apps/web/e2e/tests/pr/mobile-pr-rerequest-review.spec.ts`
|
||||
- **Method:** make the existing first PR fixture merge-ready, assert title and
|
||||
action geometry plus 44px touch targets before re-requesting the dismissed
|
||||
review, then retain the existing no-horizontal-overflow assertion.
|
||||
|
||||
## Verification Results
|
||||
|
||||
- Content-aware RED: at the initial 1000px detail width, the fixed container
|
||||
query kept actions inline and forced the title onto two lines; the regression
|
||||
expected one full-width title line with actions below.
|
||||
- Desktop Chromium passed 1/1 after intrinsic flex wrapping. The scenario
|
||||
proves a 1200px inline single-line state, a content-derived squeezed
|
||||
single-line title with actions below, and a 600px full-width wrapping title
|
||||
with actions below.
|
||||
- CI follow-up replaced the fixed squeezed width with one derived from rendered
|
||||
title and action sizes so font metrics cannot move the test across the flex
|
||||
boundary; desktop Chromium passed 1/1.
|
||||
- Mobile Chromium passed 1/1, preserving 44px action targets, natural title
|
||||
wrapping, the dismissed-review re-request flow, and zero document horizontal
|
||||
overflow.
|
||||
- Live isolated rendering matched the three desktop geometries: at 1200px the
|
||||
title and actions shared `y=92`; at 1000px the 976px one-line title ended
|
||||
before actions at `y=120`; at 600px the 576px two-line title ended before
|
||||
actions at `y=140`.
|
||||
- Live 320px rendering kept the three-line 296px title above the action cluster,
|
||||
with document `scrollWidth` equal to its 320px `clientWidth`.
|
||||
- Targeted ESLint and Prettier checks passed; `git diff --check` was clean and
|
||||
no fixed change-request breakpoint classes remained.
|
||||
|
||||
## Implementation Waves And Parallel Candidates
|
||||
|
||||
Wave 1:
|
||||
|
||||
- [x] [Task 01: Make the PR detail header panel-responsive](task-01-panel-responsive-header.md)
|
||||
|
||||
Execution is sequential in the primary conversation. No subagent delegation is
|
||||
planned or authorized.
|
||||
|
||||
## Risks
|
||||
|
||||
- The title must keep `flex-basis: auto`; a zero basis would let actions remain
|
||||
inline while the title shrinks and wraps, recreating the regression.
|
||||
- The action cluster needs `min-width: 0` and a bounded shrink path so it can
|
||||
wrap its own controls on phones after moving to a separate flex line.
|
||||
- Header actions appear asynchronously as GitHub status and merge-method data
|
||||
settle. Geometry assertions must wait for both named buttons instead of using
|
||||
a fixed delay.
|
||||
- Long unbroken title tokens must wrap inside the title content box rather than
|
||||
create horizontal overflow.
|
||||
- `ChangeRequestDetail` is a host-owned provider-neutral surface. Applying the
|
||||
responsive composition there intentionally keeps GitHub and compatible
|
||||
plugin review panels aligned with ADR
|
||||
`2026-08-06-plugin-code-host-dashboard-parity`; provider mutation behavior
|
||||
remains untouched.
|
||||
@@ -0,0 +1,117 @@
|
||||
---
|
||||
id: "01-panel-responsive-header"
|
||||
title: "Make the PR detail header panel-responsive"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: []
|
||||
plan: "plan.md"
|
||||
spec: "../../specs/pr-detail-header-width/spec.md"
|
||||
---
|
||||
|
||||
# Task 01: Make the PR Detail Header Panel-Responsive
|
||||
|
||||
## Acceptance
|
||||
|
||||
- The action cluster shares the title row only when the complete title remains
|
||||
one line; otherwise it moves below and returns the full row to the title.
|
||||
- Long titles wrap to show their complete text instead of clipping with an
|
||||
ellipsis, on desktop and phone Review surfaces. Lines fill the available
|
||||
title width normally instead of balancing into similarly sized rows.
|
||||
- At narrow widths, the action cluster starts at the title's leading edge
|
||||
instead of floating independently on the trailing side.
|
||||
- The transition uses intrinsic content width rather than a fixed breakpoint,
|
||||
and crossing it loses or remounts no action.
|
||||
- The 320px phone Review flow retains touch-usable actions, one scroll owner,
|
||||
the existing re-request outcome, and no document-level horizontal overflow.
|
||||
|
||||
## TDD sequence
|
||||
|
||||
1. Add a desktop geometry regression covering inline-fit, squeezed, and
|
||||
full-width wrapping states. Confirm RED at a width where actions force a
|
||||
title that fits alone onto two lines.
|
||||
2. Replace the fixed container query with intrinsic flex wrapping while keeping
|
||||
natural title wrapping, long-token protection, action order, and leading
|
||||
alignment.
|
||||
3. Run the focused desktop scenario until green, then reuse its production
|
||||
build for the focused `mobile-chrome` scenario.
|
||||
4. Refactor only if the minimal class change exposes duplication; rerun both
|
||||
scenarios after any refactor.
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/web/components/integrations/change-request-detail.tsx`
|
||||
- `apps/web/components/integrations/change-request-detail-header.tsx`
|
||||
- `apps/web/e2e/tests/pr/pr-detail-layout.spec.ts`
|
||||
- `apps/web/e2e/tests/pr/mobile-pr-rerequest-review.spec.ts`
|
||||
- `docs/plans/pr-detail-header-width/plan.md`
|
||||
- `docs/plans/pr-detail-header-width/task-01-panel-responsive-header.md`
|
||||
|
||||
## Verification
|
||||
|
||||
Run from the repository root:
|
||||
|
||||
```bash
|
||||
cd apps && \
|
||||
pnpm install --frozen-lockfile && \
|
||||
pnpm --filter @kandev/web lint -- components/integrations/change-request-detail.tsx components/integrations/change-request-detail-header.tsx e2e/helpers/layout-assertions.ts e2e/tests/pr/pr-detail-layout.spec.ts e2e/tests/pr/mobile-pr-rerequest-review.spec.ts && \
|
||||
cd web && \
|
||||
pnpm e2e:run --host --project chromium tests/pr/pr-detail-layout.spec.ts -- --grep "moves PR actions below before they force the title to wrap" --workers=1 && \
|
||||
pnpm e2e:run --host --no-build --project mobile-chrome tests/pr/mobile-pr-rerequest-review.spec.ts -- --grep "uses bottom-nav Review to re-request a dismissed review without overflow" --workers=1
|
||||
```
|
||||
|
||||
The first managed E2E run rebuilds production frontend and backend artifacts;
|
||||
the second reuses those unchanged artifacts. Confirm each command discovers
|
||||
one intended test before accepting it as evidence.
|
||||
|
||||
## Dependencies
|
||||
|
||||
None.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`. Production layout and both geometry regressions define one
|
||||
coupled responsive contract and belong in one Red-Green-Refactor cycle.
|
||||
|
||||
## Inputs
|
||||
|
||||
- Scenarios in `docs/specs/pr-detail-header-width/spec.md`.
|
||||
- Frontend and mobile contracts in `plan.md`.
|
||||
- Shared header implementation in
|
||||
`apps/web/components/integrations/change-request-detail-header.tsx`.
|
||||
- Dockview resize helpers in `apps/web/e2e/helpers/dockview-resize.ts`.
|
||||
- Existing desktop and phone PR setup in the two owning E2E specs.
|
||||
|
||||
## Risks
|
||||
|
||||
- Preserve the title's automatic flex basis; `flex-1` would use a zero basis
|
||||
and let the action cluster force title wrapping on the same row.
|
||||
- Let the action cluster shrink within the available row after it wraps below,
|
||||
otherwise phone controls can overflow horizontally.
|
||||
- Wait for both GitHub actions through observable locators. Do not add sleeps or
|
||||
increase timeouts to hide asynchronous setup.
|
||||
- Keep long-title content inside its content box while allowing natural
|
||||
multiline layout.
|
||||
|
||||
## Output contract
|
||||
|
||||
Report changed files, observed RED failures, final commands and test counts,
|
||||
rendered evidence, blockers and risks, plus synchronized task and plan statuses
|
||||
in this conversation.
|
||||
|
||||
## Results
|
||||
|
||||
- RED: at the initial 1000px detail width, actions stayed inline and forced the
|
||||
title onto two lines instead of moving below.
|
||||
- GREEN: intrinsic flex wrapping passed desktop Chromium 1/1 across 1200px,
|
||||
a content-derived squeezed width, and 600px detail widths. Actions remain
|
||||
inline only in the first single-line-fit state; squeezed and wrapping states
|
||||
give the title a full row.
|
||||
- Mobile Chromium passed 1/1 while retaining 44px targets, action order,
|
||||
re-request behavior, natural wrapping, and zero horizontal overflow.
|
||||
- CI follow-up replaced the fixed squeezed width with a content-derived width
|
||||
so font metrics cannot move the test across the flex boundary; desktop
|
||||
Chromium passed 1/1.
|
||||
- Live rendering confirmed the 1200px inline, 1000px stacked single-line, 600px
|
||||
stacked multiline, and 320px phone states. Targeted ESLint and Prettier
|
||||
passed, `git diff --check` was clean, and the fixed container-query classes
|
||||
were absent.
|
||||
@@ -174,6 +174,7 @@ Subscription quota tracking and per-agent cheap-model profile routing.
|
||||
| [ci-pr-automation](ui/ci-pr-automation.md) | building |
|
||||
| [github-pr-review-actions](ui/github-pr-review-actions.md) | shipped |
|
||||
| [github-saved-query-defaults](ui/github-saved-query-defaults.md) | shipped |
|
||||
| [pr-detail-header-width](pr-detail-header-width/spec.md) | shipped |
|
||||
| [pr-task-status-summary](ui/pr-task-status-summary.md) | shipped |
|
||||
| [comment-markdown](ui/comment-markdown.md) | shipped |
|
||||
| [resizable-markdown-tables](ui/resizable-markdown-tables.md) | building |
|
||||
|
||||
@@ -0,0 +1,67 @@
|
||||
---
|
||||
status: shipped
|
||||
created: 2026-08-14
|
||||
owner: Kandev frontend
|
||||
---
|
||||
|
||||
# Responsive PR Detail Header
|
||||
|
||||
## Why
|
||||
|
||||
The PR Details panel can be much narrower than the browser viewport. When
|
||||
header actions consume that width, the pull-request title collapses until the
|
||||
review being shown is no longer identifiable.
|
||||
|
||||
## What
|
||||
|
||||
- The shared change-request detail header keeps the complete review title
|
||||
readable at every supported panel width. Long titles wrap within their
|
||||
available title area instead of clipping with an ellipsis. Wrapping follows
|
||||
normal line flow: each line uses its available width before text continues
|
||||
on the next line; the title is not balanced into equal-length lines.
|
||||
- Header actions, including GitHub's **Approve as...** and **Squash and merge**
|
||||
controls, may share the title row only when the complete title remains on one
|
||||
line beside them.
|
||||
- When the title and action cluster do not fit together, the complete action
|
||||
cluster moves below the title. The title then owns the full row and wraps only
|
||||
when it is longer than that full width. There is no fixed pixel breakpoint;
|
||||
the transition follows actual title and action content.
|
||||
- A stacked action cluster starts at the same leading edge as the title and may
|
||||
wrap internally when needed.
|
||||
- Responsive behavior follows available change-request detail width,
|
||||
independent of browser viewport width, so resized desktop panels, localized
|
||||
action labels, and phone Review surfaces obey the same invariant.
|
||||
- Stacking preserves action order, labels, enabled and pending states, click
|
||||
behavior, accessible names, and existing touch-target sizing.
|
||||
- The header and its controls remain contained without document-level
|
||||
horizontal overflow.
|
||||
|
||||
## Scenarios
|
||||
|
||||
- **GIVEN** a desktop PR Details panel where the complete title and actions fit
|
||||
together, **WHEN** the header lays out, **THEN** the single-line title and
|
||||
action group appear inline and every control remains usable.
|
||||
- **GIVEN** the same title and actions in a squeezed panel where sharing a row
|
||||
would wrap the title, **WHEN** the header lays out, **THEN** the action cluster
|
||||
moves below and the title recovers the full row before wrapping.
|
||||
- **GIVEN** a title longer than the full available row, **WHEN** it wraps,
|
||||
**THEN** no header action occupies space to its right and all actions appear
|
||||
below from the same leading edge.
|
||||
- **GIVEN** a phone-sized task with a linked pull request, **WHEN** the user
|
||||
opens Review, **THEN** the title appears above the touch-usable action cluster,
|
||||
which may wrap internally, and the document has no horizontal overflow.
|
||||
- **GIVEN** panel width, title copy, localization, or visible action labels
|
||||
change, **WHEN** combined intrinsic width crosses the available width, **THEN**
|
||||
the same actions move between stacked and inline placement without being lost
|
||||
or reordered.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- Changing approval or merge eligibility, mutations, labels, or provider data.
|
||||
- Changing title copy, font size, or font weight.
|
||||
- Moving actions into an overflow menu or changing the PR Details panel's
|
||||
navigation, placement, scrolling, or persistence.
|
||||
|
||||
## Implementation plan
|
||||
|
||||
[Implementation plan](../../plans/pr-detail-header-width/plan.md)
|
||||
Reference in New Issue
Block a user