ci: add a macOS desktop e2e lane - #3137
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review. 📝 WalkthroughSummaryThis PR adds a macOS 15 desktop E2E lane. It uses the existing shard configuration and preserves the Linux The PR extends the existing E2E path. It does not create a separate fixture or test framework. The solution is coherent and small. The new macOS lane is required for platform coverage. The scrolling helper and updated hit-testing diagnostics address macOS-specific window restoration and virtualized transcript timing. No deletion is evident without reducing regression coverage. Some test diagnostics could be simplified later if they prove unnecessary after the macOS lane stabilizes. Validation
Review-relevant risks
WalkthroughThe pull request adds a macOS desktop E2E lane, broadens isolated-display handling to macOS, tightens CI checkout permissions, reserves prompt-rail space for overlay scrollbars, and improves prompt-rail and slash-command E2E synchronization. ChangesmacOS E2E and CI display support
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The PR adds macOS desktop end-to-end coverage, but a revised relay lifecycle timeout may allow regressions in the 10ms fallback deadline to pass undetected. The change is otherwise mergeable with explicit owner awareness and follow-up on that test guard. Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Playwright
participant Electron
CI->>Playwright: Run desktop E2E tests on macOS 15
Playwright->>Electron: Launch visible fixture windows
Electron-->>Playwright: Render prompt rail and composer
Playwright-->>CI: Report E2E results
CI->>CI: Run alignment audit
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ff3a97dc-ecaf-49cf-b569-3206d4d82ba6
📒 Files selected for processing (5)
.github/workflows/ci.ymlapps/desktop/e2e/fixtures.tsapps/desktop/e2e/playwright.config.tsscripts/fixture-env.mjsscripts/fixture-window.mjs
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/styles/chat-header.css (1)
159-162: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDo not rely on
scrollbar-gutter: stablefor overlay scrollbars.The CSS Overflow specification defines
stablefor classic scrollbars. Overlay scrollbars do not create a gutter. (w3.org) This declaration therefore cannot provide the stated macOS guarantee. Verify that.maka-chat-layoutis the actual[data-chat-scroll-container="true"]element. If it is not, the rule is inert. Sinceprompt-rail.cssalready applies an explicit 48px offset, remove this rule unless classic-scrollbar stability on this exact scroll box is a separate requirement.Sources: Path instructions, MCP tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 868001b8-cc67-4d25-966d-f8d405bec20b
📒 Files selected for processing (3)
apps/desktop/e2e/prompt-rail.spec.tsapps/desktop/src/renderer/styles/chat-header.cssapps/desktop/src/renderer/styles/prompt-rail.css
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 561c6bc7-ce31-4fff-8cd6-e688c98d6d12
📒 Files selected for processing (3)
apps/desktop/e2e/prompt-rail.spec.tsapps/desktop/e2e/slash-command-menu.spec.tspackages/eval/harbor/test_relay_lifecycle.py
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/e2e/prompt-rail.spec.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
There was a problem hiding this comment.
Pull request overview
This PR extends the desktop end-to-end CI surface to include macOS runners so platform-specific renderer/compositor behaviors (overlay scrollbars, App Nap throttling, post-scroll hit-testing) can fail PRs before merge, while preserving the existing Linux e2e job name to keep required checks stable.
Changes:
- Add a new
e2e_macosGitHub Actions job (macos-15) alongside the existing Linux xvfbe2elane. - Generalize “CI display must be visible” detection from Linux-only (
isCiLinuxDisplay) to cross-platform (isCiIsolatedDisplay), and update fixture launch behavior accordingly. - Harden prompt-rail styling and e2e tests against macOS-specific hit-testing/scrolling/titlebar overlay behaviors; reduce a slash-command menu flake by ensuring composer focus before typing.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/fixture-window.mjs | Switches to isCiIsolatedDisplay() to decide when the fixture window must be visible on CI. |
| scripts/fixture-env.mjs | Adds cross-platform CI-display detection and fixes CI flag parsing (CI=false no longer treated as truthy). |
| apps/desktop/src/renderer/styles/prompt-rail.css | Moves prompt-rail away from macOS overlay scrollbar hit regions to prevent dead-band clicks. |
| apps/desktop/e2e/slash-command-menu.spec.ts | Stabilizes slash-command triggering by refocusing the composer before typing /. |
| apps/desktop/e2e/prompt-rail.spec.ts | Improves macOS-visible-window robustness for scroll mounting and hit-testing assertions. |
| apps/desktop/e2e/playwright.config.ts | Updates documentation comment to reflect macOS CI compositor behavior. |
| apps/desktop/e2e/fixtures.ts | Uses isCiIsolatedDisplay() so macOS CI runs with a visible Electron window when needed. |
| .github/workflows/ci.yml | Adds e2e_macos lane and hardens checkout by disabling persisted credentials; sets minimal contents: read permissions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e7cfa3e to
5843859
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
apps/desktop/src/renderer/styles/prompt-rail.css:53
- The comment says “GitHub's macos-15 image uses Always”, but the new macOS CI lane explicitly sets
AppleShowScrollBarstoWhenScrolling. This inconsistency makes it unclear which scrollbar mode the 48px offset is primarily targeting (and can mislead future debugging if CI behavior changes).
sailed through CI green. The overlay dead band is 14px when the bar
appears only while scrolling, and ~16px when the system shows scroll
bars Always — GitHub's macos-15 image uses Always, and a visible
Electron window can stack a native overlay on the webview's own bar.
Rest the rail at `space-8 + space-4` (48px), clear of that stack, and
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Adding a dedicated macOS lane is the right way to validate an OS-specific compositor/overlay problem, and the fixture isolation is thoughtfully scoped. One test seam still defeats that goal: the critical jump assertions bypass the native pointer path, while the test itself records that the first tick is hidden by the titlebar.
The first-principles split is simpler: prove user reachability with one real pointer interaction on a visible tick and assert its destination; test jump/auto-follow state transitions separately at the logic boundary. That removes the synthetic-click exception and makes each test responsible for exactly one claim.
Review performed with Codex reviewer agents and DeepSeek V4 Flash as advisory tools; I verified this finding against the latest head and live CI state.
中文评论
增加独立 macOS lane 是验证特定系统 compositor/overlay 问题的正确方向，fixture 隔离也很克制。但关键跳转断言绕过了原生 pointer path，而测试本身又明确记录首个 tick 被 titlebar 覆盖，因此当前测试不能证明真实用户交互可达。
更符合第一性原理的最小方案是：用一次真实 pointer 交互点击可见 tick 并断言落点，单独在逻辑边界验证 jump/auto-follow 状态变化。这样可以删掉 synthetic click 例外，每个测试只证明一个事实。
本次审查使用了 Codex reviewer agents 与 DeepSeek V4 Flash 作为辅助工具；我已依据最新 head 和实时 CI 状态复核该问题。
|
/agentic_review |
Code Review by Qodo
1. macOS failure is nonblocking
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
This PR also changes the visible prompt-rail position on macOS to avoid overlay scrollbar and titlebar hit regions. Could you please add a screenshot of the resulting prompt rail in the Desktop UI, ideally with the macOS overlay scrollbar visible? A before/after comparison is welcome, but one clearly annotated result is sufficient. Thanks! Posted by Codex on behalf of Astro-Han. |
bc64b35 to
cebe9c5
Compare
|
Rebased onto current Here is the requested macOS result with the native overlay scrollbar visible at the far right. The prompt rail is inset from that hit region and its first-to-last tick column stays below the titlebar clearance:
The rebased implementation restores normal Playwright pointer clicks for the first tick and verifies the complete painted rail column instead of trimming the titlebar-covered prefix. Local validation on the new head: prompt-rail E2E 7/7, the previously failing titlebar workbar scenario 1/1, Desktop build and typecheck, CI contract tests 29/29, helper tests 61/61, lint, format, ASF headers, and diff checks. The new CI run is currently |
cebe9c5 to
217df6e
Compare
|
@Astro-Han Fixed the two failures from the current macOS run in
Local macOS verification:
The updated-head workflow is awaiting creation/approval; please approve it when GitHub exposes the run. |
217df6e to
940e129
Compare
|
@Astro-Han Rebased onto current The previous red
The fresh exact-head workflow can now provide the remote Linux/macOS evidence. The fresh workflow is waiting for maintainer approval: CI 32638153455. Please approve this exact-head run. |
940e129 to
999d3e1
Compare
|
The previous CI run had one macOS geometry failure: I reran the same head locally on macOS with The branch is now rebased onto current |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at exact head 999d3e10167024926b58fea393cbd79dc54351d0. [P1] + [P2]; not ready to merge.
[P1] The geometry-wait fix is deterministically ineffective on the lane it targets — apps/desktop/e2e/prompt-rail.spec.ts:200-244
The change wraps the continuous hit-test in a 10 s expect.poll, on the premise that the rail needs more time to settle. Two hosted runs at patch-equivalent heads say otherwise:
| run | head | e2e_macos |
result |
|---|---|---|---|
32638153455 |
940e129c7 |
failure | insideViewport:true, hasSpan:true, misses:74 |
32640104914 |
999d3e101 |
failure | insideViewport:true, hasSpan:true, misses:74 |
test_linux is green in both; the aggregate fails closed as designed.
Identical numbers across two runs is the part that matters. misses:74 is not boundary jitter that a longer wait would absorb — it is the same sustained non-hitting each time, which falsifies the "just wait for layout" premise directly. A local 10/10 green doesn't outweigh two same-valued reds on the actual target lane; different platform, different result, and the target lane is the one that has to pass.
What the evidence does not yet settle is whether the cause is real product geometry or a hit-test seam in the harness — see the P2, which is why that distinction can't currently be made. Either way, one of the two has to be fixed and the exact head has to go green before merge. Loosening the continuous-reachability assertion is not an option here: that assertion is the contract, and relaxing it converts a visible failure into an invisible one.
For the record, the other failing spec in that job — a skill-draft-lifecycle click timeout — is on an unchanged path and I'm not attributing it to this PR.
[P2] The macOS lane produces nothing to diagnose with — .github/workflows/ci.yml:235-262
The new macOS lane has no upload-artifact step at all, and the artifacts API returns 0 for both runs above. So a runner-only failure currently cannot be investigated.
The second commit makes this worse in a specific way: the previous assertion emitted rail travel hits ${JSON.stringify(travel.sample)}, and the poll message that replaced it still computes sample but never prints it. What survives in the log is misses=74 with no way to see which elements intercepted those 74 y-positions — which is exactly the information needed to tell product geometry from a harness seam.
Suggested: upload apps/desktop/test-results plus traces and screenshots on always(), and restore sample to the failure message. That is likely a prerequisite for diagnosing the P1 rather than a follow-up to it.
Closed from earlier rounds
The previously-raised planner concern is withdrawn: the planner suite is 29/29 and every non-Storybook apps/desktop/** path does select E2E. Both real runs confirm the Linux/macOS selection and the fail-closed aggregate, so there is no missed-selection defect.
999d3e1 to
960c92b
Compare
|
Addressed both findings at exact head
Validation: full Desktop E2E passed 56 with 1 existing skip; the final continuous-path target passed 10/10; build, typecheck, lint, format, ASF headers, and CI planner 30/30 passed. The fresh exact-head checks are ready for maintainer approval. |
Summary
Desktop e2e only ran on Linux xvfb, so macOS overlay-scrollbar hit testing and App Nap compositor throttling could not fail a PR. This adds a
macos-15sibling of the existinge2ejob.The Linux job name is unchanged so the required check stays intact. Isolated CI displays (xvfb and GitHub macOS runners) show the fixture window; local machines stay hidden.
Fixes #2342
Test plan
isCiIsolatedDisplaycovers Linux CI, macOS CI, and local Darwin