feat(web): mobile drawer keyboard footer + NavSections tests #1043

Merged

charles merged 4 commits from feat/web-1021-mobile-drawer-nav into main 2026-05-10 15:05:31 +00:00

Conversation 13 Commits 4 Files changed 5 +92 -20

dev commented 2026-05-10 12:30:07 +00:00

Collaborator

Copy link

Drawer pins keyboard shortcuts next to the version strip; rail keeps the inline shortcuts row via a showKeyboardShortcutButton flag. App-shell tests cover section parity, close-on-tap, and shortcut callback.

Test plan

• bun run typecheck (repo root / pre-push hook)
• Biome on changed components + tests
• cd apps/web && bun run test (browser mode — app-shell + nav-sections)

Closes #1021

Drawer pins keyboard shortcuts next to the version strip; rail keeps the inline shortcuts row via a `showKeyboardShortcutButton` flag. App-shell tests cover section parity, close-on-tap, and shortcut callback. ## Test plan - `bun run typecheck` (repo root / pre-push hook) - Biome on changed components + tests - `cd apps/web && bun run test` (browser mode — `app-shell` + `nav-sections`) Closes #1021

dev added 1 commit 2026-05-10 12:30:07 +00:00

feat(web): mobile drawer footer keyboard + NavSections flag ... ac600ba4a1

Co-authored-by: Cursor <cursoragent@cursor.com>

dev requested review from reviewer 2026-05-10 12:36:10 +00:00

reviewer requested changes 2026-05-10 12:42:41 +00:00

Dismissed

reviewer left a comment

Copy link

CI red — job qa-1 / step Test failed (3m47s). Two tests in app-shell.test.tsx throw Playwright strict-mode violations because screen.getByTestId(…) is unscoped and resolves to 2 elements (one in the sidebar rail, one in the drawer).

• test-gap app-shell.test.tsx:181 — screen.getByTestId("nav-board").click() matches both getByTestId('nav-drawer').getByTestId('nav-board') and getByTestId('app-shell-sidebar').getByTestId('nav-board'). Scope to the drawer: screen.getByTestId("nav-drawer").getByTestId("nav-board").click().
• test-gap app-shell.test.tsx:190 — screen.getByTestId("nav-keyboard-shortcuts").click() matches the new DrawerChromeFooter button and the sidebar rail's NavSections button (rail always shows the button regardless of showKeyboardShortcutButton). Scope to the drawer: screen.getByTestId("nav-drawer").getByTestId("nav-keyboard-shortcuts").click().

The qa job (id 8121) failed at "Set up job" in 0s — looks like a runner infrastructure blip, not a code issue.

Production code in nav-sections.tsx and app-shell.tsx looks correct.

CI red — job `qa-1` / step **Test** failed (3m47s). Two tests in `app-shell.test.tsx` throw Playwright strict-mode violations because `screen.getByTestId(…)` is unscoped and resolves to 2 elements (one in the sidebar rail, one in the drawer). - **test-gap** `app-shell.test.tsx:181` — `screen.getByTestId("nav-board").click()` matches both `getByTestId('nav-drawer').getByTestId('nav-board')` and `getByTestId('app-shell-sidebar').getByTestId('nav-board')`. Scope to the drawer: `screen.getByTestId("nav-drawer").getByTestId("nav-board").click()`. - **test-gap** `app-shell.test.tsx:190` — `screen.getByTestId("nav-keyboard-shortcuts").click()` matches the new `DrawerChromeFooter` button *and* the sidebar rail's `NavSections` button (rail always shows the button regardless of `showKeyboardShortcutButton`). Scope to the drawer: `screen.getByTestId("nav-drawer").getByTestId("nav-keyboard-shortcuts").click()`. The `qa` job (id 8121) failed at "Set up job" in 0s — looks like a runner infrastructure blip, not a code issue. Production code in `nav-sections.tsx` and `app-shell.tsx` looks correct.

dev added 1 commit 2026-05-10 12:44:31 +00:00

fix(web): scope mobile drawer tests to avoid strict-mode duplicates ... d6dad6db45

Co-authored-by: Cursor <cursoragent@cursor.com>

dev commented 2026-05-10 12:44:34 +00:00

Author

Collaborator

Copy link

Addressed the review: nav-board and nav-keyboard-shortcuts clicks in app-shell.test.tsx are now scoped under getByTestId("nav-drawer") so Playwright strict mode does not match the duplicate elements in the sidebar rail. Pushed as d6dad6d on feat/web-1021-mobile-drawer-nav.

Local just qa could not finish the web Vitest browser suite here (Chromium missing libglib-2.0.so.0 in this environment); just typecheck, just lint, and just fmt-check passed, and pre-push typecheck + Biome ran clean.

Addressed the review: **`nav-board`** and **`nav-keyboard-shortcuts`** clicks in `app-shell.test.tsx` are now scoped under **`getByTestId("nav-drawer")`** so Playwright strict mode does not match the duplicate elements in the sidebar rail. Pushed as `d6dad6d` on `feat/web-1021-mobile-drawer-nav`. Local **`just qa`** could not finish the web Vitest browser suite here (Chromium missing `libglib-2.0.so.0` in this environment); **`just typecheck`**, **`just lint`**, and **`just fmt-check`** passed, and pre-push typecheck + Biome ran clean.

dev requested review from reviewer 2026-05-10 12:47:33 +00:00

reviewer requested changes 2026-05-10 13:11:53 +00:00

Dismissed

reviewer left a comment

Copy link

• test-gap: CI still red — qa / qa-1, Test step failed (1m2s). Typecheck / Lint / Format all green. The strict-mode scoping fix (nav-drawer.getByTestId(...)) is correct, but the race condition in "tapping a nav link closes the drawer" is still present.

  apps/web/src/components/app-shell.test.tsx line ~183:

  // current — synchronous, doesn't wait for React state flush
  const shell = screen.getByTestId("shell-children").element() as HTMLElement;
  expect(shell.ownerDocument.querySelector('[data-testid="nav-drawer"]')).toBeNull();
  

  After await click() Playwright resolves before React's batched state update is flushed to the DOM, so nav-drawer can still be present when the assertion runs.

  Fix — replace with the retrying form:

  await screen.getByTestId("nav-drawer").getByTestId("nav-board").click();
  await expect.element(screen.getByTestId("nav-drawer")).not.toBeInTheDocument();
  

  expect.element(...) uses Playwright's built-in retry loop, so it will wait until the drawer is gone.

- **test-gap**: CI still red — `qa / qa-1`, **Test** step failed (1m2s). Typecheck / Lint / Format all green. The strict-mode scoping fix (`nav-drawer.getByTestId(...)`) is correct, but the race condition in "tapping a nav link closes the drawer" is still present. `apps/web/src/components/app-shell.test.tsx` line ~183: ```tsx // current — synchronous, doesn't wait for React state flush const shell = screen.getByTestId("shell-children").element() as HTMLElement; expect(shell.ownerDocument.querySelector('[data-testid="nav-drawer"]')).toBeNull(); ``` After `await click()` Playwright resolves before React's batched state update is flushed to the DOM, so `nav-drawer` can still be present when the assertion runs. Fix — replace with the retrying form: ```tsx await screen.getByTestId("nav-drawer").getByTestId("nav-board").click(); await expect.element(screen.getByTestId("nav-drawer")).not.toBeInTheDocument(); ``` `expect.element(...)` uses Playwright's built-in retry loop, so it will wait until the drawer is gone.

dev added 1 commit 2026-05-10 13:13:26 +00:00

fix(web): wait for drawer unmount in app-shell test ... 82f1a694df

Use expect.element(...).not.toBeInTheDocument() after the nav click so the
assertion retries until React flushes the drawer close, avoiding a race with
synchronous DOM queries.

Co-authored-by: Cursor <cursoragent@cursor.com>

dev commented 2026-05-10 13:13:26 +00:00

Author

Collaborator

Copy link

Follow-up on review (race in “tapping a nav link closes the drawer”) — Pushed 82f1a694 on feat/web-1021-mobile-drawer-nav.

Replaced the synchronous querySelector('[data-testid="nav-drawer"]') assertion with:

await expect.element(screen.getByTestId("nav-drawer")).not.toBeInTheDocument();

so Playwright retries until the drawer is gone after the batched close.

Earlier strict-mode feedback (nav-drawer-scoped nav-board / nav-keyboard-shortcuts) remains as in d6dad6d.

(No inline review threads on the forge side — this comment maps to the latest REQUEST_CHANGES review body.)

**Follow-up on review (race in “tapping a nav link closes the drawer”)** — Pushed `82f1a694` on `feat/web-1021-mobile-drawer-nav`. Replaced the synchronous `querySelector('[data-testid="nav-drawer"]')` assertion with: `await expect.element(screen.getByTestId("nav-drawer")).not.toBeInTheDocument();` so Playwright retries until the drawer is gone after the batched close. Earlier **strict-mode** feedback (`nav-drawer`-scoped `nav-board` / `nav-keyboard-shortcuts`) remains as in `d6dad6d`. *(No inline review threads on the forge side — this comment maps to the latest `REQUEST_CHANGES` review body.)*

dev requested review from reviewer 2026-05-10 13:19:29 +00:00

reviewer requested changes 2026-05-10 13:20:39 +00:00

Dismissed

reviewer left a comment

Copy link

• ci: Run #3383 is red on head 82f1a69. Cannot approve with failing CI — fix the failing job and push a new commit.

- **ci**: Run #3383 is red on head `82f1a69`. Cannot approve with failing CI — fix the failing job and push a new commit.

dev commented 2026-05-10 13:20:40 +00:00

Author

Collaborator

Copy link

🤖 Review loop capped — auto-merging

Reviewer reviewer submitted 3 REQUEST_CHANGES rounds on this PR against author dev. Per the max_review_rounds=3 policy, the review cycle is halted and boss will squash-merge the PR now.

What still applies

• PR must be open, mergeable (no conflicts), and CI green. If any of those fail, the force-merge dispatch stops and posts an explanatory comment — no hard bypass.
• The latest review state is APPROVED check is waived for this merge. The review will be REQUEST_CHANGES, and that's by design.

Rationale

Each round costs ~5 min × 2 agents × 1M-context, and past round 3 findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues.

Cap is max_review_rounds=3 (set via agents.json::pipeline.max_review_rounds). To raise the cap, update the config. To revert to operator-handoff instead of auto-merge, swap the forceMerge branch in guardAuthorDispatch + handleChangesRequested.

## 🤖 Review loop capped — auto-merging Reviewer `reviewer` submitted **3 REQUEST_CHANGES rounds** on this PR against author `dev`. Per the `max_review_rounds=3` policy, the review cycle is halted and boss will squash-merge the PR now. ### What still applies - PR must be **open**, **mergeable** (no conflicts), and **CI green**. If any of those fail, the force-merge dispatch stops and posts an explanatory comment — no hard bypass. - The `latest review state is APPROVED` check is **waived** for this merge. The review will be REQUEST_CHANGES, and that's by design. ### Rationale Each round costs ~5 min × 2 agents × 1M-context, and past round 3 findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues. _Cap is `max_review_rounds=3` (set via `agents.json::pipeline.max_review_rounds`). To raise the cap, update the config. To revert to operator-handoff instead of auto-merge, swap the `forceMerge` branch in `guardAuthorDispatch` + `handleChangesRequested`._

code-lead commented 2026-05-10 13:21:05 +00:00

Collaborator

Copy link

Cannot force-merge: CI is failing on 82f1a69. Failing run: https://forge.jacquin.app/charles/claude-hooks/actions/runs/1861 — fix the failure and push a new commit before re-requesting merge.

Cannot force-merge: CI is failing on `82f1a69`. Failing run: https://forge.jacquin.app/charles/claude-hooks/actions/runs/1861 — fix the failure and push a new commit before re-requesting merge.

dev force-pushed feat/web-1021-mobile-drawer-nav from 82f1a694df to c0d20a46bb 2026-05-10 13:29:04 +00:00 Compare

dev requested review from reviewer 2026-05-10 13:32:05 +00:00

reviewer requested changes 2026-05-10 13:33:16 +00:00

Dismissed

reviewer left a comment

Copy link

• ci: Run 3386 is failure on head c0d20a46. Fix the failing CI job before this can merge. (API didn't return step-level logs — check https://forge.jacquin.app/charles/claude-hooks/actions/runs/1864 for the concrete error.)

- **ci**: Run 3386 is `failure` on head `c0d20a46`. Fix the failing CI job before this can merge. (API didn't return step-level logs — check https://forge.jacquin.app/charles/claude-hooks/actions/runs/1864 for the concrete error.)

dev commented 2026-05-10 13:33:18 +00:00

Author

Collaborator

Copy link

🤖 Review loop capped — auto-merging

Reviewer reviewer submitted 4 REQUEST_CHANGES rounds on this PR against author dev. Per the max_review_rounds=3 policy, the review cycle is halted and boss will squash-merge the PR now.

What still applies

• PR must be open, mergeable (no conflicts), and CI green. If any of those fail, the force-merge dispatch stops and posts an explanatory comment — no hard bypass.
• The latest review state is APPROVED check is waived for this merge. The review will be REQUEST_CHANGES, and that's by design.

Rationale

Each round costs ~5 min × 2 agents × 1M-context, and past round 4 findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues.

Cap is max_review_rounds=3 (set via agents.json::pipeline.max_review_rounds). To raise the cap, update the config. To revert to operator-handoff instead of auto-merge, swap the forceMerge branch in guardAuthorDispatch + handleChangesRequested.

## 🤖 Review loop capped — auto-merging Reviewer `reviewer` submitted **4 REQUEST_CHANGES rounds** on this PR against author `dev`. Per the `max_review_rounds=3` policy, the review cycle is halted and boss will squash-merge the PR now. ### What still applies - PR must be **open**, **mergeable** (no conflicts), and **CI green**. If any of those fail, the force-merge dispatch stops and posts an explanatory comment — no hard bypass. - The `latest review state is APPROVED` check is **waived** for this merge. The review will be REQUEST_CHANGES, and that's by design. ### Rationale Each round costs ~5 min × 2 agents × 1M-context, and past round 4 findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues. _Cap is `max_review_rounds=3` (set via `agents.json::pipeline.max_review_rounds`). To raise the cap, update the config. To revert to operator-handoff instead of auto-merge, swap the `forceMerge` branch in `guardAuthorDispatch` + `handleChangesRequested`._

code-lead commented 2026-05-10 13:33:42 +00:00

Collaborator

Copy link

Force-merge blocked: CI is failing on c0d20a4 — https://forge.jacquin.app/charles/claude-hooks/actions/runs/1864. Fix the failing job and push a green commit before re-dispatching.

Force-merge blocked: CI is failing on `c0d20a4` — https://forge.jacquin.app/charles/claude-hooks/actions/runs/1864. Fix the failing job and push a green commit before re-dispatching.

dev referenced this pull request 2026-05-10 14:05:33 +00:00

feat(web): mobile drawer adopts grouped <NavSections> #1021

charles referenced this pull request from a commit 2026-05-10 14:15:56 +00:00

test(event-log-delta): drain taskHistory in afterAll so history-contract test doesn't bleed

charles force-pushed feat/web-1021-mobile-drawer-nav from c0d20a46bb to 47b47f0a66 2026-05-10 14:46:22 +00:00 Compare

dev requested review from reviewer 2026-05-10 14:52:25 +00:00

reviewer requested changes 2026-05-10 14:53:54 +00:00

reviewer left a comment

Copy link

• ci: Run #1870 (id 3392) on head 47b47f0 is failure. The API returned no job/step detail — check the run at https://forge.jacquin.app/charles/claude-hooks/actions/runs/1870 to identify the failing step. Code changes look correct on inspection; unblocks once CI is green.

- **ci**: Run #1870 (id 3392) on head `47b47f0` is `failure`. The API returned no job/step detail — check the run at https://forge.jacquin.app/charles/claude-hooks/actions/runs/1870 to identify the failing step. Code changes look correct on inspection; unblocks once CI is green.

dev commented 2026-05-10 14:53:56 +00:00

Author

Collaborator

Copy link

🤖 Review loop capped — auto-merging

Reviewer reviewer submitted 5 REQUEST_CHANGES rounds on this PR against author dev. Per the max_review_rounds=3 policy, the review cycle is halted and boss will squash-merge the PR now.

What still applies

• PR must be open, mergeable (no conflicts), and CI green. If any of those fail, the force-merge dispatch stops and posts an explanatory comment — no hard bypass.
• The latest review state is APPROVED check is waived for this merge. The review will be REQUEST_CHANGES, and that's by design.

Rationale

Each round costs ~5 min × 2 agents × 1M-context, and past round 5 findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues.

Cap is max_review_rounds=3 (set via agents.json::pipeline.max_review_rounds). To raise the cap, update the config. To revert to operator-handoff instead of auto-merge, swap the forceMerge branch in guardAuthorDispatch + handleChangesRequested.

## 🤖 Review loop capped — auto-merging Reviewer `reviewer` submitted **5 REQUEST_CHANGES rounds** on this PR against author `dev`. Per the `max_review_rounds=3` policy, the review cycle is halted and boss will squash-merge the PR now. ### What still applies - PR must be **open**, **mergeable** (no conflicts), and **CI green**. If any of those fail, the force-merge dispatch stops and posts an explanatory comment — no hard bypass. - The `latest review state is APPROVED` check is **waived** for this merge. The review will be REQUEST_CHANGES, and that's by design. ### Rationale Each round costs ~5 min × 2 agents × 1M-context, and past round 5 findings are usually nitpick spiral or reviewer non-determinism rather than real correctness issues. _Cap is `max_review_rounds=3` (set via `agents.json::pipeline.max_review_rounds`). To raise the cap, update the config. To revert to operator-handoff instead of auto-merge, swap the `forceMerge` branch in `guardAuthorDispatch` + `handleChangesRequested`._

code-lead commented 2026-05-10 14:54:22 +00:00

Collaborator

Copy link

Cannot merge: CI is failing on 47b47f0. Failing run: https://forge.jacquin.app/charles/claude-hooks/actions/runs/1870 — please fix and re-push before merge can proceed.

Cannot merge: CI is failing on `47b47f0`. Failing run: https://forge.jacquin.app/charles/claude-hooks/actions/runs/1870 — please fix and re-push before merge can proceed.

charles added 1 commit 2026-05-10 14:58:12 +00:00

test(agent-config-routes): drop process-global mock.module of ../sse ... 2ea55aa7fd

Bun's `mock.module` is process-global; once this file runs, every later
test in the same Bun process sees the no-op `broadcastSSE`. That broke
`assign-dedup.test.ts` (and indirectly `handleIssueUnassigned` tests)
whose assertions wait for `dispatch.duplicate_running` events to reach
`addSSEListener` subscribers — with the stub installed, no events ever
fire, listener stays empty, expectations fail.

The original stub was defensive (suppress SSE noise during route
tests). The real `broadcastSSE` is a no-op when no listeners are
attached, which is exactly the state these tests run in. Drop the
sse mock; keep the render-queue mock (it does need to suppress FS
writes).

Same class of cross-file pollution as PR #1046's `__resetHistoryForTest`
fix — Bun's process-global module/state singletons keep biting. The
janitor.ts file header already documents this exact `mock.module` sharp
edge.

dev requested review from reviewer 2026-05-10 15:01:13 +00:00

charles merged commit 54d0694d3a into main 2026-05-10 15:05:31 +00:00

charles deleted branch feat/web-1021-mobile-drawer-nav 2026-05-10 15:05:31 +00:00

charles referenced this pull request from a commit 2026-05-10 15:05:31 +00:00

feat(web): mobile drawer keyboard footer + NavSections tests (#1043)

Sign in to join this conversation.

Reviewers

No reviewers

reviewer

Labels

Clear labels   

area:agents

Agent types, pool scheduling, per-instance config

  

area:dashboard

Dashboard UI and observability surfaces

  

area:database

DB layer — schema, migrations, ORM, raw SQL

  

area:design

UI/UX mockup work — routes to designer agent

  

area:design-review

Design review dispatch — routes to design-reviewer agent

  

area:flows

Flow runner — YAML loader, executor, op registry, expression eval

  

area:infra

Deployment, isolation, containers, systemd units

  

area:meta

Tracking, scaffolding, project setup

  

area:security

Security — routes to reviewer-security (opus)

  

area:sessions

Session-id store, Claude SDK resume logic

  

area:webhook

Forgejo webhook routing and handlers

  

area:workdir

Clone cache, worktrees, git identity

  

security

Security-sensitive issue

  

type:bug

Bug

  

type:chore

Chore

  

type:meta

Tracking or decisions, not implementation work

  

type:user-story

User story

No labels

area:agents

area:dashboard

area:database

area:design

area:design-review

area:flows

area:infra

area:meta

area:security

area:sessions

area:webhook

area:workdir

security

type:bug

type:chore

type:meta

type:user-story

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

4 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

charles/claude-hooks!1043

No description provided.