Skip to content

fix(gui): prevent blank scrolling past the Usage report (carry #6089) - #6218

Merged
lidge-jun merged 3 commits into
devfrom
codex/carry-6089-usage-scroll
Sep 28, 2026
Merged

lidge-jun merged 3 commits into
devfrom
codex/carry-6089-usage-scroll

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Summary

Carries #6089 by @fflake33 onto current dev: scrolling past the end of the Usage report no longer opens a large empty page.

The cache-hit-rate explanations from #5333 are absolutely positioned sr-only spans inside the capped models table. With no positioned ancestor inside that scrollport, their overflow extended the outer document (about 1,335 CSS px on the reported page). .usw-section .tbl-wrap now gets position: relative, so the captions stay inside the table's own scroll area; screen-reader text, internal table scrolling and last-row reachability are unchanged.

  • gui/src/styles-usage-workspace.css: the containing block.
  • gui/tests/usage-scroll-containment.test.ts: CI guard for the rule; gui/tests/usage-scroll-browser.ts: explicitly invoked real-browser geometry regression (happy-dom does no layout).
  • gui/tests/usage-custom-range.test.tsx: teardown restores global property descriptors.
  • structure/dashboard-and-usage.md: containment contract.

The three source commits are cherry-picked unchanged with their original author; the merge commit that refreshed the fork branch is dropped. #6089 stayed in draft on its contributor readiness checklist and its fork CI needed maintainer approval, so this maintainer carry replaces it.

Screenshots

Synthetic fixture with the repository's real stylesheets, from #6089 (no user data).

Before: the document scrolls about 946px beyond the report.

Before: scrolling past the Usage fixture produces an empty page

After: scrolling stops at the report boundary; the table still scrolls internally.

After: the Usage fixture remains visible at the bottom

Verification

  • cd gui && bun test tests/usage-layout.test.ts tests/usage-custom-range.test.tsx tests/usage-scroll-containment.test.ts: 44 pass / 0 fail on this branch.
  • cd gui && bun run lint and bun run build: pass (existing large-chunk warning only).
  • bun run typecheck, bun run structure:check, bun run privacy:scan: pass.
  • The browser geometry test (gui/tests/usage-scroll-browser.ts) needs a live browser session and was not rerun here; on fix(gui): prevent blank scrolling past the Usage report #6089's head the author reported 4/4 at 1280px and 390px with 0px excess scroll, and the production CSS is identical.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Closes #6089

Co-authored-by: Jian Gong fflake33@icloud.com

Summary by CodeRabbit

  • Bug Fixes
    • Usage tables now keep screen-reader captions anchored correctly within their scrollable areas.
  • Tests
    • Added checks for table scroll containment across desktop and mobile layouts, including short and long reports.
    • Improved test cleanup to restore global properties accurately.
  • Documentation
    • Updated usage-accounting documentation with information about table scroll behavior and its coverage.

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.
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 28, 2026 23:27
@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-28T23:29:35.913720Z 16cb602 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions github-actions Bot added the bug Something isn't working label Sep 28, 2026
@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 6edb61b6-d355-4fc0-b17b-1c3d66c65e9f

📥 Commits

Reviewing files that changed from the base of the PR and between ab58072 and 16cb602.

📒 Files selected for processing (5)
  • gui/src/styles-usage-workspace.css
  • gui/tests/usage-custom-range.test.tsx
  • gui/tests/usage-scroll-browser.ts
  • gui/tests/usage-scroll-containment.test.ts
  • structure/dashboard-and-usage.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The Usage table wrapper now contains absolutely positioned screen-reader captions within its scrollable area. Source-level and browser tests check scroll boundaries, last-row reachability, and caption presence across short and long tables at desktop and mobile widths.

Changes

Usage table scroll containment

Layer / File(s) Summary
Position the Usage table scrollport
gui/src/styles-usage-workspace.css:150-151, gui/tests/usage-scroll-containment.test.ts:1-10
The table wrapper uses position: relative. A stylesheet test checks this declaration and overflow-y: auto.
Verify scroll boundaries in a browser
gui/tests/usage-scroll-browser.ts:1-90, gui/tests/usage-custom-range.test.tsx:13-54, structure/dashboard-and-usage.md:295
The browser script checks outer-document bounds, table scrolling, last-row reachability, and caption presence at two viewport widths with short and long tables. Test teardown restores saved global property descriptors. The documentation identifies the styling and browser test.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 16cb6

The Usage table containment change is mergeable after normal checks; no actionable scroll-boundary issue remains identified.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning Issue #6089 requires the Usage containment fix, descriptor-safe teardown, documentation, and a real-browser geometry regression. The CSS change in gui/src/styles-usage-workspace.css sets `.usw-secti… Fix gui/tests/usage-scroll-browser.ts so the assertion compares the document's actual scroll extent after scrolling with the report boundary. The test must fail when `document.documentElement.scrollHeight - document.documentElement.client…
Docstring Coverage ⚠️ Warning 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 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the GUI fix that prevents blank scrolling beyond the Usage report. It matches the primary change described in the pull request.
Out of Scope Changes check ✅ Passed The five changed files remain within Issue #6089. The CSS rule fixes the required containing block. The browser script and source-level guard test the required containment and geometry behavior. The c…
Full details: Linked Issues check

Explanation

Issue #6089 requires the Usage containment fix, descriptor-safe teardown, documentation, and a real-browser geometry regression. The CSS change in gui/src/styles-usage-workspace.css sets .usw-section .tbl-wrap to position: relative and keeps overflow-y: auto. gui/tests/usage-custom-range.test.tsx saves and restores full global property descriptors. structure/dashboard-and-usage.md documents the contract. The browser fixture in gui/tests/usage-scroll-browser.ts uses the source stylesheet, synthetic short and long tables, and 1280px and 390px widths. However, lines 67-70 compute expectedBottom = Math.min(w.innerHeight, main.getBoundingClientRect().bottom) and then blankBeyondMain = Math.max(0, expectedBottom - main.getBoundingClientRect().bottom). Because expectedBottom is always less than or equal to main bottom, blankBeyondMain is always 0. The regression therefore does not detect excess outer-document scroll. The reported passing browser run does not establish this requirement.

Resolution

Fix gui/tests/usage-scroll-browser.ts so the assertion compares the document's actual scroll extent after scrolling with the report boundary. The test must fail when document.documentElement.scrollHeight - document.documentElement.clientHeight exceeds the allowed report-end distance, while retaining the short/long and desktop/mobile cases. Rerun the browser regression after correcting the measurement.

Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 66 / 80

사용량 화면에서 모델 표를 끝까지 내린 뒤에도, 페이지 아래에 큰 빈 칸이 남습니다. 캐시 적중률 칸에 눈에는 안 보이고 소리로만 읽어 주는 설명(.sr-only)이 붙어 있는데, 이 글은 position: absolute입니다. 표 상자(.tbl-wrap)는 높이가 막혀 표 안에서만 스크롤되는데, 절대 위치의 기준이 표 안이 아니면 그 높이가 문서 전체로 새어 나갑니다. 이 PR은 .usw-section .tbl-wrap에 position: relative를 하나 더해, 숨은 설명의 기준을 표 스크롤 상자로 고정합니다. 읽어 주는 문장·표 안 스크롤·마지막 줄까지 닿는지는 그대로 둡니다.

내용은 기여자 PR #6089의 세 커밋을 현재 dev 위에 다시 올린 메인테이너 캐리입니다. 스타일 한 줄 고침 외에, CI가 그 규칙을 글자로 잠그는 테스트, 실제 브라우저에서 빈 칸을 재는 수동 스크립트, usage-custom-range 테스트가 전역 속성을 되돌릴 때 설명이 깨지지 않게 고치는 정리, structure/dashboard-and-usage.md 한 문장이 함께 들어갑니다. types.ts·config.ts 분할이나 미리보기 배포 이야기는 없습니다.

라인 - gui/tests/usage-scroll-containment.test.ts — 자동 검사가 돌리는 테스트는 CSS에 position: relative와 overflow-y: auto가 적혀 있는지만 봅니다. 빈 칸이 실제로 사라졌는지는 재지 않습니다. 그걸 재는 파일은 gui/tests/usage-scroll-browser.ts인데, BSK_SESSION이 있는 수동 실행용이고 CI에는 안 들어갑니다. PR 본문도 이 브랜치에서는 그 브라우저 검사를 다시 돌리지 않았다고 적습니다. #6089에서 통과했다는 기록과 CSS가 같다는 점에 기대는 구조입니다.

라인 - #6089 — 같은 고침의 원본 드래프트가 아직 열려 있습니다. 이 캐리가 Closes #6089를 걸었으니, 머지 전후에 원본을 superseded로 닫아 중복을 없애면 됩니다.

라인 - gui/tests/usage-custom-range.test.tsx — 전역을 값만 저장했다가 다시 심던 방식에서, 원래 속성 설명(descriptor)을 그대로 되돌리도록 바꿨습니다. 캐시 히트 고침과 직접 같지는 않지만, 같은 화면 테스트가 전역을 더럽히지 않게 막는 정리라 방향이 맞습니다.

메인테이너의 판단이 필요한 지점

happy-dom은 화면 배치를 재지 못해서, CI에는 글자 계약만 남고 빈 칸 측정은 수동 bsk 스크립트에만 있습니다. 그 한계를 알고 받을지, 아니면 나중에 Playwright 같은 CI 기하 검사로 올릴지. 또 #6089 드래프트를 이 캐리 머지와 함께 닫을지.

너의 추천

고치는 방향이 맞고 범위도 작습니다. base dev도 맞습니다. CI가 초록이 되면 dev에 넣는 것을 권합니다. 머지하면 #6089는 닫으세요. 브라우저 기하 검사는 이번에 다시 안 돌렸으니, 머지 전에 한 번만 눈으로 확인하거나 bsk 스크립트를 돌리면 더 안심입니다. 닫을 다른 중복 PR은 #6089뿐입니다.

이 댓글은 grok-bot이 작성했습니다

@lidge-jun
lidge-jun merged commit 2e35b25 into dev Sep 28, 2026
35 checks passed
@lidge-jun
lidge-jun deleted the codex/carry-6089-usage-scroll branch September 28, 2026 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants