Skip to content

ci: wire the agent-picker dialog guard into CI and extend it to the edit dialog #9023

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
javenciu wants to merge 1 commit into from
+163 −87
Closed

ci: wire the agent-picker dialog guard into CI and extend it to the edit dialog #9023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 19 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2550,6 +2550,25 @@ jobs:
working-directory: website
run: npx playwright install chromium

- name: Real-browser dialog guard (agent picker in Schedule dialogs)
# The unit suite pins structure only: AgentSelector.dialog.test.tsx's own
# header says the interaction cannot be exercised faithfully under
# happy-dom and delegates the end-to-end proof to this script -- which,
# until #8963, no workflow invoked, so the guarded behaviour had no
# automatic guard at all. It lives in this job for the same reason as the
# render gate below: it reuses the Chromium install above and the
# production bundle from `npm run build`, and needs no gateway -- the
# script serves website/dist over loopback and answers every /api/** call
# from fixtures, like the other capture harnesses.
#
# The outDir is passed explicitly so the screenshots land under
# website/test-results/, which the `Upload Playwright failure artifacts`
# step below already uploads on `failure()`. The script's default is
# /tmp, which an ephemeral runner discards -- leaving only the thrown
# error to debug a real-browser failure. No new upload step is needed.
working-directory: website
run: npm run verify:agent-selector-dialog -- test-results/agent-selector-dialog

- name: i18n render-time gate (en-XA + shipped locales)
# Phase 5 of the i18n plan. Every other i18n gate reads SOURCE or CATALOG
# JSON, so three defect classes are invisible to all of them: a string that
Expand Down
10 changes: 7 additions & 3 deletions docs/ci/e2e-gate.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,9 +175,13 @@ silent darkening is no guard.
`ci.yml`'s `e2e` job (`E2E (stub ACP backend, offline)`) installs the backend
with `--group dev`, runs `npm ci` and `npm run build` in `website/`, stages
`website/dist` into `src/kiro_crew/static/dist` so the specs render the real
bundled dashboard rather than a 404, installs Chromium, runs the i18n render-time
gate (which reuses that Chromium install), and finally runs `python setup.py
test_e2e`.
bundled dashboard rather than a 404, installs Chromium, runs the real-browser
dialog guard (`npm run verify:agent-selector-dialog` — the end-to-end proof
that `AgentSelector.dialog.test.tsx` delegates here because happy-dom cannot
exercise Radix's layer interplay; it drives the agent picker inside BOTH the
create and the edit job dialogs against `website/dist` with fixture-stubbed
`/api/**`), runs the i18n render-time gate (which reuses that Chromium
install), and finally runs `python setup.py test_e2e`.

### The browser install is budgeted, and installs no apt packages

Expand Down
1 change: 1 addition & 0 deletions src/kiro_crew/builtin_skills/kirocrew-dev/prepare-pr/profiles/kirocrew.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
"BASE=\"$(git merge-base HEAD origin/main)\" && I18N_BASE_REF=\"$BASE\" npm --prefix website run i18n:check",
"npm --prefix website run lint:phantom-classes -- --test",
"BASE=\"$(git merge-base HEAD origin/main)\" && PHANTOM_BASE_REF=\"$BASE\" npm --prefix website run lint:phantom-classes",
"npm --prefix website run verify:agent-selector-dialog",
"BASE=\"$(git merge-base HEAD origin/main)\" && I18N_BASE_REF=\"$BASE\" npm --prefix website run i18n:render",
"BASE=\"$(git merge-base HEAD origin/main)\" && I18N_BASE_REF=\"$BASE\" SCOPED_TESTS_BASE_REF=\"$BASE\" python3 scripts/run_scoped_tests.py --surface frontend",
"npm --prefix website run jscpd",
Expand Down
220 changes: 136 additions & 84 deletions website/scripts/verify-agent-selector-dialog.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,25 @@
* Testing Library's event batch, so the popup never opens under fireEvent.
*
* Drives the REAL built SPA (website/dist) behind the shared `serveDist`
* server with every /api/** call answered from fixtures:
* open /schedule -> Add job (Radix MODAL dialog) -> open the agent picker ->
* click a non-default agent -> assert the value committed AND the dialog
* stayed open -> reopen and assert the keyboard path (filter input focused,
* ArrowDown roves to an option, Enter on a narrowed filter commits).
* server with every /api/** call answered from fixtures, against BOTH dialogs
* that host the picker (#8963 — the edit dialog used to have no guard at all):
*
* create: open /schedule -> Add job (Radix MODAL dialog) -> drive the picker
* edit: close it -> click the fixture job's row (same JobDetailDialog,
* titled by the job's name) -> drive the same picker again
*
* The drill per dialog: open the agent picker -> click a non-default agent ->
* assert the value committed AND the dialog stayed open -> reopen and assert
* the keyboard path (filter input focused, ArrowDown roves to an option,
* Enter on a narrowed filter commits) -> reopen and assert a real wheel event
* scrolls the overflowing list -> Escape dismisses the popup, not the dialog.
*
* With the pre-fix build (bare createPortal to document.body) the option click
* times out on Playwright's hit-test: react-remove-scroll's
* `pointer-events: none` on the body swallows it — run with EXPECT=broken to
* capture that state as the "before" evidence instead of failing.
* capture that state as the "before" evidence instead of failing. Broken mode
* drives the create dialog only: it exists to reproduce the #6358 pre-fix
* state, which predates the edit-dialog leg.
*
* Usage: EXPECT=fixed|broken node scripts/verify-agent-selector-dialog.mjs [outDir]
*/
Expand Down Expand Up @@ -47,6 +56,96 @@ const AGENTS = [
})),
]

// One persisted job so the schedule table has a row to click: the EDIT dialog
// is reachable only through an existing job (SchedulePage `openDetail` on the
// row), which is exactly the leg the create-only fixture left undriven. A
// plain message job (no `script`/`command`) keeps the agent picker rendered.
const JOBS = [{
id: 'job-01', name: 'Nightly digest', message: 'Summarize the day',
enabled: true, schedule: 'every day 09:00', last_status: 'ok',
agent: 'kirocrew',
}]

/**
* The interaction under test, identical for both host dialogs: the agent
* picker opened and committed from inside a Radix MODAL dialog. `shot`
* prefixes the screenshots so the create and edit runs stay distinguishable.
*/
async function driveAgentPicker(page, dialog, shot) {
const trigger = dialog.getByRole('button', { name: 'Switch agent' })
await trigger.click()
const listbox = page.getByRole('listbox', { name: 'Agent list' })
await listbox.waitFor({ timeout: 5000 })

await page.screenshot({ path: join(OUT, `${shot}-dropdown-open.png`) })

await page.getByRole('option', { name: /oncall/ }).click({ timeout: 5000 })

// The selection must commit…
const committed = await trigger.textContent()
if (!committed?.includes('oncall')) {
throw new Error(`[${shot}] agent selection did not commit: trigger reads "${committed}"`)
}
// …and Radix's DismissableLayer must treat it as INSIDE the dialog's layer
// stack: an outside-interaction would have closed the whole dialog.
if (!(await dialog.count())) {
throw new Error(`[${shot}] selecting an agent closed the job dialog underneath`)
}
await page.screenshot({ path: join(OUT, `${shot}-selection-committed.png`) })

// Keyboard path: reopen — the filter input must take focus (the dialog's
// FocusScope used to reclaim it), ArrowDown must rove to an option, and
// Enter on a narrowed filter must commit.
await trigger.click()
await listbox.waitFor({ timeout: 5000 })
const input = page.getByLabel('Filter agents')
if (!(await input.evaluate(el => el === document.activeElement))) {
throw new Error(`[${shot}] filter input did not take focus inside the modal dialog`)
}
await page.keyboard.press('ArrowDown')
const onOption = await page.evaluate(() => document.activeElement?.getAttribute('role') === 'option')
if (!onOption) throw new Error(`[${shot}] ArrowDown did not move focus to an option (keyboard still dead)`)
await page.keyboard.press('ArrowUp')
await input.pressSequentially('res')
await page.screenshot({ path: join(OUT, `${shot}-keyboard-filter.png`) })
await page.keyboard.press('Enter')
const kbCommitted = await trigger.textContent()
if (!kbCommitted?.includes('research')) {
throw new Error(`[${shot}] keyboard selection did not commit: trigger reads "${kbCommitted}"`)
}
if (!(await dialog.count())) {
throw new Error(`[${shot}] keyboard selection closed the job dialog underneath`)
}

// Escape must dismiss only the popup on reopen, never the dialog.
await trigger.click()
await listbox.waitFor({ timeout: 5000 })

// The option list must also SCROLL inside the modal: the popover portals
// outside DialogContent, so it sits in neither react-remove-scroll's lock
// container nor its shards — react-remove-scroll cancels wheel events it
// does not recognise, so drive a REAL wheel over the list and assert it
// moved (with a long roster this is a third way the picker could be
// "unusable inside dialogs").
const scrollable = await listbox.evaluate(el => el.scrollHeight > el.clientHeight)
if (!scrollable) {
throw new Error(`[${shot}] fixture roster does not overflow the list — the wheel assertion is vacuous`)
}
await listbox.hover()
await page.mouse.wheel(0, 120)
await page.waitForTimeout(200)
const scrolled = await listbox.evaluate(el => el.scrollTop)
if (scrolled <= 0) {
throw new Error(`[${shot}] wheel over the agent list did not scroll it inside the modal dialog`)
}

await page.keyboard.press('Escape')
await listbox.waitFor({ state: 'detached', timeout: 5000 })
if (!(await dialog.count())) {
throw new Error(`[${shot}] Escape on the agent popup also closed the job dialog underneath`)
}
}

const { srv, base } = await serveDist()
const browser = await chromium.launch()

Expand All @@ -65,32 +164,33 @@ try {
await json(route, { agents: AGENTS, default_agent: 'kirocrew' })
return true
}
if (path === '/api/crons') {
await json(route, { jobs: JOBS })
return true
}
return false
},
})
await page.addInitScript(() => localStorage.setItem('mc-lang', 'en'))

await page.goto(base + '/schedule', { waitUntil: 'domcontentloaded' })
await page.getByRole('button', { name: /Add job|Create your first job/ }).first().waitFor({ timeout: 15000 })
await page.getByRole('button', { name: 'Add job' }).first().waitFor({ timeout: 15000 })

// Open the create-job MODAL dialog.
await page.getByRole('button', { name: /Add job|Create your first job/ }).first().click()
await page.getByRole('button', { name: 'Add job' }).first().click()
const dialog = page.getByRole('dialog', { name: 'New job' })
await dialog.waitFor({ timeout: 10000 })

// THE interaction under test: the agent picker opened and committed from
// inside a Radix modal dialog.
const trigger = dialog.getByRole('button', { name: 'Switch agent' })
await trigger.click()
const listbox = page.getByRole('listbox', { name: 'Agent list' })
await listbox.waitFor({ timeout: 5000 })

await page.screenshot({ path: join(OUT, EXPECT === 'broken' ? 'before-dropdown-open.png' : 'after-dropdown-open.png') })

if (EXPECT === 'broken') {
// Pre-fix build: the popup renders but sits under the modal's
// pointer-events cut, so the click on an option never lands. Playwright's
// hit-test surfaces exactly that — the timeout IS the defect.
const trigger = dialog.getByRole('button', { name: 'Switch agent' })
await trigger.click()
const listbox = page.getByRole('listbox', { name: 'Agent list' })
await listbox.waitFor({ timeout: 5000 })
await page.screenshot({ path: join(OUT, 'before-dropdown-open.png') })

let clickLanded = true
try {
await page.getByRole('option', { name: /oncall/ }).click({ timeout: 3000 })
Expand All @@ -106,73 +206,25 @@ try {
await page.screenshot({ path: join(OUT, 'before-click-through.png') })
console.log('OK (broken build confirmed): option click does not land / does not commit')
} else {
await page.getByRole('option', { name: /oncall/ }).click({ timeout: 5000 })

// The selection must commit…
const committed = await trigger.textContent()
if (!committed?.includes('oncall')) {
throw new Error(`agent selection did not commit: trigger reads "${committed}"`)
}
// …and Radix's DismissableLayer must treat it as INSIDE the dialog's layer
// stack: an outside-interaction would have closed the whole dialog.
if (!(await dialog.count())) {
throw new Error('selecting an agent closed the job dialog underneath')
}
await page.screenshot({ path: join(OUT, 'after-selection-committed.png') })

// Keyboard path: reopen — the filter input must take focus (the dialog's
// FocusScope used to reclaim it), ArrowDown must rove to an option, and
// Enter on a narrowed filter must commit.
await trigger.click()
await listbox.waitFor({ timeout: 5000 })
const input = page.getByLabel('Filter agents')
if (!(await input.evaluate(el => el === document.activeElement))) {
throw new Error('filter input did not take focus inside the modal dialog')
}
await page.keyboard.press('ArrowDown')
const onOption = await page.evaluate(() => document.activeElement?.getAttribute('role') === 'option')
if (!onOption) throw new Error('ArrowDown did not move focus to an option (keyboard still dead)')
await page.keyboard.press('ArrowUp')
await input.pressSequentially('res')
await page.screenshot({ path: join(OUT, 'after-keyboard-filter.png') })
await page.keyboard.press('Enter')
const kbCommitted = await trigger.textContent()
if (!kbCommitted?.includes('research')) {
throw new Error(`keyboard selection did not commit: trigger reads "${kbCommitted}"`)
}
if (!(await dialog.count())) {
throw new Error('keyboard selection closed the job dialog underneath')
}

// Escape must dismiss only the popup on reopen, never the dialog.
await trigger.click()
await listbox.waitFor({ timeout: 5000 })

// The option list must also SCROLL inside the modal: the popover portals
// outside DialogContent, so it sits in neither react-remove-scroll's lock
// container nor its shards — react-remove-scroll cancels wheel events it
// does not recognise, so drive a REAL wheel over the list and assert it
// moved (with a long roster this is a third way the picker could be
// "unusable inside dialogs").
const scrollable = await listbox.evaluate(el => el.scrollHeight > el.clientHeight)
if (!scrollable) {
throw new Error('fixture roster does not overflow the list — the wheel assertion is vacuous')
}
await listbox.hover()
await page.mouse.wheel(0, 120)
await page.waitForTimeout(200)
const scrolled = await listbox.evaluate(el => el.scrollTop)
if (scrolled <= 0) {
throw new Error('wheel over the agent list did not scroll it inside the modal dialog')
}

await page.keyboard.press('Escape')
await listbox.waitFor({ state: 'detached', timeout: 5000 })
if (!(await dialog.count())) {
throw new Error('Escape on the agent popup also closed the job dialog underneath')
}

console.log('OK: select-in-dialog commits (mouse + keyboard), dialog survives, Escape scoped')
await driveAgentPicker(page, dialog, 'create')
console.log('OK [create]: select-in-dialog commits (mouse + keyboard), dialog survives, Escape scoped')

// Same component, same modal, DIFFERENT entry: the edit dialog opens from
// an existing job's row and titles itself with the job's name. Until this
// leg existed the guarded behaviour had a guard on one of its two doors.
//
// Fresh navigation, NOT Escape: the drill just closed the agent popup with
// Escape, and a second Escape aimed at the dialog races the popup's
// unmounting DismissableLayer, which can still swallow it — observed as a
// create-dialog-never-detaches timeout on an identical rebuild. A goto has
// no such race and also clears the create form's leftover state.
await page.goto(base + '/schedule', { waitUntil: 'domcontentloaded' })
await page.getByText('Nightly digest', { exact: true }).first().click()
const editDialog = page.getByRole('dialog', { name: 'Nightly digest' })
await editDialog.waitFor({ timeout: 10000 })

await driveAgentPicker(page, editDialog, 'edit')
console.log('OK [edit]: select-in-dialog commits (mouse + keyboard), dialog survives, Escape scoped')
}

await context.close()
Expand Down
Loading