fix: limit announcement modal height (#307) - #312
Conversation
WalkthroughThe pull request modifies three component files to address announcement modal viewport constraints. It adjusts Announcement icon styling, adds optional style customization to FlexCol, and significantly restructures Modal to use React Portal with height constraints, adjusted z-indices, and scrollable overflow behavior for content exceeding viewport height. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
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
🤖 Fix all issues with AI agents
In @.env:
- Line 2: Add a trailing newline to the .env file so it ends with a blank line;
update the file containing the line "GENERATE_SOURCEMAP=false" to ensure a final
newline character is present (POSIX-compliant end-of-file newline).
| @@ -1,2 +1,2 @@ | |||
| CI=false | |||
| GENERATE_SOURCEMAP=false | |||
| GENERATE_SOURCEMAP=false No newline at end of file | |||
There was a problem hiding this comment.
Add a trailing newline at the end of the file.
Per static analysis, the file is missing a blank line at the end. Many tools and POSIX conventions expect files to end with a newline.
Proposed fix
CI=false
-GENERATE_SOURCEMAP=false
+GENERATE_SOURCEMAP=false
+📝 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.
| GENERATE_SOURCEMAP=false | |
| GENERATE_SOURCEMAP=false | |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 2-2: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🤖 Prompt for AI Agents
In @.env at line 2, Add a trailing newline to the .env file so it ends with a
blank line; update the file containing the line "GENERATE_SOURCEMAP=false" to
ensure a final newline character is present (POSIX-compliant end-of-file
newline).
|
Thank you for your contribution! The following files seem unrelated to the UI fix, can you please roll them back to simplify the review process?
|
|
Please add link to related issue in PR description: |
b1140dd to
4ac1fc8
Compare
|
@bordalix Please review the flagged PR when you have time and let me know if there’s anything I should update. Happy to contribute |
bordalix
left a comment
bordalix
left a comment
There was a problem hiding this comment.
concept NACK
Instead of scrolling the whole modal content (see attached video) only the middle section (the 3 bullet points) should scroll.
Screen.Recording.2026-02-02.at.14.25.44.mov
There was a problem hiding this comment.
All the changes is this file seem not related to this PR.
Please remove.
| top: '0', | ||
| left: '0', | ||
| zIndex: 22, // Higher than overlay | ||
| zIndex: 90, |
There was a problem hiding this comment.
| zIndex: 90, | |
| zIndex: 90, // Higher than overlay |
keep comments
| position: 'absolute' as 'absolute', | ||
| pointerEvents: 'none' as 'none', // Allow clicks to pass through to children | ||
| position: 'fixed' as 'fixed', | ||
| pointerEvents: 'none' as 'none', |
There was a problem hiding this comment.
| pointerEvents: 'none' as 'none', | |
| pointerEvents: 'none' as 'none', // Allow clicks to pass through to children |
keep comments
| border: '1px solid var(--dark10)', | ||
| backgroundColor: 'var(--ion-background-color)', | ||
| pointerEvents: 'auto' as 'auto', // Enable clicks on the modal content | ||
| pointerEvents: 'auto' as 'auto', |
There was a problem hiding this comment.
| pointerEvents: 'auto' as 'auto', | |
| pointerEvents: 'auto' as 'auto', // Enable clicks on the modal content |
keep comments
| }, []) | ||
|
|
||
| return ( | ||
| return createPortal( |
There was a problem hiding this comment.
We need to evaluate the usage of createPortal when the app is used with expo (i.e. mobile apps)
|
👋 Hey @kushbosamiya — @bordalix requested changes on this PR back on February 2nd and it's been quiet since. Is this still something you plan to address, or should we close it? |
|
Hey @kushbosamiya — just checking in 👋 @bordalix requested changes a few days ago. Any progress or questions on the feedback? Happy to help unblock if needed. |
🔍 Arkana PR ReviewPR: wallet#312 — fix: limit announcement modal height (#307) Changes
Findings✅ Portal approach is correct. Using
✅ FlexCol ✅ No security concerns. No user input handling changes. Verdict: LGTM — solid fix for the mobile overflow issue. The portal pattern is the right approach. |
|
Hey @kushbosamiya — this PR has had changes requested for about 7 weeks (bordalix's review on Feb 2). Still planning to address the feedback? Happy to help if you're stuck on anything. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review — fix/limits-announcement-modal
Blocker: PR cannot be merged (DIRTY state — stale base)
mergeStateStatus: DIRTY. This PR branches from a stale point in master. The local master clone already contains a completely different Modal.tsx (AnimatePresence/Framer-Motion-based, position: fixed throughout, no old inline-only approach) and an already-fixed Announcement.tsx (lines 118–143 already implement maxHeight: '85vh', overflowY: 'auto', and a flex layout that pins the CTA buttons). Merging this PR as-is would discard that entire refactor. Rebase against master first, then re-evaluate what, if anything, remains to be done.
Finding 1 — CTA buttons not actually pinned (stated goal unmet)
src/components/Modal.tsx (new innerStyle, line ~35–42 in the diff):
const innerStyle = {
...
maxHeight: 'calc(100vh - 4rem)',
display: 'flex',
flexDirection: 'column',
overflowY: 'auto', // ← scrolls everything, including buttons
...
}overflowY: 'auto' is set on the container that holds all children, including the CTA buttons. When content genuinely overflows, the buttons scroll out of view. The PR description says "Keep CTA buttons always visible" but the implementation does not achieve this for long content. The correct pattern (which master's Announcement.tsx:118–143 already uses) is to separate the scrollable content region (flex: '1 1 auto'; minHeight: 0; overflowY: 'auto') from the CTA row (flex-shrink: 0). The fix needs to live in the caller layout, not the Modal wrapper itself — putting scroll on the wrapper forces every caller to compensate.
Finding 2 — Redundant zIndex: 95 on innerStyle
src/components/Modal.tsx (innerStyle):
zIndex: 95,The containerStyle div (position: fixed; zIndex: 90) already establishes its own stacking context. The inner zIndex: 95 only competes with siblings within that container, of which there are none. It has no effect on the overlay's z-ordering. Remove it to keep the z-index ladder legible.
Finding 3 — PrettyIcon bleed fix is correct
src/components/Announcement.tsx — PrettyIcon width: 'calc(100% + 2rem)' + margin: '-1rem -1rem 0 -1rem' correctly compensates for the modal's 1rem padding on each side, bleeding the gradient background to the modal edge. This is the right approach.
Finding 4 — createPortal is the right call
Portalling to document.body correctly escapes the Ionic stacking context that was causing the blur overlay to sit on top of the modal content. No objection here, though the jsdom note in receive-qrcode.test.tsx:327 suggests the test harness already has a workaround pattern — check that createPortal doesn't break existing modal tests (bridge-confirmation.test.tsx).
Finding 5 — Trailing whitespace
src/components/Modal.tsx, containerStyle block: zIndex: 90, and pointerEvents: 'none' as 'none', have trailing spaces. Nit, but clean it up.
Finding 6 — No new tests
No tests are added for the height-constrained scroll behaviour or the createPortal rendering path. After rebase, add a test asserting the modal renders in document.body and that buttons remain in the viewport when content overflows.
Summary
| Merge state | ❌ DIRTY — rebase required |
| CTA pinning | ❌ Incomplete — buttons still scroll with content |
createPortal |
✅ Correct approach |
PrettyIcon bleed |
✅ Correct |
FlexCol style escape-hatch |
✅ Low risk |
Redundant inner zIndex |
|
| Test coverage | ❌ None added |
Rebase against master; after the rebase much of the Modal.tsx work may already be present. Then fix the CTA pinning by keeping overflowY out of the modal wrapper and instead structuring it inside Announcement (or wherever CTA buttons live), following the pattern already in master.
5 participants
Fixes #307
Fixes an issue where the blurred section overlaps the component screen on mobile when users zoom in, hiding the CTA buttons.
What’s changed :
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.