Fix iOS Safari viewport shrinking in dashboard - #81
Open
willianpm wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds a useViewportHeight hook that computes and syncs --app-dvh and --app-vv-offset-top CSS variables (from visualViewport or window), clamps scroll roots, includes tests, and wires the hook into App, DashboardLayout, and PollDrafts to replace hardcoded dvh-based sizing. ChangesViewport Height Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
dashboard/frontend/src/app/components/DashboardLayout.tsxParsing error: 'import' and 'export' may appear only with 'sourceType: module' dashboard/frontend/src/app/lib/useViewportHeight.tsParsing error: 'import' and 'export' may appear only with 'sourceType: module' 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
dashboard/frontend/src/app/lib/useViewportHeight.test.tsx (2)
47-56: ⚡ Quick winAssert listener cleanup by unmounting the component.
This test currently verifies registration but not teardown. Add an
unmount()assertion so cleanup regressions are caught.Proposed diff
- render(<TestComponent />); + const { unmount } = render(<TestComponent />); @@ expect(addEventListener).toHaveBeenCalledWith('resize', expect.any(Function)); expect(addEventListener).toHaveBeenCalledWith('scroll', expect.any(Function)); expect(removeEventListener).not.toHaveBeenCalled(); + + unmount(); + expect(removeEventListener).toHaveBeenCalledWith('resize', expect.any(Function)); + expect(removeEventListener).toHaveBeenCalledWith('scroll', expect.any(Function));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dashboard/frontend/src/app/lib/useViewportHeight.test.tsx` around lines 47 - 56, The test registers viewport listeners but never unmounts the TestComponent to assert cleanup; after render(<TestComponent />) and the existing waitFor assertion, call the returned unmount() from render (e.g., const { unmount } = render(...)) and then assert that removeEventListener was called for both 'resize' and 'scroll' (use expect(removeEventListener).toHaveBeenCalledWith('resize', expect.any(Function)) and similarly for 'scroll') and/or that removeEventListener was called the expected number of times instead of leaving only the registration checks in the useViewportHeight test.
25-29: ⚡ Quick winRestore
window.innerHeightafter overriding it in tests.Line 61 mutates a global descriptor, but cleanup does not restore it. This can cause cross-test leakage as the suite grows.
Proposed diff
describe('useViewportHeight', () => { + const originalInnerHeight = Object.getOwnPropertyDescriptor(window, 'innerHeight'); + afterEach(() => { document.documentElement.style.removeProperty('--app-dvh'); setVisualViewport(undefined); + if (originalInnerHeight) { + Object.defineProperty(window, 'innerHeight', originalInnerHeight); + } });Also applies to: 61-64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dashboard/frontend/src/app/lib/useViewportHeight.test.tsx` around lines 25 - 29, The test mutates the global window.innerHeight descriptor but the afterEach cleanup only removes --app-dvh and calls setVisualViewport(undefined); capture the original window.innerHeight descriptor before overriding (e.g., const originalInnerHeight = Object.getOwnPropertyDescriptor(window, 'innerHeight')) and restore it in the afterEach along with the existing cleanup (use Object.defineProperty(window, 'innerHeight', originalInnerHeight) if the descriptor exists); update the test file where you override window.innerHeight (the setup logic around the innerHeight override) to save the descriptor so afterEach can restore it to prevent cross-test leakage.dashboard/frontend/src/app/lib/useViewportHeight.ts (1)
23-29: ⚡ Quick winAvoid redundant CSS var writes on high-frequency events.
Line 43 triggers immediate updates for every
visualViewportscroll/resize event, and Line 28 writes the CSS variable even when unchanged. Guarding duplicate writes will reduce style churn on iOS keyboard animations and scroll bursts.Proposed diff
function setViewportHeightVar(height: number) { if (typeof document === 'undefined') { return; } const nextHeight = Math.round(height); if (!Number.isFinite(nextHeight) || nextHeight <= 0) { return; } - document.documentElement.style.setProperty(VIEWPORT_VAR, `${nextHeight}px`); + const nextValue = `${nextHeight}px`; + if (document.documentElement.style.getPropertyValue(VIEWPORT_VAR) === nextValue) { + return; + } + document.documentElement.style.setProperty(VIEWPORT_VAR, nextValue); }Also applies to: 40-56, 67-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dashboard/frontend/src/app/lib/useViewportHeight.ts` around lines 23 - 29, The update routine in useViewportHeight.ts is writing the CSS var on every visualViewport event even when unchanged; change the handler (the code that computes nextHeight and calls document.documentElement.style.setProperty with VIEWPORT_VAR) to skip setting the property if the numeric value equals the last-applied value (store a module-level or closure-scoped lastHeight) or compare against getComputedStyle(document.documentElement).getPropertyValue(VIEWPORT_VAR) before calling setProperty, and also throttle/debounce the visualViewport listener if needed for bursty events; apply the same guard pattern to the other update points mentioned (the blocks around lines 40-56 and 67-69).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@dashboard/frontend/src/app/lib/useViewportHeight.test.tsx`:
- Around line 47-56: The test registers viewport listeners but never unmounts
the TestComponent to assert cleanup; after render(<TestComponent />) and the
existing waitFor assertion, call the returned unmount() from render (e.g., const
{ unmount } = render(...)) and then assert that removeEventListener was called
for both 'resize' and 'scroll' (use
expect(removeEventListener).toHaveBeenCalledWith('resize', expect.any(Function))
and similarly for 'scroll') and/or that removeEventListener was called the
expected number of times instead of leaving only the registration checks in the
useViewportHeight test.
- Around line 25-29: The test mutates the global window.innerHeight descriptor
but the afterEach cleanup only removes --app-dvh and calls
setVisualViewport(undefined); capture the original window.innerHeight descriptor
before overriding (e.g., const originalInnerHeight =
Object.getOwnPropertyDescriptor(window, 'innerHeight')) and restore it in the
afterEach along with the existing cleanup (use Object.defineProperty(window,
'innerHeight', originalInnerHeight) if the descriptor exists); update the test
file where you override window.innerHeight (the setup logic around the
innerHeight override) to save the descriptor so afterEach can restore it to
prevent cross-test leakage.
In `@dashboard/frontend/src/app/lib/useViewportHeight.ts`:
- Around line 23-29: The update routine in useViewportHeight.ts is writing the
CSS var on every visualViewport event even when unchanged; change the handler
(the code that computes nextHeight and calls
document.documentElement.style.setProperty with VIEWPORT_VAR) to skip setting
the property if the numeric value equals the last-applied value (store a
module-level or closure-scoped lastHeight) or compare against
getComputedStyle(document.documentElement).getPropertyValue(VIEWPORT_VAR) before
calling setProperty, and also throttle/debounce the visualViewport listener if
needed for bursty events; apply the same guard pattern to the other update
points mentioned (the blocks around lines 40-56 and 67-69).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3220c78a-e9bf-4d7c-a97e-da4c91d4a8ed
📒 Files selected for processing (5)
dashboard/frontend/src/app/App.tsxdashboard/frontend/src/app/components/DashboardLayout.tsxdashboard/frontend/src/app/lib/useViewportHeight.test.tsxdashboard/frontend/src/app/lib/useViewportHeight.tsdashboard/frontend/src/app/pages/PollDrafts.tsx
Add --app-vv-offset-top CSS var and update useViewportHeight to set offset and clamp the dashboard s croll root. Prevents leftover black bar and content jumping on iOS Safari when the virtual keyboard opens/closes .
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dashboard/frontend/src/app/components/DashboardLayout.tsx (1)
52-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMobile header does not respect viewport offset.
The mobile header uses
fixed top-0, which positions it at the top of the viewport regardless of the parent container's--app-vv-offset-topoffset. On iOS Safari, when the visualViewport has a non-zero offset (e.g., virtual keyboard open, status bar area), the header will be positioned at viewporttop: 0while the root content starts attop: var(--app-vv-offset-top), creating a misalignment.🔧 Proposed fix to align mobile header with viewport offset
- <div className="lg:hidden fixed top-0 left-0 right-0 z-50 bg-[`#5865F2`] dark:bg-[`#4752C4`] text-white p-4 flex items-center justify-between"> + <div className="lg:hidden fixed top-[var(--app-vv-offset-top,0px)] left-0 right-0 z-50 bg-[`#5865F2`] dark:bg-[`#4752C4`] text-white p-4 flex items-center justify-between">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dashboard/frontend/src/app/components/DashboardLayout.tsx` around lines 52 - 87, The mobile header div in DashboardLayout.tsx (the element with className "lg:hidden fixed top-0 left-0 right-0 z-50 ...") is fixed to top:0 and ignores the app visual-viewport offset; change it to use the CSS variable for the viewport offset so it aligns with the root content by replacing the hard-coded top:0 with an inline style or class that sets top: 'var(--app-vv-offset-top)' (e.g., add style={{ top: 'var(--app-vv-offset-top)' }} while keeping left-0 and right-0 and the existing classes), ensuring the header respects the visualViewport offset on iOS Safari.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@dashboard/frontend/src/app/components/DashboardLayout.tsx`:
- Around line 52-87: The mobile header div in DashboardLayout.tsx (the element
with className "lg:hidden fixed top-0 left-0 right-0 z-50 ...") is fixed to
top:0 and ignores the app visual-viewport offset; change it to use the CSS
variable for the viewport offset so it aligns with the root content by replacing
the hard-coded top:0 with an inline style or class that sets top:
'var(--app-vv-offset-top)' (e.g., add style={{ top: 'var(--app-vv-offset-top)'
}} while keeping left-0 and right-0 and the existing classes), ensuring the
header respects the visualViewport offset on iOS Safari.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea84c8ed-0399-43be-a702-aac978fe03d1
📒 Files selected for processing (2)
dashboard/frontend/src/app/components/DashboardLayout.tsxdashboard/frontend/src/app/lib/useViewportHeight.ts
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
Root Cause
On iOS Safari, the visual viewport stays reduced after the virtual keyboard closes when the app uses a fixed-height scroll container (h-dvh) inside body with overflow hidden. The dynamic viewport unit does not reliably re-expand, so the container keeps the smaller height and the page appears permanently shrunk.
Fix
Tests
Type Safety
No dedicated typecheck script exists for this repo.
Regression Considerations
Summary by CodeRabbit
New Features
Bug Fixes
Tests