fix: restore phone-safe dock behavior - #1355
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughChangesThe PR replaces phone fixed-inset viewport roots with a shared in-flow, viewport-bounded shell, updates calculator dock focus reset behavior, and strengthens phone geometry, resize, hydration, and source-structure tests. Phone viewport behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/globals.css`:
- Around line 2341-2349: Update the .phone-viewport-shell declarations inside
the max-width 639px media query to retain 100vh as the fallback, then move the
100svh min-height and height overrides into an `@supports` block for svh. Preserve
the existing sizing behavior while eliminating duplicate declarations flagged by
Stylelint.
🪄 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: CHILL
Plan: Pro
Run ID: ff8d646c-42fa-4b01-bed5-9e8b7cfcac64
📒 Files selected for processing (10)
docs/branch-review-ledger.mddocs/search-chrome-behaviour.mdsrc/app/globals.csssrc/components/ClinicalDashboard.tsxsrc/components/calculators/search-page.tsxsrc/components/clinical-dashboard/global-search-shell.tsxsrc/components/clinical-dashboard/use-hide-on-scroll.tstests/clinical-dashboard-merge-artifacts.test.tstests/ui-phone-scroll.spec.tstests/ui-tools.spec.ts
| @media (max-width: 639px) { | ||
| .phone-viewport-shell { | ||
| position: relative; | ||
| width: 100%; | ||
| min-height: 100vh; | ||
| height: 100vh; | ||
| min-height: 100svh; | ||
| height: 100svh; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Avoid duplicate declarations that fail Stylelint.
The 100vh/100svh fallback pattern triggers the configured declaration-block-no-duplicate-properties errors. Put the svh override in @supports so the stylesheet passes lint while preserving fallback behavior.
Proposed fix
`@media` (max-width: 639px) {
.phone-viewport-shell {
position: relative;
width: 100%;
min-height: 100vh;
height: 100vh;
- min-height: 100svh;
- height: 100svh;
}
}
+
+@supports (height: 100svh) {
+ `@media` (max-width: 639px) {
+ .phone-viewport-shell {
+ min-height: 100svh;
+ height: 100svh;
+ }
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @media (max-width: 639px) { | |
| .phone-viewport-shell { | |
| position: relative; | |
| width: 100%; | |
| min-height: 100vh; | |
| height: 100vh; | |
| min-height: 100svh; | |
| height: 100svh; | |
| } | |
| `@media` (max-width: 639px) { | |
| .phone-viewport-shell { | |
| position: relative; | |
| width: 100%; | |
| min-height: 100vh; | |
| height: 100vh; | |
| } | |
| } | |
| `@supports` (height: 100svh) { | |
| `@media` (max-width: 639px) { | |
| .phone-viewport-shell { | |
| min-height: 100svh; | |
| height: 100svh; | |
| } | |
| } | |
| } |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 2345-2345: Duplicate property "min-height" (declaration-block-no-duplicate-properties)
(declaration-block-no-duplicate-properties)
[error] 2346-2346: Duplicate property "height" (declaration-block-no-duplicate-properties)
(declaration-block-no-duplicate-properties)
🤖 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 `@src/app/globals.css` around lines 2341 - 2349, Update the
.phone-viewport-shell declarations inside the max-width 639px media query to
retain 100vh as the fallback, then move the 100svh min-height and height
overrides into an `@supports` block for svh. Preserve the existing sizing behavior
while eliminating duplicate declarations flagged by Stylelint.
Source: Linters/SAST tools
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45b6545917
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useEffect(() => { | ||
| if (!activeCalc) return undefined; | ||
| let cancelled = false; | ||
| if (!activeCalc) return; |
There was a problem hiding this comment.
When the required unit suite runs, tests/mobile-chrome-paint-contract.test.ts:62 still requires the exact source text if (!activeCalc) return undefined, so changing this branch to bare return makes that test—and therefore verify:cheap—fail before the UI fix can be handed off. Update the contract assertion to reflect the intentional cleanup-free effect form.
AGENTS.md reference: AGENTS.md:L167-L168
Useful? React with 👍 / 👎.
|
Reviewed at exact head 45b6545. No replacement hunk. The Safari scroll-owner/100dvh approach was stale and current main supersedes it. Source branch and worktree are preserved; this PR is closed only to remove the superseded review queue entry. |
1 participant
Summary
RAG impact
Summary by CodeRabbit
Bug Fixes
Tests
Documentation