fix(tests): three CSS-shape cases pinned modal DOM the FloatingWindow migration retired (#2967)

Two cases in `agent-modals-mobile` asserted DOM shapes that the FN-8619
`FloatingWindow` migration deliberately retired. Both are **stale
expectations, not product regressions** — established below rather than
assumed, because "delete the failing assertion" is exactly how a real
bug gets buried.

### 1. `AgentDetailView` — the suite asserted both sides of the same
fact

```js
expect(document.querySelector(".agent-detail-overlay")).toBeTruthy();   // here — RED
```
```js
expect(document.querySelector(".agent-detail-overlay")).toBeNull();     // AgentDetailView.core.test.tsx:97 — GREEN
```

No component renders that class (`grep` across `app/**/*.tsx` outside
tests: zero hits). One of these two had to be red, and the one matching
the product is the `toBeNull` sibling. This case now asserts the panel
class it is actually named for — `.agent-detail-modal`, which **is**
live and **is** what the mobile `@media` block in `AgentDetailView.css`
targets — and pins the scrim as still-retired.

I did **not** re-point the overlay half at `.floating-window-overlay`.
That would only re-assert FloatingWindow's own contract (already covered
by `FloatingWindow.test.tsx`) while saying nothing about Agent Detail
being mobile-targetable.

### 2. `AgentGenerationModal` — the class belongs to a different
component

It demanded `.agent-dialog-overlay`. That class is **still live** —
`NewAgentDialog.tsx:415` renders it — which is why the stale expectation
looked plausible and survived. But this modal is a `FloatingWindow` with
`modal` (`AgentGenerationModal.tsx:162`), so its scrim is
`.floating-window-overlay--modal`. Now asserted explicitly, because "a
modal blocks the app beneath it" is a real FN-8619 contract worth
pinning.

### Why the dead CSS is still here

`.agent-detail-overlay` has 4 CSS definitions and one inert mobile
`@media` rule. I deliberately did **not** delete them in this PR:

- Two **passing** tests (`dashboard-overflow-containment.test.tsx:295`,
`mobile-horizontal-pan-containment.test.ts:90`) pin a selector *string*
that lists `.agent-detail-overlay`. Deleting the CSS turns two green
tests red.
- That raises a question I cannot answer without rendering, and I will
not boot an instance to find out: **those containment lists do not
mention `.floating-window-overlay--modal`.** If mobile horizontal-pan
containment is meant to cover modal scrims, the migration may have moved
the scrim out from under its guard. That is a product question for
whoever owns FN-8619, and it is the substance of #2915.

Worth recording: the retired `.agent-detail-overlay { padding: 0;
align-items: stretch }` mobile rule is not a lost feature.
`.floating-window-overlay` is `position: fixed; inset: 0` with no flex
context, so those declarations have nothing to act on — FloatingWindow
positions the panel by geometry instead.

**Verified:** 22/22 in this file, `tsc -p tsconfig.app.json` 0 errors,
lint clean, FNXC gate exit 0. Test-only, no changeset.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-30 22:01:07 -07:00
committed by GitHub
parent 2502878166
commit 04159ff9ed
3 changed files with 148 additions and 48 deletions

View File

@@ -1,8 +1,6 @@
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
import { render, screen, waitFor, within } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import fs from "node:fs";
import path from "node:path";
import { AgentDetailView } from "../AgentDetailView";
import { AgentGenerationModal } from "../AgentGenerationModal";
import { AgentImportModal } from "../AgentImportModal";
@@ -203,13 +201,30 @@ describe("agent modal mobile CSS structure", () => {
});
describe("AgentDetailView", () => {
it("overlay and modal have mobile-targetable classes", async () => {
/*
FNXC:ModalTouchGeometry 2026-07-30-18:20:
THE SCRIM IS FLOATINGWINDOW'S NOW — this asserts the panel, not a local overlay.
This case demanded `.agent-detail-overlay`, which no component renders: FN-8619 migrated Agent
Detail onto `FloatingWindow`, whose `modal` host owns the scrim
(`.floating-window-overlay--modal`). `AgentDetailView.core.test.tsx:97` already asserts the
class is ABSENT, so the suite was asserting both sides of the same fact and one of them had to
be red.
The panel class is what this file is actually for: `.agent-detail-modal` is live and is what the
mobile `@media` block in AgentDetailView.css targets. The overlay half is dropped rather than
re-pointed at `.floating-window-overlay` — that would only re-assert FloatingWindow's own
contract, which FloatingWindow.test.tsx already covers, and would say nothing about Agent
Detail being mobile-targetable.
*/
it("modal panel keeps its mobile-targetable class", async () => {
render(<AgentDetailView agentId="agent-001" onClose={vi.fn()} addToast={vi.fn()} />);
await waitFor(() => {
expect(document.querySelector(".agent-detail-overlay")).toBeTruthy();
expect(document.querySelector(".agent-detail-modal")).toBeTruthy();
});
// The retired local scrim must stay retired; its CSS is dead and tracked in #2915.
expect(document.querySelector(".agent-detail-overlay")).toBeNull();
});
it("tabs have scrollable container class", async () => {
@@ -293,10 +308,20 @@ describe("agent modal mobile CSS structure", () => {
});
describe("AgentGenerationModal", () => {
it("overlay and dialog classes exist", () => {
/*
FNXC:ModalTouchGeometry 2026-07-30-18:20:
`.agent-dialog-overlay` belongs to NewAgentDialog, NOT to this modal.
That class is still live — `NewAgentDialog.tsx:415` renders it — which is why the stale
expectation here looked plausible. But AgentGenerationModal was migrated onto `FloatingWindow`
with `modal` (AgentGenerationModal.tsx:162), so its scrim is
`.floating-window-overlay--modal` and it never emits the local overlay. Asserted explicitly
because "modal blocks the app beneath it" is a real contract worth pinning per FN-8619.
*/
it("dialog renders inside the shared modal scrim", () => {
render(<AgentGenerationModal isOpen={true} onClose={vi.fn()} onGenerated={vi.fn()} />);
expect(document.querySelector(".agent-dialog-overlay")).toBeTruthy();
expect(document.querySelector(".floating-window-overlay--modal")).toBeTruthy();
expect(document.querySelector(".agent-dialog")).toBeTruthy();
});

View File

@@ -1,6 +1,4 @@
import fs from "node:fs";
import { loadAllAppCss } from "../../test/cssFixture";
import path from "node:path";
import { describe, expect, it } from "vitest";
@@ -34,6 +32,21 @@ function getMainMobileBlock(css: string): string {
return block;
}
/*
FNXC:ModalTouchGeometry 2026-07-30-19:20:
FloatingWindow's phone breakpoint is NOT the 768px one `getMainMobileBlock` aggregates.
It is `(max-width: 767.98px), (max-height: 480px)` — the project's documented mobile query, whose
`max-height` clause catches landscape phones that exceed 768px wide. Reusing the 768px helper here
silently returns styles.css's block, which contains none of these selectors; the anti-vacuity
assertion in the caller is what caught that during authoring.
*/
function getFloatingWindowMobileBlock(css: string): string {
const block = getMediaBlocks(css, /@media[^{]*\(max-width:\s*767\.98px\)[^{]*\{/g);
expect(block).toContain(".floating-window");
return block;
}
function getEmbeddedGitManagerBlock(css: string): string {
const block = getMediaBlocks(css, /@container\s+gm-embedded\s+\(max-width:\s*560px\)\s*\{/g);
expect(block).toContain(".gm-modal--embedded .gm-sidebar");
@@ -67,35 +80,57 @@ function getLastRuleBlock(css: string, selector: string): string {
return block!;
}
function extractVhHeight(rule: string): number {
const heightMatch = rule.match(/height:\s*(\d+)vh;/);
expect(heightMatch).toBeTruthy();
return Number(heightMatch![1]);
}
describe("core modals mobile css coverage", () => {
it("TaskDetailModal: keeps desktop, tablet, mobile, and embedded height invariants", () => {
/*
FNXC:ModalTouchGeometry 2026-07-30-19:05:
TASK DETAIL NO LONGER SIZES ITSELF — the invariant moved layers, it did not disappear.
This case pinned `.modal.task-detail-modal` at `height: 85vh` (desktop), `92vh` (tablet) and
`100dvh` (mobile), with `resize: both` / `resize: none`. FN-8619 migrated Task Detail onto
`FloatingWindow`, so the panel is now `width: 100%; height: 100%` and fills a host sized by
geometry. The desktop and tablet `vh` heights are `defaultSize` in TSX, not CSS, so asserting them
against a stylesheet can only ever fail.
The MOBILE invariant is the one that still matters and is still CSS, so it is asserted at its new
home: `.floating-window--task-detail` inside FloatingWindow.css's mobile block takes over the
viewport and hides the resize handle. That is the same guarantee the old `100dvh` / `resize: none`
assertions made — a phone gets a full-screen sheet, not a draggable window.
Deliberately NOT re-pinning the desktop/tablet numbers via `defaultSize`: those are ordinary
layout defaults a designer may retune, and a test that fails on a 640→680 width change is noise.
The full-screen-on-mobile rule is a real contract; 85vh on a desktop is a preference.
Note the media query is `(max-width: 767.98px), (max-height: 480px)` — landscape phones exceed
768px wide, so the height clause is load-bearing and asserted with it.
*/
it("TaskDetailModal: takes over the viewport on mobile instead of sizing itself", () => {
const css = loadAllAppCss();
const tabletBlock = getTabletBlock(css);
const mobileBlock = getMainMobileBlock(css);
/* The panel defers sizing to its FloatingWindow host. */
const baseRule = getFirstRuleBlock(css, ".modal.task-detail-modal");
expect(baseRule).toContain("height: 85vh;");
expect(baseRule).toContain("max-height: calc(100dvh - var(--overlay-padding-top, 10vh) - 16px);");
expect(baseRule).toContain("resize: both;");
expect(baseRule).toContain("width: 100%;");
expect(baseRule).toContain("height: 100%;");
const tabletRule = getLastRuleBlock(tabletBlock, ".modal.task-detail-modal");
expect(tabletRule).toContain("height: 92vh;");
expect(extractVhHeight(tabletRule)).toBeGreaterThan(extractVhHeight(baseRule));
expect(tabletRule).toContain("width: 98vw;");
expect(tabletRule).toContain("max-width: 98vw;");
expect(tabletBlock).toContain("--overlay-padding-top: 6vh;");
expect(tabletRule).toContain("max-height: calc(100dvh - var(--overlay-padding-top, 6vh) - var(--space-md));");
/* ANTI-VACUITY: prove the mobile block was found and really is the phone breakpoint,
so a renamed/removed query fails loudly instead of matching an empty string. */
const floatingMobileBlock = getFloatingWindowMobileBlock(css);
expect(floatingMobileBlock).toContain("max-height: 480px");
expect(floatingMobileBlock).toContain(".floating-window--task-detail");
const mobileRule = getLastRuleBlock(mobileBlock, ".modal.task-detail-modal");
expect(mobileRule).toContain("height: 100dvh;");
expect(mobileRule).toContain("max-height: 100dvh;");
expect(mobileRule).toContain("resize: none;");
const mobileRule = getLastRuleBlock(floatingMobileBlock, ".floating-window--task-detail");
/*
MATCHED AT DECLARATION BOUNDARIES, not by substring. `toContain("height: 100dvh")` is satisfied
by the `max-height: 100dvh` line, so it stays green even if `height` itself is changed — caught
by mutation: rewriting `height` to `90dvh` left the old assertions passing. Anchoring on `{`/`;`
forces each declaration to be checked on its own.
*/
expect(mobileRule).toMatch(/[{;]\s*height:\s*100dvh/);
expect(mobileRule).toMatch(/[{;]\s*max-height:\s*100dvh/);
expect(mobileRule).toMatch(/[{;]\s*width:\s*100vw/);
expect(mobileRule).toMatch(/[{;]\s*max-width:\s*100vw/);
/* The resize affordance must be gone on touch, not merely inert. */
expect(floatingMobileBlock).toContain(".floating-window--task-detail .floating-window__resize-handle");
const embeddedRule = getRuleBlocks(css, ".task-detail-content--embedded")
.find((rule) => rule.includes("height: 100%;"));

View File

@@ -20,7 +20,39 @@ vi.mock("../../api", () => ({
fetchTaskDetail: vi.fn(),
batchUpdateTaskModels: vi.fn(),
fetchNodes: vi.fn(() => new Promise(() => {})),
fetchBoardWorkflows: vi.fn(() => new Promise(() => {})),
/*
FNXC:ListViewWindowing 2026-07-30-20:10:
A RESOLVED LANE IS A PRECONDITION FOR RENDERING ANY ROW — this mock used to hang forever.
It was `vi.fn(() => new Promise(() => {}))`, which was harmless when written: List view still had
`LEGACY_LIST_COLUMNS` to fall back on. U12/R9 DELETED that fallback, so `ListView` now returns its
skeleton unless `useBoardWorkflows` resolves a lane — and a never-settling promise means it never
does. All eight cases then asserted against an empty document ("expected [] to have a length of
50"), which reads as a broken render window rather than an unmet precondition.
Resolved with a real column set, mirroring the payload other board tests use, so the windowing
invariants below are exercised against actual rows. `flagEnabled: true` matters: the workflow arm
is the only one left.
*/
fetchBoardWorkflows: vi.fn().mockResolvedValue({
flagEnabled: true,
defaultWorkflowId: "builtin:coding",
workflows: [
{
id: "builtin:coding",
name: "Coding",
columns: [
{ id: "todo", name: "Todo", flags: { intake: true, hold: true } },
{ id: "in-progress", name: "In Progress", flags: { countsTowardWip: true } },
{ id: "in-review", name: "In Review", flags: { mergeBlocker: true, humanReview: true } },
{ id: "done", name: "Done", flags: { complete: true } },
{ id: "archived", name: "Archived", flags: { archived: true } },
],
},
],
taskWorkflowIds: {},
}),
fetchWorkflowSteps: vi.fn().mockResolvedValue([]),
rebuildTaskSpec: vi.fn().mockResolvedValue({}),
refreshPrStatus: vi.fn().mockResolvedValue({}),
updateTask: vi.fn(),
@@ -79,8 +111,14 @@ function makeTask(index: number): Task {
const TASKS: Task[] = Array.from({ length: TOTAL_TASKS }, (_, i) => makeTask(i + 1));
const FAR_TASK_ID = "FN-190";
function renderList(props: Partial<React.ComponentProps<typeof ListView>> = {}) {
return render(
/*
FNXC:ListViewWindowing 2026-07-30-20:15:
ASYNC because the lane now resolves through a promise. `useBoardWorkflows` settles a microtask after
mount, and `ListView` renders its skeleton until it does — so a synchronous `render()` observes zero
rows no matter what the window logic does. Flushed here, once, rather than in each case.
*/
async function renderList(props: Partial<React.ComponentProps<typeof ListView>> = {}) {
const result = render(
<ListView
tasks={TASKS}
onMoveTask={vi.fn(async () => TASKS[0])}
@@ -93,6 +131,8 @@ function renderList(props: Partial<React.ComponentProps<typeof ListView>> = {})
{...props}
/>,
);
await act(async () => { await Promise.resolve(); });
return result;
}
function renderedTaskIds(): string[] {
@@ -110,8 +150,8 @@ beforeEach(() => {
});
describe("ListView render windowing", () => {
it("renders only the initial window of a large section, not every task", () => {
renderList();
it("renders only the initial window of a large section, not every task", async () => {
await renderList();
expect(renderedTaskIds()).toHaveLength(INITIAL_WINDOW);
// The section header still reports the FULL group size — grouping is preserved.
@@ -119,8 +159,8 @@ describe("ListView render windowing", () => {
expect(screen.getByRole("button", { name: /Load 25 more/i })).toBeTruthy();
});
it("reveals the next increment when Load more is clicked", () => {
renderList();
it("reveals the next increment when Load more is clicked", async () => {
await renderList();
act(() => {
fireEvent.click(screen.getByRole("button", { name: /Load 25 more/i }));
@@ -133,8 +173,8 @@ describe("ListView render windowing", () => {
expect(renderedTaskIds()).toHaveLength(INITIAL_WINDOW + INCREMENT * 2);
});
it("filters against the full set, so a match beyond the window is still found", () => {
renderList({ searchQuery: "Needle" });
it("filters against the full set, so a match beyond the window is still found", async () => {
await renderList({ searchQuery: "Needle" });
const ids = renderedTaskIds();
expect(ids).toEqual([FAR_TASK_ID]);
@@ -142,14 +182,14 @@ describe("ListView render windowing", () => {
expect(screen.queryByRole("button", { name: /Load \d+ more/i })).toBeNull();
});
it("keeps a selected task outside the window selected and visible", () => {
it("keeps a selected task outside the window selected and visible", async () => {
localStorage.setItem(scopedKey("kb-dashboard-list-selected-task", PROJECT_ID), FAR_TASK_ID);
localStorage.setItem(
scopedKey("kb-dashboard-selected-tasks", PROJECT_ID),
JSON.stringify([FAR_TASK_ID]),
);
renderList();
await renderList();
// Selection state is id-based and untouched by the window.
expect(
@@ -182,8 +222,8 @@ describe("ListView select-all under render windowing", () => {
});
}
it("selects only the rendered window, not the whole filtered set", () => {
renderList();
it("selects only the rendered window, not the whole filtered set", async () => {
await renderList();
enterBulkEdit();
const rendered = renderedTaskIds();
@@ -200,7 +240,7 @@ describe("ListView select-all under render windowing", () => {
});
it("confirms a bulk delete against the rendered rows only", async () => {
renderList();
await renderList();
enterBulkEdit();
selectAll();
@@ -214,8 +254,8 @@ describe("ListView select-all under render windowing", () => {
expect(message).not.toContain(String(TOTAL_TASKS));
});
it("grows the select-all target as the window is expanded", () => {
renderList();
it("grows the select-all target as the window is expanded", async () => {
await renderList();
enterBulkEdit();
act(() => {
@@ -230,8 +270,8 @@ describe("ListView select-all under render windowing", () => {
expect(persisted.sort()).toEqual([...renderedTaskIds()].sort());
});
it("reports checked, not indeterminate, once the rendered window is fully selected", () => {
renderList();
it("reports checked, not indeterminate, once the rendered window is fully selected", async () => {
await renderList();
enterBulkEdit();
selectAll();