Repository navigation
Conversation
Absolutely positioned screen-reader captions escaped the capped table's overflow and enlarged the outer document, allowing scrolling into blank space after the Usage report ended. Establish the table scrollport as their containing block and cover long and short tables at desktop and mobile widths.
|
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 (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe Usage table wrapper now establishes a positioning context for screen-reader captions. New tests check its CSS declarations and browser behavior at desktop and mobile widths. The custom-range test also restores global property descriptors during teardown. ChangesUsage table scroll containment
Test global restoration
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue was found. The rendered browser check still needs to be run as part of normal readiness checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 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 |
|
⏳ DRAFT
What to do
Review readiness checklist
2/4 boxes ticked. This PR stays in draft until every box above is ticked. |
리뷰 · 우선순위 48 / 80사용량 화면은 표 끝까지 내린 뒤에도 아래로 빈 페이지가 남습니다. PR 설명에 적힌 실제 화면에서는 그 빈 높이가 약 1335픽셀입니다. 모델 표의 캐시 칸에는 눈에는 안 보이고 소리로만 읽어 주는 설명이 붙어 있습니다. 표 상자는 높이가 막혀 표 안에서만 스크롤되는데, 그 설명의 위치 기준이 표 밖(문서 전체)이라 막힌 높이가 페이지 아래로 새어 나갑니다. 이 PR은 사용량 구역의 표 상자( gui/tests/usage-scroll-containment.test.ts:8 - 깃허브 자동 검사가 돌리는 테스트는 스타일 파일에 메인테이너의 판단이 필요한 지점 너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
@coderabbitai review Preparing this draft for review readiness. Please review the current head, including the scope and limitations of the source-contract test versus the rendered browser regression. |
|
|
@coderabbitai review Head is now bb40ef8. The earlier request was interrupted by the test-isolation fix; no further code changes are planned while this review runs. |
✅ Action performedReview finished.
|
|
Readiness update: the combined Usage test failure is fixed in bb40ef8 by restoring original global property descriptors (and deleting originally absent globals) during teardown. The focused combined run now passes 44/44; the full GUI suite completed with 2,589 passed and 0 failed. GUI lint also passes. The full root suite is still running. The browser-geometry regression remains explicitly local; the CI source-contract guard does not claim to measure browser layout. |
|
Readiness attestation for bb40ef8 is complete in the PR description. The full GUI suite and rendered regression pass; root full-suite failures and the clean-dev comparison are documented under the scoped validation exception. All PR-only failure names were rechecked: transient plugin/sidecar failures cleared, and the remaining native-Codex assertion also fails on clean dev. CodeRabbit reviewed this head with no actionable comments, and there are no unresolved review threads. All four repository-managed readiness boxes are checked; exact-head hosted CI remains required before merge. |
|
Reviewed exact head |
|
Exact-head source re-review at |
Ingwannu
left a comment
There was a problem hiding this comment.
Approved exact head 4dcb745d980532906d7e8e99689a23cef34b1c70. The containment fix is narrowly scoped, preserves screen-reader copy and table scrolling, creates no new stacking context, and the test descriptor cleanup is correct. Exact-head Cross-platform CI and React Doctor are now green, the current-dev virtual merge is clean, and no P0–P2 defect or unresolved review thread remains. The author/maintainer can now complete draft readiness and the final UI evidence pass.
|
@lidge-jun exact-head approval is in and hosted CI is green. Please take the final UI/readiness pass; I left the author-owned draft state unchanged. |
Summary
Fix scrolling past the end of the Usage report into a large empty page.
The cache-hit-rate explanations added in #5333 include absolutely positioned
sr-onlyspans inside the capped models table. Without a positioned ancestor inside that scrollport, their overflow contributes to the outer document instead of staying inside the table. On the reported page, scrolling the body to the report's end still left approximately 1,335 CSS pixels of outer-document scroll range..usw-section .tbl-wrapas the captions' containing block withposition: relative.structure/dashboard-and-usage.md.Related to #5333, but not a replacement for its cache-rate or horizontal-scrolling improvements. No duplicate issue or PR was found. No API, dependency, visible-copy, or user workflow changes.
Screenshots
Synthetic regression fixture using the repository's real stylesheets, captured in Chromium after attempting to scroll beyond the report's end. These contain no user usage data. The before image restores the original
position: staticbehavior; the after image uses this fix. Images live only on the fork's separatepr-assetsbranch and are pinned by commit SHA.Before: the document scrolls approximately 946px beyond the fixture report.
After: scrolling stops at the report boundary; the table remains independently scrollable.
Verification
Current head — 2026-09-28
Merged current
devat754fcef2finto the existing PR branch without conflicts. Exact head:4dcb745d980532906d7e8e99689a23cef34b1c70. The diff against this base remains the original five files; this refresh adds no new behavior or dependencies.cd gui && BSK_SESSION=<active-agent-window> bun tests/usage-scroll-browser.ts— 4/4 passed on this head in Chromium 152. At 1280px and 390px, both short and long tables measured 0px of excess outer scroll; captions remained present, the last row was reachable, and long tables scrolled internally. This is an explicitly invoked browser test, not a hosted CI test. The existing synthetic before/after screenshots above remain applicable to the unchanged production fix.cd gui && bun test tests/usage-layout.test.ts tests/usage-custom-range.test.tsx tests/usage-scroll-containment.test.ts— 44 passed, 0 failed, 619 assertions.cd gui && bun test tests— 2,625 passed, 0 failed, 304 files, 22,016 assertions.bun run typecheck— passed.cd gui && bun run lint— passed.cd gui && bun run build— passed; existing large-chunk warning only.bun run structure:check— passed.bun run privacy:scan— passed.git diff upstream/dev...HEAD --check— passed.Dependencies were restored from the unchanged frozen lockfiles. The first root validation attempt stopped before running checks because Bun's installation hook had been skipped; completing the existing Bun package's installation hook resolved that setup issue. The successful results above are from the subsequent completed checks.
Exact-head hosted CI — maintainer action required
The push triggered both workflows for
4dcb745d980532906d7e8e99689a23cef34b1c70, but GitHub reportsaction_requiredbefore any jobs run:A repository maintainer must approve the fork workflow runs. The contributor account has read access to the upstream repository and cannot approve or dispatch its CI. These workflows have not passed; successful hygiene/target checks do not substitute for them. This addresses the stale branch and refreshes the browser evidence requested in the maintainer feedback; hosted CI and maintainer approval remain outstanding.
Root full-suite scope exception
The root full suite was not rerun for this merge-only refresh. The original five-file patch is unchanged, and the previous full head/base attempts below were disproportionately expensive and exposed unrelated failures. Current-head evidence is the complete GUI suite, the focused combined regression, the rendered-browser checks, and the other completed checks above. Root runtime and cross-platform coverage remain for the blocked hosted CI; no current-head full-suite success is claimed.
The following records concern the previous head
bb40ef837and its old base, not the current head:bun run testwas run, not skipped: it exited 1 with 86 distinct failing case names. A clean detached checkout of the exact base (24b2f39b7) also failed, reporting 94 distinct failing case names before the runner stopped it at its 900-second deadline (exit 124). These are not passing full-suite results. Seventy failing case names occur in both runs, including provider-management and Compatibility Lab cases.The 16 PR-only names were investigated with:
bun run test tests/lib/plugin-loader.test.ts tests/vision/sidecar-settings-vision-filter.test.ts tests/vision/sidecar-settings-web-search-gate.test.ts tests/vision/vision-backend-union.test.ts tests/web-search/web-search-backend-union.test.ts tests/codex-integration/native-codex-toggle.test.tsResult: 101 passed, 4 skipped, 1 failed. Plugin/vision/web-search timeout failures cleared without code changes. The remaining native-Codex off/on assertion (
state: absentversus expectedcurrent) reproduces identically on cleandev:bun run test tests/codex-integration/native-codex-toggle.test.tsgives 12 passed, 1 failed. The Anthropic Fast API failure also reproduces on both head and clean base withbun scripts/test.ts tests/adapters/anthropic/anthropic-fast-opt-in.test.ts: 7 passed, 1 failed, expecting HTTP 200 and receiving 400.The scoped validation exception is used because another full local run is disproportionate for this two-line CSS fix after both the PR run and the 15-minute baseline attempt exposed unrelated baseline/environment failures. Changed behavior is covered by the passing full GUI suite, 44-case focused combined run, four rendered-browser cases, typecheck, build, lint, structure and privacy checks. No root runtime files are changed. Required exact-head hosted CI and cross-platform validation remain for maintainers/CI before merge; this attestation does not claim those checks passed.
Review
The maintainer reviewed
bb40ef837and found no blocking P0–P2 defect or unresolved thread. CodeRabbit also reported no actionable comments for that previous head. The current head preserves the same patch on the refreshed base; those historical reviews are not presented as exact-head hosted CI or approval of this new commit.Checklist
Review readiness checklist