Skip to content

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

Closed
fflake33 wants to merge 4 commits into
lidge-jun:devfrom
fflake33:fix/usage-scroll-containment
Closed

fflake33 wants to merge 4 commits into
lidge-jun:devfrom
fflake33:fix/usage-scroll-containment

Conversation

@fflake33

@fflake33 fflake33 commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

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-only spans 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.

  • Establish .usw-section .tbl-wrap as the captions' containing block with position: relative.
  • Preserve screen-reader explanations, internal table scrolling, and last-row reachability.
  • Add a real-browser geometry regression using the actual stylesheets and synthetic table rows at 1280px and 390px, with both short and long tables. It is an explicitly invoked browser test because happy-dom does not perform layout.
  • Repair Usage custom-range test teardown so combined test runs preserve global property descriptors.
  • Document the containment contract in 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: static behavior; the after image uses this fix. Images live only on the fork's separate pr-assets branch and are pinned by commit SHA.

Before: the document scrolls approximately 946px beyond the fixture report.

Before: scrolling past the Usage fixture produces an empty page

After: scrolling stops at the report boundary; the table remains independently scrollable.

After: the Usage fixture remains visible at the bottom

Verification

Current head — 2026-09-28

Merged current dev at 754fcef2f into 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 reports action_required before 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 bb40ef837 and its old base, not the current head:

bun run test was 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.ts

Result: 101 passed, 4 skipped, 1 failed. Plugin/vision/web-search timeout failures cleared without code changes. The remaining native-Codex off/on assertion (state: absent versus expected current) reproduces identically on clean dev: bun run test tests/codex-integration/native-codex-toggle.test.ts gives 12 passed, 1 failed. The Anthropic Fast API failure also reproduces on both head and clean base with bun 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 bb40ef837 and 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

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

Review readiness checklist

  • Required local validation passed; commands, results, and any full-suite exception are documented.
  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • I resolved all correct Codex and CodeRabbit findings.
  • My PR is ready for review.

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.
@coderabbitai

coderabbitai Bot commented Sep 27, 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: 38e3b343-80c5-40f0-9530-58f0b545b0d1

📥 Commits

Reviewing files that changed from the base of the PR and between 24b2f39 and bb40ef8.

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

Changes

Usage table scroll containment

Layer / File(s) Summary
Table wrapper positioning and contract
gui/src/styles-usage-workspace.css, gui/tests/usage-scroll-containment.test.ts
The table wrapper uses relative positioning with vertical scrolling. A source-contract test checks both declarations.
Browser regression coverage
gui/tests/usage-scroll-browser.ts, structure/dashboard-and-usage.md
The browser script checks overflow, caption counts, long-table scrolling, and last-row reachability at desktop and mobile widths. The documentation describes these checks.

Test global restoration

Layer / File(s) Summary
Global descriptor restoration
gui/tests/usage-custom-range.test.tsx
The test captures each global’s property descriptor, then restores it or deletes the property during teardown.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: lidge-jun

Merge Risk: ⚪ Minimal · up to bb40e

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)

Check name Status Explanation Resolution
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 (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preventing blank scrolling beyond the Usage report. It matches the stylesheet fix, browser regression test, and documentation updates.
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
🧪 Generate unit tests (beta)
  • 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.

@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • missing_regression_test — Behavior changed under src/ or gui/src/ without a test change. Add focused coverage or obtain test-exception-approved.

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

github-actions Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (2/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 2/4).

Review readiness checklist

  • ✅ Required local validation passed; commands, results, and any full-suite exception are documented.
  • ✅ I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

2/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@github-actions github-actions Bot removed the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 27, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 48 / 80

사용량 화면은 표 끝까지 내린 뒤에도 아래로 빈 페이지가 남습니다. PR 설명에 적힌 실제 화면에서는 그 빈 높이가 약 1335픽셀입니다. 모델 표의 캐시 칸에는 눈에는 안 보이고 소리로만 읽어 주는 설명이 붙어 있습니다. 표 상자는 높이가 막혀 표 안에서만 스크롤되는데, 그 설명의 위치 기준이 표 밖(문서 전체)이라 막힌 높이가 페이지 아래로 새어 나갑니다.

이 PR은 사용량 구역의 표 상자(.usw-section .tbl-wrap)에 position: relative를 하나 더합니다. 그러면 숨은 설명의 기준이 표 스크롤 상자가 되어, 빈 공간은 문서 밖으로 나가지 않습니다. 읽어 주는 문장은 빼지 않습니다. 베이스는 dev이고, 같은 버그를 다루는 다른 열린 PR은 없습니다.

gui/tests/usage-scroll-containment.test.ts:8 - 깃허브 자동 검사가 돌리는 테스트는 스타일 파일에 position: relative와 overflow-y: auto라는 글자가 있는지만 확인합니다. 빈 페이지가 실제로 사라졌는지는 보지 않습니다. 그걸 재는 파일은 gui/tests/usage-scroll-browser.ts입니다. 이 파일은 bsk와 열려 있는 Agent Window가 있어야 하고, 자동 검사는 실행하지 않습니다. 글자만 남긴 채 배치가 다시 깨져도 자동 검사는 통과할 수 있습니다.

메인테이너의 판단이 필요한 지점
이 PR은 아직 드래프트입니다. 준비 체크 4칸은 모두 비어 있습니다. 작성자는 전체 테스트를 돌리지 않았고, usage-layout과 usage-custom-range를 같이 돌리면 3개가 실패한다고 적었습니다. 그 두 파일은 이 PR에서 바뀌지 않았고, layout만 따로 돌리면 통과한다고 합니다.
첫 커밋 직후 위생 검사 댓글은 회귀 테스트가 없다고 했습니다. 다음 커밋에서 글자 검사 테스트를 넣었고, 지금 위생 검사는 통과입니다. 그 옛 댓글로 막을 필요는 없습니다.
structure/dashboard-and-usage.md에 넣은 문장은 사용량 파일을 설명하는 문단 끝에 붙어 있습니다. 내용 자체는 이 고침과 맞습니다. 화면 규칙으로 문단을 나눌지는 취향입니다.

너의 추천
고치는 방향은 맞습니다. 표 상자를 숨은 설명의 기준 상자로 두면, 막힌 표 높이가 페이지 전체로 새지 않습니다. 닫을 중복 PR은 없습니다. 드래프트 체크가 끝나기 전에는 머지하지 않으면 됩니다. happy-dom은 화면 배치를 재지 못해서, 빈 칸 측정은 bsk 테스트에만 있고 자동 검사는 글자만 잠급니다. 그 한계를 알고 받으면 됩니다.

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

@fflake33

Copy link
Copy Markdown
Contributor Author

@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

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Head commit changed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@fflake33

Copy link
Copy Markdown
Contributor Author

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

@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@fflake33

Copy link
Copy Markdown
Contributor Author

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.

@github-actions
github-actions Bot marked this pull request as ready for review September 27, 2026 15:15
@fflake33

Copy link
Copy Markdown
Contributor Author

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.

@Ingwannu

Copy link
Copy Markdown
Owner

Reviewed exact head bb40ef837d1ad106f78c19eea554262582321f09. The position: relative containment correctly anchors the absolutely positioned .sr-only description inside the Usage scroller; I found no blocking P0–P2 defect or unresolved thread. Holding approval because this head is 93 commits behind current dev and has no hosted CI beyond hygiene/target/CodeRabbit. Please update onto current dev and run exact-head CI; the browser geometry behavior will still need the stated manual evidence because the relevant test is not executed in CI.

@fflake33
fflake33 marked this pull request as draft September 28, 2026 09:07
@fflake33
fflake33 marked this pull request as ready for review September 28, 2026 09:09
@github-actions
github-actions Bot marked this pull request as draft September 28, 2026 10:27
@Ingwannu

Copy link
Copy Markdown
Owner

Exact-head source re-review at 4dcb745d980532906d7e8e99689a23cef34b1c70 found no P0–P2 defect; the scroll containment, accessibility copy, stacking behavior, and descriptor cleanup are coherent, and the current-dev virtual merge is clean. I approved the latest React Doctor run and Cross-platform CI run. Keep the PR draft until those exact-head jobs are green; if they pass, this is an approval candidate. @lidge-jun please take the final UI/readiness pass after CI.

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Ingwannu

Copy link
Copy Markdown
Owner

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

@fflake33
fflake33 marked this pull request as ready for review September 28, 2026 16:12
@github-actions
github-actions Bot marked this pull request as draft September 28, 2026 16:13
lidge-jun added a commit that referenced this pull request Sep 28, 2026
…#6218)

Carries #6089: .usw-section .tbl-wrap becomes the containing block for the absolutely positioned sr-only cache-rate captions, so their overflow stays inside the table scrollport instead of extending the outer document.

Co-authored-by: Jian Gong <fflake33@icloud.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev as #6218 (2e35b25): your three commits were cherry-picked unchanged with you as author, and the squash commit carries Co-authored-by: Jian Gong. Exact-head CI passed on the carry. Thank you for the fix and the browser evidence.

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.

3 participants