Uh oh!
There was an error while loading. Please reload this page.
fix(desktop): contain first-run onboarding viewport - #3195
Conversation
Warning Review limit reached
Next review available in:6 minutes Limit details: You’ve used all 3 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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; 0 remain after this review. 📝 WalkthroughProblem solvedThe PR prevents first-run onboarding from causing page scrolling. It uses ChatLayout’s available height instead of a It removes transcript empty spacing and composer dock padding while onboarding owns the empty state. The onboarding card and provider list retain internal scrolling. Source of truth and solution scopeThe PR extends the existing onboarding layout path. It does not create a parallel path. The solution is focused. It adds onboarding-specific CSS, one shared Electron fixture, and two viewport regression tests. No code or test can be removed without weakening behavior or regression coverage. Complexity delta
Runtime maintenance complexity decreases. The added test complexity is justified by the page-scroll regression it covers. Validation and risksThe tests check viewport geometry, page overflow, chat-container overflow, onboarding containment, minimum-height behavior, internal onboarding scrolling, and bottom-of-content visibility. The PR reports Biome checks, typechecking, renderer build, Playwright tests, and real Electron visual inspection. Direct check output is not available. The final required-check status is therefore unverified. Review-relevant risksThe CSS changes affect onboarding height, overflow, spacing, and scrolling. These changes can alter user-visible behavior. Material changes in these areas require independent human review under repository policy. The fixture adds a test-only public field. No security, licensing, release, or governance effect was identified in the current diff. The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughThe changes constrain the onboarding surface to the chat viewport and add end-to-end coverage for normal and minimum-height windows. The tests use a visible, unseeded Chinese-locale onboarding window and verify internal scrolling. ChangesOnboarding viewport
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk:⚪ Minimal · up to This localized viewport and regression-test change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoContain first-run onboarding within the desktop chat viewport
AI Description
Diagram
High-Level Assessment
Files changed (3) |
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: fe71bfef-a504-40d5-9992-579098550ee8
📒 Files selected for processing (3)
apps/desktop/e2e/fixtures.tsapps/desktop/e2e/onboarding-viewport.spec.tsapps/desktop/src/renderer/styles/onboarding.css
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
Uh oh!
There was an error while loading. Please reload this page.
Code Review by Qodo
1. |
Uh oh!
There was an error while loading. Please reload this page.
c5c43de to
f764b54CompareThere 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: 20f5e681-d9c8-40d1-bfc6-7d278c25a3a6
📒 Files selected for processing (2)
apps/desktop/e2e/onboarding-viewport.spec.tsapps/desktop/src/renderer/styles/onboarding.css
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/styles/onboarding.css
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
Uh oh!
There was an error while loading. Please reload this page.
f764b54 to
dd33cc1Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Re-reviewed head dd33cc1 against current main. No actionable findings.
The root cause is real: the transcript-only flex spacer and the onboarding surface's viewport-derived minimum height combined to expand ChatLayout beyond its available block size. The final patch keeps outer scroll ownership with ChatLayout, preserves user-reachable internal scrolling at the supported minimum height, and scopes the behavior to the mounted onboarding surface.
Independent verification on a clean detached worktree:
- Biome check passed for all three changed files.
npm --workspace @maka/desktop run build:with-depspassed.npm --workspace @maka/desktop run typecheckpassed.- Targeted real Electron Playwright suite passed: 2/2, including wheel-driven scrolling at 320px height.
- All required GitHub checks are green; the PR is mergeable and clean.
Optimality: this is the smallest maintainable correction for the current ownership boundary. It adds no runtime state, public API, configuration, dependency, or parallel layout path. I do not recommend deleting any production code or either regression test, and a deeper component refactor is not warranted unless more full-viewport chat states appear.
Residual risk: the scoped CSS necessarily depends on Astryx ChatMessageList/ChatLayout DOM structure. The real Electron geometry and user-scroll assertions provide appropriate regression coverage for that contract.
Ready to merge.
Astro-Han
commented
Aug 18, 2026
Thanks for the fix — the phantom scrollbar is real (the empty-state flex spacer in Conclusion: PASS with one P2 (reached at the repo's supported minimum window) plus P3s. P2 — at the supported minimum height (480×320) the onboarding content is hard-clipped and unreachable — an accessibility regression relative to before. The three-layer P3 (optional): the commit's AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on 中文摘要（AI 辅助审查）结论：PASS（1 个 P2 + P3）。问题真实（ChatMessageList 空态前恒有 aria-hidden flex spacer 把内容推到底部，onboarding surface 的 min-height 自撑与其叠加——内容放得下却有页面滚动条，952px 视口→1159px 滚动范围）；:has(.maka-onboarding-surface) 作用域干净不污染普通对话，spacer 选择器精确匹配不会误伤 sentinel。默认尺寸（1240×820）下 e2e 断言 scrollHeight==clientHeight 成立无回归。CI 全绿。P2：仓库支持的最小高度（480×320）下 onboarding 内容被三层 overflow hidden 链硬裁剪且不可达——卡片固有 ~590-660px（onboarding-hero 四行 provider 列表+双按钮、wordmark+Card+skip），320px 窗口聊天视口仅 ~240-260px，下半部分（Browse providers/连接/Skip）被裁掉且全链无任何滚动路径；PR 之前 root 滚动能到达被裁内容（surface 无 overflow 约束、messageArea flex:1 0 auto 不收缩），所以这是本 PR 引入的可达性回归。@media (max-height:720px) 只改 align-items:start 无内部滚动兜底。仓库显式把 320px 维护为受测契约（window-state.ts:31-37 SAFE_MIN_HEIGHT）。建议：保留外层 contain、surface 改 overflow-y:auto（放不下时内部滚动），并补 480×320 e2e（fixture 尺寸 env 只在设 scenario 时读取，main-window.ts:600-616）；现有 cardBottom<=viewportBottom 断言正好是现成测试，缺的只是尺寸覆盖。P3（可选）：commit 的 Generated-by: Codex trailer 被写成字面 \n 转义（非真实 trailer 行，Ai Use Disclosure 检查已 warning，squash 时应重写）；e2e 只覆盖默认 1240×820（正因如此 P2 得以通过 CI）；结构性选择器依赖 Astryx DOM 顺序（spacer 需是 last-child 前一兄弟），已注释+e2e 兜底但脆弱。 |
Keep the onboarding surface inside ChatLayout's available height, preserve internal scrolling at the supported minimum window height, and cover both document and onboarding scroll geometry with Electron E2E. Generated-by: Codex
dd33cc1 to
ba16360CompareUh oh!
There was an error while loading. Please reload this page.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for carrying the short-viewport fix through to the final head. I reviewed exact head ba16360dd791e8952eab5d43be7ecb57a3b14723.
The problem definition is correct: the onboarding empty-state spacer and viewport-sized surface created an outer scroll authority that did not belong in the chat layout. The current solution stays at the nearest existing seam: onboarding-scoped CSS removes the spacer/dock contribution, keeps the outer layout contained, and gives the onboarding surface its own overflow-y: auto fallback. That is simpler than threading a second layout mode through shared UI components.
The final 480×320 regression now proves that the content remains reachable through internal scrolling, while the normal-size test protects the original phantom-scrollbar fix. Current-head checks are green, and the PR includes visual-inspection evidence.
Conclusion: APPROVE — no P0–P2 findings.
Unverified, non-blocking gap: the minimum-height case uses Playwright viewport sizing rather than a separate native BrowserWindow resize/zoom matrix; the current Electron coverage and geometry assertions are sufficient for this localized change.
AI-assisted review disclosure: Codex coordinated an independent @reviewer pass and a read-only ollama-cloud/deepseek-v4-flash:high pass. The review traced the ChatLayout/ChatMessageList DOM and overflow chain, checked the final short-viewport fix and regression tests, and refreshed current-head CI/review state. No local Electron run was performed in this final pass. A human contributor remains responsible for the final merge decision.
中文摘要
已复评最终 head。问题根因是 onboarding 空态 spacer 与视口高度共同制造了不应存在的外层滚动；当前方案在 onboarding 现有 CSS seam 内解决，没有新增并行布局状态。最终版本使用 surface 内部滚动，480×320 回归测试证明最小高度下内容仍可达，正常尺寸测试继续保护 phantom scrollbar 修复；当前 checks 全绿且 PR 已提供视觉检查证据。没有 P0–P2，结论：APPROVE。
非阻断缺口：最小高度用的是 Playwright viewport，而不是额外的原生窗口 resize/zoom 矩阵。
Summary
100dvh.Root cause
ChatMessageList reserves a flexible spacer before ordinary empty states. The onboarding surface also declared a viewport-derived minimum height, so it overflowed its half-height flex slot and expanded ChatLayout's self-scroll range. In the failing test, a 952px chat viewport produced 1159px of scrollable content even though the card itself fit on screen.
Verification
npx biome check apps/desktop/src/renderer/styles/onboarding.css apps/desktop/e2e/fixtures.ts apps/desktop/e2e/onboarding-viewport.spec.tsnpm --workspace @maka/desktop run typechecknpm --workspace @maka/desktop run build:renderernpx playwright test --config e2e/playwright.config.ts e2e/onboarding-viewport.spec.tsAI use
Select exactly one:
Tool(s) and scope: Codex diagnosed the flex/viewport interaction, implemented the scoped CSS fix, and added the Electron E2E regression test.
Checklist
Does this PR entail a change in behavior?