feat: make site responsive on mobile#346
feat: make site responsive on mobile#346n1c01a5 wants to merge 8 commits intoProof-Of-Humanity:testnetsfrom
Conversation
Remove MobileBlocker that was blocking all mobile users. Fix MobileMenu to use the actual chain instead of hardcoded Ethereum. Stack filter bar vertically on small screens, make dropdown buttons full-width on mobile, fix footer gap overflow, and improve AirdropBanner close button touch target. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
👷 Deploy request for proof-of-humanity-v2 pending review.Visit the deploys page to approve it
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors multiple UI components for improved responsive behavior, removes the MobileBlocker component and its usage, passes a Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/Header/index.tsx (1)
33-33:⚠️ Potential issue | 🟠 Major
chain!non-null assertion will cause runtime crashes on unsupported chains.
chains.find(...)at line 33 returnsChain | undefined. When a user connects to a chain not inconfig.chains,chainisundefinedat runtime, but the!assertion suppresses TypeScript checks. The three consumers all declarechainas a required non-optional prop and directly access.idand/or.name:
- DesktopNavigation (line 108): Accesses
chain.idat line 35 → crashes- MobileMenu (line 115): Passes
chainto WalletSection which accesses.idand.name→ crashes- WalletSection (line 121): Accesses
chain.idat line 28 andchain.nameat line 29 → crashesWhen the wallet section attempts to render on desktop or in the mobile menu, "Cannot read property 'id/name' of undefined" errors will be thrown. The safe fix is to provide a fallback chain:
- const chain = chains.find(chain => chain.id === chainId) + const chain = chains.find(c => c.id === chainId) ?? chains[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/Header/index.tsx` at line 33, The code uses chains.find(...) to set const chain which can be undefined and then relies on a non-null assertion; replace this by providing a safe fallback Chain before passing into consumers: compute a fallback (e.g., first entry of chains or a constructed default Chain) and assign it to chain when find returns undefined, then pass that guaranteed-non-undefined chain into DesktopNavigation, MobileMenu, and WalletSection so their accesses to chain.id and chain.name are safe; update the variable referenced by the existing const chain and ensure the fallback is typed as Chain to satisfy TypeScript.
🤖 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/components/AirdropBanner.tsx`:
- Line 40: The tappable control in the AirdropBanner component currently uses
className="... p-2 ..." which yields a 36px tap area; update that class to use
p-3 (12px padding) so the final touch target equals 44px to match the PR
description and the AAA target—locate the element in AirdropBanner.tsx (the
element with className containing right-4 top-1/2 -translate-y-1/2 p-2) and
replace p-2 with p-3.
---
Outside diff comments:
In `@src/app/Header/index.tsx`:
- Line 33: The code uses chains.find(...) to set const chain which can be
undefined and then relies on a non-null assertion; replace this by providing a
safe fallback Chain before passing into consumers: compute a fallback (e.g.,
first entry of chains or a constructed default Chain) and assign it to chain
when find returns undefined, then pass that guaranteed-non-undefined chain into
DesktopNavigation, MobileMenu, and WalletSection so their accesses to chain.id
and chain.name are safe; update the variable referenced by the existing const
chain and ensure the fallback is typed as Chain to satisfy TypeScript.
p-2 (8px) yields 36px total, p-3 (12px) yields 44px which meets WCAG 2.5.5 AAA target size. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🔥 |
|
@n1c01a5 Thanks a lot for taking time to work on mobile view, though we still have some issues suggested by @nachikeighth
|
- AirdropBanner: add pr-10/pr-12 padding to prevent close button overlap - Rewards page: add mt-6 top margin below header - Video step: reduce wallet address margins on mobile, add break-all/font-mono - Profile details: center claimer address, chain name, POH ID on mobile - CirclesStepCard: add mobile margins, responsive text sizing, leading-relaxed - Request page: fix w-[84vw] to w-[92vw] on small screens, remove flex/grid conflict on policy link, fix ml-2 to ml-0 on mobile - ActionBar: responsive padding/gap, center text and buttons on mobile, flex-wrap justify-center for vouch/fund buttons - Webcam: add touchAction manipulation, z-10 on record button, min-h container, remove click preventDefault blocking touch events, add aspect-video - Video recording: replace hardcoded mimeType with isTypeSupported fallback chain, add video track readiness check, use mediaRecorder.mimeType for blob Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/app/[pohid]/claim/Video.tsx (2)
72-86: HoistgetSupportedMimeTypeto module scope.Defining it inside
startRecordingre-creates the closure (and itstypesarray) on every record press. Moving it outside has no behavioural change but avoids unnecessary allocation and makes the probe reusable.♻️ Proposed refactor
+const getSupportedMimeType = (): string | undefined => { + const types = [ + 'video/webm;codecs="vp9,opus"', + 'video/webm;codecs="vp8,opus"', + 'video/webm;codecs="vp8"', + 'video/webm', + 'video/mp4;codecs="h264,aac"', + 'video/mp4;codecs="h264"', + 'video/mp4', + ]; + for (const type of types) { + if (MediaRecorder.isTypeSupported(type)) return type; + } + return undefined; +}; + function VideoStep({ advance, video$, isRenewal, videoError }: PhotoProps) { // ... const startRecording = () => { // ... - const getSupportedMimeType = () => { - // ... - }; const mimeType = getSupportedMimeType();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[pohid]/claim/Video.tsx around lines 72 - 86, The helper getSupportedMimeType is currently declared inside startRecording causing it and its types array to be recreated on every record press; hoist getSupportedMimeType to module scope (file-level, above the component) and keep its name and behavior identical, then update startRecording to call the module-scoped getSupportedMimeType; ensure no other closures or references rely on local variables so the probe remains reusable and allocations are avoided.
91-98: Guard against emptydatainondataavailableto avoid an opaque sanitizer error.When recording is very short or aborted (e.g., immediate stop),
data.sizecan be0. Passing an emptyArrayBuffertovideoSanitizereither returns garbage or throws, surfacing a confusing "Unexpected error" toast.🛡️ Proposed fix
mediaRecorder.ondataavailable = async ({ data }) => { + if (!data || data.size === 0) { + toast.error("No video data was captured. Please try recording again."); + return; + } loading.start("Processing video");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[pohid]/claim/Video.tsx around lines 91 - 98, The ondataavailable handler (mediaRecorder.ondataavailable) must guard against empty recordings by checking the incoming data (the event's data Blob) before constructing the Blob or passing it to videoSanitizer: if data is falsy or data.size === 0, stop the "Processing video" loading state and return early (or handle gracefully) to avoid creating an empty Blob and calling videoSanitizer with an empty ArrayBuffer which triggers the sanitizer error; implement this check before the newlyRecorded/blob creation and before any call to videoSanitizer.src/components/Webcam.tsx (1)
113-113:aspect-videois a no-op here;bg-red-500/50may be a debug artifact.Two separate concerns on this line:
aspect-videois redundant: Per the CSS spec,aspect-ratiois ignored when bothwidthandheightare explicitly set to non-autovalues. Sinceh-full w-fullsets both dimensions,aspect-videohas no effect. It can be removed for clarity.
bg-red-500/50: A semi-transparent red background is visible while the camera feed hasn't started (initial load, permission prompt, slow device). If this is intentional (e.g., a "camera inactive" indicator) it should be documented; otherwise it should be replaced with a neutral colour or removed.♻️ Suggested cleanup
- className="h-full w-full aspect-video object-cover bg-red-500/50" + className="h-full w-full object-cover bg-black/50"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Webcam.tsx` at line 113, In the Webcam component (src/components/Webcam.tsx) the img/video element using className="h-full w-full aspect-video object-cover bg-red-500/50" should be cleaned up: remove the redundant "aspect-video" since h-full and w-full override aspect-ratio, and either remove or replace "bg-red-500/50" (the semi-transparent red debug background) with a neutral placeholder or no background unless the red is an intentional "camera inactive" indicator—if intentional, add a brief comment in Webcam explaining that behaviour; otherwise update the className to something like "h-full w-full object-cover" (or a neutral bg class) to eliminate the debug artifact.
🤖 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/app/`[pohid]/claim/Video.tsx:
- Line 89: The MediaRecorder construction can throw and must be wrapped in
try-catch: in Video.tsx around the line creating mediaRecorder (new
MediaRecorder(camera.stream, mimeType ? { mimeType } : undefined)), wrap that
instantiation in a try block and on catch call setRecording(false), log the
error (console.error) and surface a user-visible error (use the existing
toast/error UI used elsewhere), then return early so the UI state stays
consistent; ensure you still clean up any partial state (e.g., do not leave any
dangling event listeners) and keep references to mediaRecorder only when
construction succeeds.
---
Nitpick comments:
In `@src/app/`[pohid]/claim/Video.tsx:
- Around line 72-86: The helper getSupportedMimeType is currently declared
inside startRecording causing it and its types array to be recreated on every
record press; hoist getSupportedMimeType to module scope (file-level, above the
component) and keep its name and behavior identical, then update startRecording
to call the module-scoped getSupportedMimeType; ensure no other closures or
references rely on local variables so the probe remains reusable and allocations
are avoided.
- Around line 91-98: The ondataavailable handler (mediaRecorder.ondataavailable)
must guard against empty recordings by checking the incoming data (the event's
data Blob) before constructing the Blob or passing it to videoSanitizer: if data
is falsy or data.size === 0, stop the "Processing video" loading state and
return early (or handle gracefully) to avoid creating an empty Blob and calling
videoSanitizer with an empty ArrayBuffer which triggers the sanitizer error;
implement this check before the newlyRecorded/blob creation and before any call
to videoSanitizer.
In `@src/components/Webcam.tsx`:
- Line 113: In the Webcam component (src/components/Webcam.tsx) the img/video
element using className="h-full w-full aspect-video object-cover bg-red-500/50"
should be cleaned up: remove the redundant "aspect-video" since h-full and
w-full override aspect-ratio, and either remove or replace "bg-red-500/50" (the
semi-transparent red debug background) with a neutral placeholder or no
background unless the red is an intentional "camera inactive" indicator—if
intentional, add a brief comment in Webcam explaining that behaviour; otherwise
update the className to something like "h-full w-full object-cover" (or a
neutral bg class) to eliminate the debug artifact.
| }; | ||
|
|
||
| const mimeType = getSupportedMimeType(); | ||
| const mediaRecorder = new MediaRecorder(camera.stream, mimeType ? { mimeType } : undefined); |
There was a problem hiding this comment.
new MediaRecorder(...) must be wrapped in a try-catch — unguarded construction can cause silent record failures.
MediaRecorder.isTypeSupported() is advisory; the constructor can still throw NotSupportedError (browser codec negotiation mismatch) or SecurityError (stream locked). When it throws here, setRecording(true) is never called, no toast is shown, and the button appears to do nothing — matching the "video record button does not work" regression described in the PR comments.
🛡️ Proposed fix
- const mediaRecorder = new MediaRecorder(camera.stream, mimeType ? { mimeType } : undefined);
+ let mediaRecorder: MediaRecorder;
+ try {
+ mediaRecorder = new MediaRecorder(camera.stream, mimeType ? { mimeType } : undefined);
+ } catch (err: any) {
+ toast.error(err?.message || "Failed to initialize recorder. Please try again.");
+ return;
+ }📝 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.
| const mediaRecorder = new MediaRecorder(camera.stream, mimeType ? { mimeType } : undefined); | |
| let mediaRecorder: MediaRecorder; | |
| try { | |
| mediaRecorder = new MediaRecorder(camera.stream, mimeType ? { mimeType } : undefined); | |
| } catch (err: any) { | |
| toast.error(err?.message || "Failed to initialize recorder. Please try again."); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[pohid]/claim/Video.tsx at line 89, The MediaRecorder construction
can throw and must be wrapped in try-catch: in Video.tsx around the line
creating mediaRecorder (new MediaRecorder(camera.stream, mimeType ? { mimeType }
: undefined)), wrap that instantiation in a try block and on catch call
setRecording(false), log the error (console.error) and surface a user-visible
error (use the existing toast/error UI used elsewhere), then return early so the
UI state stays consistent; ensure you still clean up any partial state (e.g., do
not leave any dangling event listeners) and keep references to mediaRecorder
only when construction succeeds.
The content-wide class had md:mt-12 but no mobile margin, causing Rewards page (and any content-wide page) to sit flush against the header on mobile. Matches the content class which already has mt-8. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add border-b border-white/20 to visually separate banner from header - Pin X button to top-right corner (top-2 right-2) so it stays fixed regardless of text wrapping height Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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/app/globals.css`:
- Around line 64-66: The margin breakpoint for .content-wide is inconsistent
with .content: update the .content-wide class (which currently uses md:mt-12) to
use sm:mt-12 so its top margin scales at the same 640px breakpoint as .content
(ensure you change the md:mt-12 token to sm:mt-12 in the .content-wide rule).
- ActionButton: w-full on mobile, auto-width on md+ for better touch targets - ActionBar: stack buttons vertically on mobile with flex-col, row on md+ - content-wide: use sm:mt-12 instead of md:mt-12 to match content class pattern Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- MobileMenu: full-width dropdown with centered nav items - btn-main/btn-sec: w-full on mobile, w-auto on md+ globally - AirdropBanner: close button in flex flow (no overlap), stronger separator - Grid: Load More button full-width on mobile, centered on md+ - ActionBar: clean up ExternalLink className formatting Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@madhurMongia thanks for the review on #346! We've (with Claude) pushed fixes for the main issues you flagged. Just a heads up, the priority right is shipping a functional mobile version. Some of the finer details can wait. Can always polish later (as the desktop one ;)) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/app/Header/MobileMenu.tsx (1)
22-77: Optional: setdisplayNameon theforwardRefcomponent.
forwardRefwrappers lose their name in React DevTools unlessdisplayNameis explicitly set.♻️ Proposed addition
); +MobileMenu.displayName = "MobileMenu"; + export default MobileMenu;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/Header/MobileMenu.tsx` around lines 22 - 77, The forwardRef-wrapped MobileMenu component should have an explicit displayName to preserve its name in React DevTools; after the forwardRef declaration for MobileMenu, set MobileMenu.displayName = "MobileMenu" (referencing the MobileMenu symbol and the forwardRef wrapper) so the component shows a readable name in debugging tools.src/components/AirdropBanner.tsx (2)
13-13: Orphanedrelativeclass on outerdiv.The close button is now inline in the flex row (no longer
absolute-positioned), so therelativecontext anchor on the outerdivhas no effect. Safe to remove.♻️ Proposed cleanup
- <div className="gradient w-full relative border-b-2 border-white/30"> + <div className="gradient w-full border-b-2 border-white/30">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/AirdropBanner.tsx` at line 13, The outer div in the AirdropBanner component contains an orphaned "relative" Tailwind class that is no longer needed because the close button is inline (not absolutely positioned); remove "relative" from the className on the outer div in AirdropBanner (the div with className "gradient w-full relative border-b-2 border-white/30") and verify no other code in AirdropBanner depends on that positioning.
36-43: Addtype="button"to the close button.Without an explicit type, the button defaults to
type="submit"inside any ancestor<form>, whiche.preventDefault()only partially guards against (JS-only). Declaringtype="button"makes the intent unambiguous at the HTML level.♻️ Proposed fix
<button + type="button" onClick={(e) => { e.preventDefault(); setIsVisible(false); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/AirdropBanner.tsx` around lines 36 - 43, In AirdropBanner's close button (the JSX element using onClick that calls setIsVisible(false)), add an explicit type="button" attribute to the <button> element so it won't act as a submit button when rendered inside a form; locate the button with the onClick handler in the AirdropBanner component and add the type attribute to make the intent explicit at the HTML level.
🤖 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/app/Header/MobileMenu.tsx`:
- Line 17: MobileMenuProps currently requires chain but the parent passes chain
with a non-null assertion (chain!) which can still be undefined at runtime;
update the contract and guards so runtime crashes are prevented: either make the
prop optional (change MobileMenuProps to chain?: { id: number; name: string })
and add defensive checks in MobileMenu and WalletSection before accessing
chain.id or chain.name (render a loading/fallback or disable wallet actions when
chain is undefined), or keep the prop required but change the parent (where
chains.find(...) is used and chain! is applied) to only render
MobileMenu/WalletSection when chain is truthy; reference MobileMenuProps,
MobileMenu, and WalletSection when making the change and ensure every access to
chain is guarded or behind a conditional render.
---
Duplicate comments:
In `@src/components/AirdropBanner.tsx`:
- Around line 41-56: The close button's touch target was reduced because the SVG
was changed to width/height="16" while the button kept className="... p-2 ...",
resulting in a 32px tap area; update the close control in AirdropBanner (the
button element with className containing "p-2" and the nested svg with
width/height attributes) so the tap target meets the 44px guideline by either
changing the padding to p-3 (className: replace p-2 with p-3) or restoring the
svg to 20×20 (set svg width="20" height="20"), or both if you prefer 44px
exactly; ensure you only modify the button's className and/or the svg
width/height in the AirdropBanner close button block.
---
Nitpick comments:
In `@src/app/Header/MobileMenu.tsx`:
- Around line 22-77: The forwardRef-wrapped MobileMenu component should have an
explicit displayName to preserve its name in React DevTools; after the
forwardRef declaration for MobileMenu, set MobileMenu.displayName = "MobileMenu"
(referencing the MobileMenu symbol and the forwardRef wrapper) so the component
shows a readable name in debugging tools.
In `@src/components/AirdropBanner.tsx`:
- Line 13: The outer div in the AirdropBanner component contains an orphaned
"relative" Tailwind class that is no longer needed because the close button is
inline (not absolutely positioned); remove "relative" from the className on the
outer div in AirdropBanner (the div with className "gradient w-full relative
border-b-2 border-white/30") and verify no other code in AirdropBanner depends
on that positioning.
- Around line 36-43: In AirdropBanner's close button (the JSX element using
onClick that calls setIsVisible(false)), add an explicit type="button" attribute
to the <button> element so it won't act as a submit button when rendered inside
a form; locate the button with the onClick handler in the AirdropBanner
component and add the type attribute to make the intent explicit at the HTML
level.
| address?: `0x${string}`; | ||
| web3Loaded: boolean; | ||
| isConnected: boolean; | ||
| chain: { id: number; name: string }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how `chain` is defined and passed in the parent Header component,
# and how WalletSection uses the chain prop.
echo "=== Header/index.tsx: how chain is sourced and passed to MobileMenu ==="
fd "index.tsx" src/app/Header --exec cat -n {} \;
echo ""
echo "=== WalletSection: how it consumes the chain prop ==="
fd "WalletSection.tsx" --exec cat -n {} \;Repository: Proof-Of-Humanity/proof-of-humanity-v2-web
Length of output: 6922
chain required but passed via chain! in parent — potential runtime crash if undefined.
MobileMenuProps declares chain as required, yet the parent (src/app/Header/index.tsx) derives it as const chain = chains.find(...) on line 33, which returns undefined if no matching chain exists. The parent then uses chain! (line 115) to suppress TypeScript errors. If the mobile menu opens before chain resolves (or if the chain lookup fails), WalletSection will receive undefined and crash when accessing chain.id or chain.name on lines 28–29.
Fix by either:
- Making
chainoptional (chain?: { id: number; name: string }) and guarding access inWalletSection/MobileMenu, or - Ensuring the parent only renders
MobileMenuandWalletSectiononcechainis guaranteed defined (not just with!).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/Header/MobileMenu.tsx` at line 17, MobileMenuProps currently requires
chain but the parent passes chain with a non-null assertion (chain!) which can
still be undefined at runtime; update the contract and guards so runtime crashes
are prevented: either make the prop optional (change MobileMenuProps to chain?:
{ id: number; name: string }) and add defensive checks in MobileMenu and
WalletSection before accessing chain.id or chain.name (render a loading/fallback
or disable wallet actions when chain is undefined), or keep the prop required
but change the parent (where chains.find(...) is used and chain! is applied) to
only render MobileMenu/WalletSection when chain is truthy; reference
MobileMenuProps, MobileMenu, and WalletSection when making the change and ensure
every access to chain is guarded or behind a conditional render.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>








Summary
MobileBlockercomponent that was blocking all mobile users (added as temporary measure in PR feat: enhance cross-chain functionality and UI updates #318)MobileMenuto pass the actual connected chain instead of hardcoded EthereumTest plan
Summary by CodeRabbit
Refactor
Style