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
1 change: 1 addition & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ itself driving one of them.
- Use the [pruning procedure](docs/worktree-hygiene.md). `remove` and the daily installed
job require fresh merge proof and a verified recovery bundle before deleting local work.
Do not bypass this with force removal, branch force-deletion, or manual directory deletion.
Unmerged work you are giving up goes through `scripts/worktree.sh retire <branch> --expect-head <SHA> --by <recorded-owner> --apply`, which verifies separate recovery snapshots and retains the original registered checkout indefinitely in private recovery storage. It removes the managed path and local branch, not the retained disk usage.
`scripts/worktree.sh audit` reports eligibility; `schedule status` reports the local job.
These tools never delete remote branches or recovery archives. GitHub deletes a PR's
branch itself when the PR merges (`delete_branch_on_merge`, on since 2026-10-15) and moves
Expand Down
9 changes: 4 additions & 5 deletions apps/desktop/e2e/code-review-a11y.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,12 @@ for (const size of [{ width: 1440, height: 900 }, { width: 900, height: 600 }])
});
}

/** The lowest WCAG contrast of any text inside an element, skipping inactive controls (WCAG 1.4.3 exempts them), with the text that scored it. */
const lowest = (page: Page, selector: string) => page.locator(selector).first().evaluate((root) => {
/** The lowest WCAG contrast of any text inside every element the selector matches, skipping inactive controls (WCAG 1.4.3 exempts them), with the text that scored it. */
const lowest = (page: Page, selector: string) => page.locator(selector).evaluateAll((roots) => {
const rgba = (s: string) => (s.match(/[\d.]+/g) ?? []).map(Number);
const lum = ([r, g, b]: number[]) => [r!, g!, b!].map((v) => { const c = v / 255; return c <= 0.03928 ? c / 12.92 : ((c + 0.055) / 1.055) ** 2.4; }).reduce((a, c, i) => a + c * [0.2126, 0.7152, 0.0722][i]!, 0);
let worst = { ratio: 21, text: "" };
const walker = document.createTreeWalker(root, NodeFilter.SHOW_TEXT);
for (let node = walker.nextNode(); node; node = walker.nextNode()) {
for (const root of roots) for (let walker = document.createTreeWalker(root, NodeFilter.SHOW_TEXT), node = walker.nextNode(); node; node = walker.nextNode()) {
const el = node.parentElement!;
const text = node.textContent?.trim() ?? "";
if (!text || el.closest('[disabled], [aria-disabled="true"]') || !el.checkVisibility({ opacityProperty: true, visibilityProperty: true })) continue;
Expand All @@ -157,7 +156,7 @@ const lowest = (page: Page, selector: string) => page.locator(selector).first().
/** The CR-4 to CR-7 text a reviewer reads, in every theme: details, activity, tree and diff, the review panel, the composers and the merge panel. */
const SLICE_TEXT: [string, string][] = [
["activity", '[data-testid="cr-activity"]'], ["checks", '[data-testid="cr-checks"]'], ["details", 'aside[aria-label="Pull request details"]'], ["details", 'aside[aria-label="Pull request details"]'],
["tree file", '[data-testid="cr-tree-file"]'], ["file section", '[data-testid="cr-file-section"]'],
["diff", '[data-testid="code-review-diff"]'], ["tree", '.cr-tree'],
["review section", '[data-testid="cr-agent"]'], ["composer", '[data-testid="cr-compose"]'],
];
for (const theme of Object.keys(THEMES) as Theme[]) {
Expand Down
62 changes: 62 additions & 0 deletions apps/desktop/e2e/code-review-agent.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -334,3 +334,65 @@ test("integrated review: populated diff → native fixture finding → confirmed
expect(runs()).toHaveLength(1);
expect(h.posts()).toHaveLength(1);
});

/** The pane's scroller for its content: the main column, or the whole body where the narrow overview stacks. */
async function overviewScroller(page: Page) {
const main = page.locator(".cr-overview-main");
const stacked = await main.evaluate((el) => getComputedStyle(el).overflowY === "visible");
return stacked ? tid(page, "code-review-overview-body") : main;
}

/** C61: the control is inside its scroller's visible box and nothing is painted over it. */
async function expectClear(page: Page, control: ReturnType<Page["locator"]>, name: string): Promise<void> {
const scroller = await overviewScroller(page);
await control.focus();
await expect(control, `${name} keeps the focus`).toBeFocused();
await expect.poll(async () => control.evaluate((el, box) => {
const r = el.getBoundingClientRect();
const hit = document.elementFromPoint(r.left + r.width / 2, r.top + r.height / 2);
return r.height > 0 && r.top >= box.top - 0.5 && r.bottom <= box.bottom + 0.5 && !!hit && (el === hit || el.contains(hit));
}, await scroller.evaluate((el) => { const b = el.getBoundingClientRect(); return { top: b.top, bottom: b.bottom }; })), `${name} is on screen and uncovered`).toBe(true);
}

/**
* C61: brought into view, the last thing in the overview's main column ends inside the scroller, uncovered. Where the
* narrow overview stacks, the sidebar follows the main column, so the item is scrolled to rather than the scroller's end.
*/
async function expectLastItemClear(page: Page, label: string): Promise<void> {
const scroller = await overviewScroller(page);
const measure = () => scroller.evaluate((el) => {
const last = document.querySelector(".cr-overview-main section:last-of-type > :last-child")!;
last.scrollIntoView({ block: "end" });
const box = el.getBoundingClientRect(), r = last.getBoundingClientRect();
const hit = document.elementFromPoint(r.left + r.width / 2, r.bottom - 2);
return { scrollerHeight: Math.round(box.height), overrun: Math.round(r.bottom - box.bottom), covered: !hit || !(last === hit || last.contains(hit)), hit: hit?.className ?? null };
});
await expect.poll(async () => { const m = await measure(); return m.scrollerHeight >= 120 && m.overrun <= 0 && !m.covered ? "clear" : JSON.stringify(m); }, `${label}: the last item clears the bottom, and the pane keeps a usable height`).toBe("clear");
}

for (const [width, height] of [[900, 600], [1440, 900]] as const) {
test(`C61: the inline Ask section and the last activity item stay reachable at ${width}×${height}, with the review panel shut and open`, async () => {
h = await launchWithGh(baseWorld());
installClaude("reply");
const page = await openLending();
// fitWindow caps the height at the display's work area, so a hosted runner's small screen tests a shorter window.
await fitWindow(h, width, height);
const ask = tid(page, "cr-agent-ask");
await expect(ask).toBeVisible();

await expectClear(page, ask.getByRole("textbox"), "Ask's text box");
await ask.getByRole("textbox").fill("A question long enough to matter.\n".repeat(8));
await expectClear(page, ask.getByRole("button", { name: "Ask", exact: true }), "Ask");
await expectLastItemClear(page, "panel shut");

await tid(page, "code-review-review").first().click();
await expect(tid(page, "cr-agent-panel")).toBeVisible();
await expectClear(page, ask.getByRole("textbox"), "Ask's text box with the panel open");
await expectClear(page, ask.getByRole("button", { name: "Ask", exact: true }), "Ask with the panel open");
await expectLastItemClear(page, "panel open");
// The panel itself: its controls fit, or scroll, inside the window rather than off its bottom edge.
const start = tid(page, "cr-agent-start");
await start.scrollIntoViewIfNeeded();
await expect(start).toBeInViewport({ ratio: 1 });
});
}
5 changes: 5 additions & 0 deletions apps/desktop/src/renderer/code-review/code-review.css
Original file line number Diff line number Diff line change
Expand Up @@ -256,3 +256,8 @@ button.cr-state { cursor: pointer; font: inherit; }

/* C76: the deletion counts on the file header and tree use the diff's text colour, which keeps AA on every theme's panel background. */
.cr-file-heading .stat.del, .cr-tree-file .stat.del { color: var(--diff-del-text); }

/* The diff's plain actions (Refresh files, Retry, Cancel context): without this they take the browser's button colours, which fail contrast on every dark theme. */
.cr-diff button:not([class]) { color: var(--text-1); background: transparent; border: 1px solid var(--border-pane); border-radius: 6px; padding: 2px 8px; font: inherit; cursor: pointer; }
.cr-diff button:not([class]):hover:not(:disabled) { background: var(--bg-row-hover); }
.cr-diff button:not([class]):disabled { cursor: default; color: var(--text-3); }
4 changes: 2 additions & 2 deletions docs/design/code-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -580,8 +580,8 @@ avatars and C58, C60 and C61 of the composer are not built (open decisions, #325
**Deviations.**

- **D6, repository in the header.** The header shows the repository name and number, as the references
do, with `owner/name` in its tooltip. It reads them from the inbox row or the resolved link until #323
binds `overview.locator` (C22).
do, with `owner/name` in its tooltip. It reads them from the inbox row or the resolved link until the
overview loads, then from the accepted `overview.locator` (C22, #477).
- **D7, state pill.** It is the state menu once the PR has loaded and main has answered for the account;
otherwise, and for a merged PR, it shows the state with no menu.
- **D8, a Pinned group.** Pins appear in a Pinned group above the inbox's own, which the references
Expand Down
Loading
Loading