Fix file list height on iOS/Android by measuring chrome dynamically#396
Merged
nitrobass24 merged 3 commits intodevelopfrom Apr 22, 2026
Merged
Fix file list height on iOS/Android by measuring chrome dynamically#396nitrobass24 merged 3 commits intodevelopfrom
nitrobass24 merged 3 commits intodevelopfrom
Conversation
PR #371 hardcoded 160px (desktop) and 200px (mobile) deductions for the viewport calc(). The estimates are too large on iOS Chrome/Safari because they count elements (e.g. a 56px navbar, 52px title bar) that don't match the actual rendered chrome — leaving a gap of empty space below the file list. They also ignore the notification banner (0px when empty, taller when shown) and file-options wrapping. Replace the guesswork with a ResizeObserver on #top-header, #file-options, and the #file-list host. Measure the virtual viewport's page-Y offset (= everything stacked above it) and expose it via --file-list-chrome-height so the calc() subtracts the real value on any device or breakpoint. The media query can now go away since measurement is intrinsically responsive. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFile-list viewport sizing moved from compile-time SASS vars to a runtime CSS custom property. A ResizeObserver measures header/options/host positions, schedules RAF-updated writes to Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/angular/src/app/pages/files/file-list.component.spec.ts`:
- Around line 446-453: The afterEach cleanup leaves a fake ResizeObserver
installed when the environment originally lacked it because it only restores
when originalResizeObserver is truthy; update the cleanup in the afterEach block
that references originalResizeObserver/ResizeObserver to restore when
originalResizeObserver is defined and otherwise delete the global ResizeObserver
(e.g. use delete (globalThis as any).ResizeObserver) so tests that start without
ResizeObserver do not retain the fake between tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d9958ed-260c-4f4c-b633-0b8a1c8d7bdc
📒 Files selected for processing (3)
src/angular/src/app/pages/files/file-list.component.scsssrc/angular/src/app/pages/files/file-list.component.spec.tssrc/angular/src/app/pages/files/file-list.component.ts
If the test environment shipped without ResizeObserver, the previous afterEach left the fake installed because it only restored on a truthy original. Delete the global when the original was undefined so later tests in the worker start clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/angular/src/app/pages/files/file-list.component.spec.ts`:
- Around line 406-456: The test redefines window.scrollY in beforeEach but never
restores it, leaking a changed global into other specs; capture the original
property descriptor (e.g., const originalScrollYDescriptor =
Object.getOwnPropertyDescriptor(window, 'scrollY') or from globalThis) when you
override scrollY in the setup and then in afterEach restore it: if
originalScrollYDescriptor is present use Object.defineProperty(window,
'scrollY', originalScrollYDescriptor) else delete (or define as undefined) the
property to return the environment to its prior state; update the
beforeEach/afterEach blocks around the ResizeObserver/requestAnimationFrame
logic and reference the scrollY override locations to ensure cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: df2e3ad4-21bd-4c4a-aa6b-51f9d21d35e6
📒 Files selected for processing (1)
src/angular/src/app/pages/files/file-list.component.spec.ts
…ests Capture scrollY's original property descriptor before overriding it to 0, then restore it (or delete, if the environment had none) in afterEach. The override previously leaked into later specs as a static 0 value, masking JSDOM's live getter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR #371 fixed Android scroll, but iOS/Chrome still shows empty space below the file list. The 160px (desktop) and 200px (mobile) deductions hardcoded in the viewport
calc()are larger than the actual rendered chrome on iOS, and they also don't adapt to notification banners or different#file-optionswrapping at intermediate widths.ResizeObserveron#top-header,#file-options, and the#file-listhost (the last covers the column header and bulk-action-bar that live above.file-viewportinside the host)..file-viewport.getBoundingClientRect().top + window.scrollY— everything stacked above the list flows into that offset — and expose it as--file-list-chrome-height.checkViewportSize()so CDK re-measures after the height updates.Works the same across iOS Safari/Chrome, Android Chrome/Firefox, and desktop because it measures the actual rendered layout rather than guessing per-breakpoint.
Test plan
cd src/angular && npx ng lint— no new warnings in the changed filescd src/angular && npx ng test— 322 tests pass (3 new)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests