From 540834bf645c7fe5fc401e26548bf311f57a4c26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Sat, 15 Aug 2026 02:28:37 +0100 Subject: [PATCH] fix: keep PR actions from wrapping the title (#2670) --- .../change-request-detail-header.tsx | 10 +- apps/web/e2e/helpers/layout-assertions.ts | 51 +++++ .../pr/mobile-pr-rerequest-review.spec.ts | 80 ++++++- .../web/e2e/tests/pr/pr-detail-layout.spec.ts | 204 +++++++++++++++++- docs/plans/pr-detail-header-width/plan.md | 157 ++++++++++++++ .../task-01-panel-responsive-header.md | 117 ++++++++++ docs/specs/INDEX.md | 1 + docs/specs/pr-detail-header-width/spec.md | 67 ++++++ 8 files changed, 674 insertions(+), 13 deletions(-) create mode 100644 docs/plans/pr-detail-header-width/plan.md create mode 100644 docs/plans/pr-detail-header-width/task-01-panel-responsive-header.md create mode 100644 docs/specs/pr-detail-header-width/spec.md diff --git a/apps/web/components/integrations/change-request-detail-header.tsx b/apps/web/components/integrations/change-request-detail-header.tsx index 09a8ac2b6c..d4b09c2efa 100644 --- a/apps/web/components/integrations/change-request-detail-header.tsx +++ b/apps/web/components/integrations/change-request-detail-header.tsx @@ -159,16 +159,20 @@ export function ChangeRequestDetailHeader({ const state = detail.draft && detail.state === "open" ? "draft" : detail.state; return (
-
+
{detail.title} -
+
{actions.map((action) => ( { + 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 { + 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. diff --git a/apps/web/e2e/tests/pr/mobile-pr-rerequest-review.spec.ts b/apps/web/e2e/tests/pr/mobile-pr-rerequest-review.spec.ts index 463ae5242b..5aa6272829 100644 --- a/apps/web/e2e/tests/pr/mobile-pr-rerequest-review.spec.ts +++ b/apps/web/e2e/tests/pr/mobile-pr-rerequest-review.spec.ts @@ -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) => diff --git a/apps/web/e2e/tests/pr/pr-detail-layout.spec.ts b/apps/web/e2e/tests/pr/pr-detail-layout.spec.ts index b82e4de663..492ec4f5db 100644 --- a/apps/web/e2e/tests/pr/pr-detail-layout.spec.ts +++ b/apps/web/e2e/tests/pr/pr-detail-layout.spec.ts @@ -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 { + 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 { + 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 { + 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 { + 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"); + }); }); diff --git a/docs/plans/pr-detail-header-width/plan.md b/docs/plans/pr-detail-header-width/plan.md new file mode 100644 index 0000000000..0293da5fc2 --- /dev/null +++ b/docs/plans/pr-detail-header-width/plan.md @@ -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. diff --git a/docs/plans/pr-detail-header-width/task-01-panel-responsive-header.md b/docs/plans/pr-detail-header-width/task-01-panel-responsive-header.md new file mode 100644 index 0000000000..a71d959b85 --- /dev/null +++ b/docs/plans/pr-detail-header-width/task-01-panel-responsive-header.md @@ -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. diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index 4cfc777756..82d7f10efd 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -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 | diff --git a/docs/specs/pr-detail-header-width/spec.md b/docs/specs/pr-detail-header-width/spec.md new file mode 100644 index 0000000000..74bd4c3cf4 --- /dev/null +++ b/docs/specs/pr-detail-header-width/spec.md @@ -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)