fix: post-merge title bar layout fixes and the tests/cli batch hang - #5936
Conversation
…ow header usable Three layout defects from the integrated title bar (#5910), found by a post-merge review: - The Combos sizing rules matched only a quota bar directly under .main; in the desktop shell the bar sits in .main-top, so the 100dvh shell overflowed the page by the 40px strip. The same rules now key on .main-top. - At <=760px the sticky .main-top painted over the sticky mobile header while scrolling. It now scrolls with the page (static, z-index auto), and the mobile header carries the window drag handlers so the window stays draggable after scrolling. - The titlebar metrics floored the points-to-CSS ratio at 1, so at 300% zoom a 360pt window kept an 80px inset and pushed the 44px menu off a 120px viewport. Zoomed in, only the lights share of the clearance shrinks (27px inset at 300%); the row keeps its 40px floor.
Two Linux test 4/4 logs stop after the first config-show child returns; the next synchronous child never yields despite its 40s timeout and the 120s batch watchdog fires. Run the CLI child asynchronously with a separate deadline and explicit stdin policy so a blocked child produces a bounded test failure. Assert that the test event loop advances during config get; this was red with spawnSync and green with spawn.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe pull request updates desktop titlebar layout and zoom metrics, applies window-chrome handlers to the mobile header in the desktop shell, and changes CLI configuration tests to run child processes asynchronously. ChangesDesktop titlebar behavior
Asynchronous CLI configuration tests
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The changes appear mergeable after normal checks. Rendered layout and the hosted-runner hang fix still need the planned validation. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 4 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c50638e290
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <header className="mobile-topbar" inert={navOpen}> | ||
| {/* At narrow widths the sidebar strip is hidden and the main strip scrolls away, so in | ||
| the desktop shell the sticky header is the window's drag surface. */} | ||
| <header className="mobile-topbar" inert={navOpen} {...(desktopShell ? windowChromeHandlers() : {})}> |
There was a problem hiding this comment.
At widths up to 760px, once .main-top scrolls away, this header is intended to become the desktop window's drag surface, but nearly all of it is occupied by buttons: the menu and action controls are buttons, and the brand is also a button whose flex: 1 1 auto fills the remaining width. windowChromeHandlers() deliberately ignores events originating inside any button, leaving only the 2px gaps and narrow outer padding as draggable targets. In a narrow Tauri window users therefore still have no practical way to drag or double-click-maximize the window after scrolling; reserve an explicit non-interactive drag region or otherwise keep a usable part of the header outside the button elements.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 62 / 80
데스크톱 앱에서 Combos 화면은 높이를 창 전체로 잡습니다. 사용량 막대가 그 위에 있으면, 막대만큼 높이를 빼야 페이지가 아래로 안 넘칩니다. 그 계산은 막대가
바탕은 라인 - 라인 - 메인테이너의 판단이 필요한 지점 좁은 데스크톱 헤더에서 창을 끄는 빈 칸을 남길지 정해야 합니다. 브랜드 글자까지 끌기로 쓰면, 홈으로 가는 클릭과 창 이동이 겹칩니다. 너의 추천 넘침, 메뉴가 가려지던 것, 확대 시 신호등 자리, CLI 묶음 멈춤은 이대로 두세요. 머지 전에 헤더 한쪽에 빈 칸을 두고, 그 칸에만 지금 핸들러가 닿게 하세요. 계획 4번은 이 브랜치에서 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
A second review of
devafter #5910, #5918 and #5924 landed found three layout defects from the integrated title bar (#5910) and a CI hang that now failstest 4/4ondevand other PRs.Desktop title bar (#5910 follow-up)
100dvhCombos shell below the quota bar only matched a bar that is a direct child of.main. The desktop shell puts the bar inside.main-top, so the shell kept a full viewport height below the 40px strip. The same rules now key on.main-top, including the ≤760px row-filling rule..main-topand.mobile-topbarwere both sticky attop: 0at the same stacking level, so the strip painted over the menu. The strip now scrolls with the page (position: static; z-index: auto), as the quota bar already did there. The mobile header gets the window drag and double-click handlers in the desktop shell, so the window can still be dragged after scrolling. Buttons are excluded from dragging, as before.CI hang (
test 4/4,tests/clibatch)tests/cli/cli-config-show-client.test.tsran the CLI withspawnSync. On the hosted Linux runner, a child that stopped responding never returned control to the event loop, even with the 40s spawn timeout. The whole 12-file batch then hit the runner's 120s watchdog (exit 124), and each file passed when run alone. This happened on thedevdispatch run atf32f9aabd7, on #5924's first attempt, and on #5928. The test now uses an asyncspawnwith ignored stdin and its own SIGKILL deadline. A stuck child becomes a named test failure instead of a batch timeout. A new assertion checks that the event loop advances during the spawn; it fails withspawnSync. The Linux trigger itself did not reproduce outside the hosted runner, so this PR's exact-head CI is the evidence.Reviewed and left alone: after a zero-output reset, a failed native Chat replacement surfaces the original stream error as
upstream_sse_error. The Responses post-header path (deferProtocolSafeResetRecovery) does the same, so this is a design question for both paths, not a regression from #5918.Screenshots
Desktop shell user agent in headless Chrome against this branch's GUI build (throwaway
HOME,OPENCODEX_HOME,CODEX_HOME).Combos at 1280×800: the page no longer overflows (
scrollHeight - innerHeight = 0).700px window after a 400px scroll: the strip has scrolled away (top −347px), the header stays on top, and a hit test at the menu's center lands on the menu.
360pt window at 300% zoom (120 CSS px): the inset is 27px and the menu spans x=27–71.
Verification
gui/tests/app-titlebar.test.tsx: 9 pass. The two new cases (zoom-in clearance, strip and header contract) fail on the previous source.bun run lint:gui, root andguitsc --noEmit,bun run build:gui,bun run structure:check,bun run privacy:scan: pass.tests/clibatch files in one process: 83 pass locally. The fix author also ran the 12-file batch on a Linux host (155 pass).test 4/4hang.Checklist
devlog/_plan/260926_bug_train_6/030_post_merge_fixes.md).Summary by CodeRabbit