Skip to content

test(web): close mobile drawer via mobile control#33

Merged
vincentkoc merged 1 commit into
mainfrom
fix/mobile-drawer-e2e
Jun 25, 2026
Merged

test(web): close mobile drawer via mobile control#33
vincentkoc merged 1 commit into
mainfrom
fix/mobile-drawer-e2e

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jun 25, 2026

Copy link
Copy Markdown
Member

What Problem This Solves

The post-merge main CI run failed in Playwright because the mobile drawer test clicked the desktop sidebar collapse button while it was offscreen at mobile width.

Why This Change Was Made

The mobile drawer already exposes a visible Close navigation control. The test should exercise that mobile control instead of relying on the desktop collapse affordance.

User Impact

No production behavior changes. This keeps the mobile navigation regression test aligned with the UI and unblocks main CI.

Evidence

  • pnpm fmt:ts:check
  • pnpm test:e2e tests/e2e/chat.spec.ts --project=chromium --grep "mobile navigation behaves like a drawer" built and started the app locally, then stopped because the local Playwright Chromium binary is not installed. The hosted PR CI has the browser bundle and is the E2E proof lane.

@clawsweeper

clawsweeper Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 25, 2026, 3:17 AM ET / 07:17 UTC.

Summary
The PR changes the mobile navigation Playwright test to click the visible Close navigation control instead of the desktop Collapse sidebar button.

Reproducibility: yes. at source level: current main's focused mobile drawer test opens mobile navigation, verifies Close navigation is visible, then clicks the desktop Collapse sidebar control. I did not run Playwright locally, so the failing CI path still depends on the hosted E2E lane.

Review metrics: 2 noteworthy metrics.

  • Patch surface: 1 test file changed, +1/-1. The diff is a narrowly scoped selector correction with no production file changes.
  • Runtime surface: 0 production files changed. This keeps compatibility, security, and user-facing behavior risk low for maintainers reviewing the PR.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Wait for the hosted Playwright E2E check to finish before merge.

Risk before merge

  • [P1] The hosted Playwright E2E check was still pending at review time; because the local focused run stopped before browser execution, maintainers should wait for that lane before merge.

Maintainer options:

  1. Decide the mitigation before merge
    Land the one-line test selector change after hosted E2E passes, while keeping production drawer behavior and existing desktop collapse coverage unchanged.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No ClawSweeper repair lane is needed because the PR already contains the narrow fix and only needs normal maintainer/CI completion.

Security
Cleared: The diff only changes a Playwright selector string in a test file, with no dependency, workflow, generated asset, secret, or runtime security surface changed.

Review details

Best possible solution:

Land the one-line test selector change after hosted E2E passes, while keeping production drawer behavior and existing desktop collapse coverage unchanged.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: current main's focused mobile drawer test opens mobile navigation, verifies Close navigation is visible, then clicks the desktop Collapse sidebar control. I did not run Playwright locally, so the failing CI path still depends on the hosted E2E lane.

Is this the best way to solve the issue?

Yes, the PR is the narrowest maintainable fix: it exercises the already-rendered mobile Close navigation control and leaves the separate desktop collapse regression test intact.

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning high; reviewed against 22e635b151fe.

Label changes

Label changes:

  • add P3: This is a low-risk test-only CI repair with no production behavior change.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate does not apply to this member-authored, test-only PR; hosted CI is the relevant behavior check.

Label justifications:

  • P3: This is a low-risk test-only CI repair with no production behavior change.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate does not apply to this member-authored, test-only PR; hosted CI is the relevant behavior check.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; it only gives SQL/sqlc guidance, which does not apply to this test-only PR. (AGENTS.md:1, 22e635b151fe)
  • Current test mismatch: On current main, the mobile drawer test opens mobile navigation, asserts Close navigation is visible, then clicks Collapse sidebar before expecting the drawer to close. (tests/e2e/chat.spec.ts:280, 22e635b151fe)
  • Mobile close control exists: The app renders a mobile-nav-backdrop button labelled Close navigation when mobileNavOpen is true, and that button calls closeMobileNav. (apps/web/src/ChatApp.svelte:2399, 22e635b151fe)
  • Desktop collapse control is separate: The Sidebar component's Collapse sidebar button is the desktop/sidebar collapse affordance wired through onToggleCollapse, so using the mobile close control is the narrower test target for the drawer close step. (apps/web/src/components/navigation/Sidebar.svelte:65, 22e635b151fe)
  • PR diff scope: The PR head changes exactly one selector in tests/e2e/chat.spec.ts, replacing the first Collapse sidebar click with Close navigation. (tests/e2e/chat.spec.ts:282, 83cb80845e06)
  • GitHub PR state: GitHub reports the PR as open, mergeable, one changed file, no labels, with Go/TypeScript/Docker checks passed and Playwright E2E still pending at review time. (83cb80845e06)

Likely related people:

  • vincentkoc: Vincent Koc has prior merged history in the affected Playwright/web area, including formatting and web UI work touching the central files, in addition to authoring this narrow test PR. (role: recent area contributor; confidence: medium; commits: 7247aee0b805, 31c507e5d632; files: tests/e2e/chat.spec.ts, apps/web/src/ChatApp.svelte)
  • Peter Steinberger: Blame attributes the original mobile navigation test and the Close navigation / Collapse sidebar controls to Peter Steinberger, with later adjacent mobile Safari layout work in the same UI/test area. (role: introduced behavior and adjacent mobile layout contributor; confidence: high; commits: ef0f042d4c3a, 11adbd1aa0b9; files: tests/e2e/chat.spec.ts, apps/web/src/ChatApp.svelte, apps/web/src/components/navigation/Sidebar.svelte)
  • clawsweeper[bot]: The current Collapse sidebar click inside the mobile drawer test was added by the merged mobile drawer collapse fix commit, so this bot-authored change is direct provenance for the line being adjusted. (role: introduced current test selector path; confidence: high; commits: 5b07eee6aabe; files: tests/e2e/chat.spec.ts, apps/web/src/ChatApp.svelte)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Jun 25, 2026
@vincentkoc
vincentkoc force-pushed the fix/mobile-drawer-e2e branch from 83cb808 to 956568c Compare June 25, 2026 07:19
@vincentkoc
vincentkoc merged commit 122ca7f into main Jun 25, 2026
4 checks passed
@vincentkoc
vincentkoc deleted the fix/mobile-drawer-e2e branch June 25, 2026 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant