Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds an Advertising page and route; updates header navigation (Superpowers, About, Cooperation submenu); updates footer social assets (X icon source changed, Discord added); introduces a MailChimp signup component with Plausible tracking; and restructures the Landing page replacing the login anchor with a router Link and refreshed content blocks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Browser
participant Landing as Landing Page
participant MailChimp as MailChimp Component
participant MailHook as useMailChimpForm
participant Plausible as Plausible Tracker
Browser->>Landing: GET /
Landing->>MailChimp: render signup UI
User->>MailChimp: submit(email)
MailChimp->>MailHook: submit(EMAIL) (async)
MailHook-->>MailChimp: success / error
MailChimp->>Plausible: track("Email Signup")
alt success
MailChimp-->>User: show success
else error
MailChimp-->>User: show error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 3
🧹 Nitpick comments (3)
src/components/LandingPage/MailChimp/index.jsx (2)
1-1: Upgrade to the maintained Plausible package.The
plausible-trackerpackage (v0.3.9) has been archived since August 2025. The maintainers have moved to@plausible-analytics/tracker, which receives continued updates and security patches.Based on learnings.
Apply this diff to upgrade to the maintained package:
-import Plausible from 'plausible-tracker'; +import Plausible from '@plausible-analytics/tracker';And update package.json:
-"plausible-tracker": "^0.3.9" +"@plausible-analytics/tracker": "latest"
29-35: Add client-side email validation.Submitting without basic email validation can lead to a poor user experience when invalid emails are rejected by Mailchimp's server.
Add validation before submission:
<form onSubmit={(event) => { event.preventDefault(); + // Basic email validation + if (!fields.EMAIL || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(fields.EMAIL)) { + return; // Could also set an error message here + } handleSubmit(fields); trackEvent('Email Signup'); }} > <input id="EMAIL" type="email" placeholder="Enter your email" value={fields.EMAIL} onChange={handleFieldChange} + required />src/pages/Advertising.jsx (1)
1-1: Remove unnecessary ESLint disable comment.The file disables the
react/jsx-props-no-spreadingrule, but no props spreading is used anywhere in the component.Apply this diff to remove the unnecessary comment:
-/* eslint-disable react/jsx-props-no-spreading */ import { GlobalStyles } from '../components/LandingPage/GlobalStyles';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
public/landing-images/advertising-hero-bg.svgis excluded by!**/*.svgpublic/landing-images/advertising-icon-1.svgis excluded by!**/*.svgpublic/landing-images/advertising-icon-2.svgis excluded by!**/*.svgpublic/landing-images/advertising-icon-3.svgis excluded by!**/*.svgpublic/landing-images/app-store.svgis excluded by!**/*.svgpublic/landing-images/arrow-down.pngis excluded by!**/*.pngpublic/landing-images/google-play.svgis excluded by!**/*.svgpublic/landing-images/home-bg.svgis excluded by!**/*.svgpublic/landing-images/home-hero-bg.svgis excluded by!**/*.svgpublic/landing-images/home-superpowers-1.mp4is excluded by!**/*.mp4public/landing-images/home-superpowers-2.mp4is excluded by!**/*.mp4public/landing-images/home-superpowers-3.mp4is excluded by!**/*.mp4public/landing-images/home-superpowers-icon-1.svgis excluded by!**/*.svgpublic/landing-images/home-superpowers-icon-2.svgis excluded by!**/*.svgpublic/landing-images/social-discord.svgis excluded by!**/*.svgpublic/landing-images/social-x.svgis excluded by!**/*.svgpublic/landing-images/web.svgis excluded by!**/*.svg
📒 Files selected for processing (7)
src/components/LandingPage/Footer/index.jsx(2 hunks)src/components/LandingPage/Header/index.jsx(1 hunks)src/components/LandingPage/MailChimp/index.jsx(1 hunks)src/containers/Main.tsx(4 hunks)src/navigation/index.tsx(4 hunks)src/pages/Advertising.jsx(1 hunks)src/pages/Landing.jsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/navigation/index.tsx (1)
src/pages/Advertising.jsx (1)
Advertising(8-143)
src/containers/Main.tsx (1)
src/pages/Advertising.jsx (1)
Advertising(8-143)
src/pages/Landing.jsx (1)
src/components/LandingPage/MailChimp/index.jsx (1)
MailChimp(4-50)
src/pages/Advertising.jsx (3)
src/components/LandingPage/GlobalStyles/index.jsx (1)
GlobalStyles(11-3018)src/components/LandingPage/Header/index.jsx (1)
Header(7-120)src/components/LandingPage/Footer/index.jsx (1)
Footer(4-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint
- GitHub Check: unit-tests
🔇 Additional comments (9)
src/containers/Main.tsx (1)
37-37: LGTM! Advertising route properly integrated.The Advertising page is correctly wired into both authenticated and unauthenticated routing flows, and the authentication checks are properly updated to treat
/advertisingas a root page.Also applies to: 671-674, 745-745, 789-792
src/navigation/index.tsx (1)
13-13: LGTM! Route definitions are consistent.The Advertising route is properly defined in the navigationRoute object and correctly added to both authorized and unauthorized navigation components.
Also applies to: 27-27, 52-55, 80-80
src/components/LandingPage/Footer/index.jsx (1)
16-16: LGTM! Social links updated correctly.The migration of the X icon to a local asset path and the addition of the Discord social link both follow the existing patterns and align with the broader asset localization effort in this PR.
Also applies to: 28-37
src/components/LandingPage/Header/index.jsx (2)
75-93: Consider text consistency: "For dApp" vs "For dApps".The navigation item text changed from "For dApps" (plural) to "For dApp" (singular) in line 82. While this may be intentional, verify that the singular form aligns with the intended messaging and content on the
/developerspage.
58-71: Navigation restructure looks good.The new navigation structure with anchor links for "Superpowers" and "About", plus the "Cooperation" dropdown menu with nested items, follows existing patterns and properly integrates the new Advertising page.
Also applies to: 75-93
src/pages/Advertising.jsx (1)
8-142: Page structure follows established patterns.The Advertising page correctly uses the GlobalStyles, Header, and Footer components, and the section structure aligns with other landing pages in the codebase.
src/pages/Landing.jsx (3)
3-3: Good use of react-router Link for internal navigation.Converting the login anchor tag to a
Linkcomponent (lines 29-34) and using Link for the web download CTA (line 243) correctly implements client-side routing for these internal navigation actions, avoiding unnecessary page reloads.Also applies to: 29-34, 243-245
8-8: MailChimp component integration looks good.The replacement of the inline form with the MailChimp component properly modularizes the email signup functionality and enables analytics tracking.
Also applies to: 209-209
214-218: Add alt attribute to PillarX logo.The logo image is missing an
altattribute, which is required for accessibility and screen reader support.Apply this diff:
<img src="/landing-images/pillarXLogo.png" alt="pillar-x-logo" + alt="PillarX logo" />Note: Remove the duplicate
altattribute on line 216 (appears to be a typo in the code).Likely an incorrect or invalid review comment.
| <input | ||
| id="EMAIL" | ||
| type="email" | ||
| placeholder="Enter your email" | ||
| value={fields.EMAIL} | ||
| onChange={handleFieldChange} | ||
| /> |
There was a problem hiding this comment.
Add accessible label for email input.
The email input lacks an associated label, which creates an accessibility barrier for screen reader users. While a placeholder is present, it's not a substitute for a proper label.
Apply this diff to add a visually-hidden label:
<form
onSubmit={(event) => {
event.preventDefault();
handleSubmit(fields);
trackEvent('Email Signup');
}}
>
+ <label htmlFor="EMAIL" className="sr-only">
+ Email address
+ </label>
<input
id="EMAIL"
type="email"
placeholder="Enter your email"
value={fields.EMAIL}
onChange={handleFieldChange}
/>You'll need to add a .sr-only utility class to your CSS:
.sr-only {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip: rect(0, 0, 0, 0);
white-space: nowrap;
border-width: 0;
}🤖 Prompt for AI Agents
In src/components/LandingPage/MailChimp/index.jsx around lines 29 to 35, add an
associated label for the email input by inserting a <label> with htmlFor="EMAIL"
and className="sr-only" immediately before the <input> so the input's id matches
the label; then add the provided .sr-only CSS to the component/global stylesheet
to visually hide the label while keeping it accessible to screen readers; keep
the existing placeholder and input props unchanged.
github-actions
bot
temporarily deployed
to
Preview (feat/new-landing-page)
Deploying x with
|
| Latest commit: |
131dd3c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://df1b1bad.x-e62.pages.dev |
| Branch Preview URL: | https://feat-new-landing-page.x-e62.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/Advertising.jsx (1)
165-188: Add alt text to Gas Tank/Web3 App Store icons.These icons convey meaning; without
alt, screen readers announce raw URLs or skip them entirely. Please supply concise descriptions.<img src="https://cdn.pillarx.app/home-superpowers-icon-1.svg" /> + <img + src="https://cdn.pillarx.app/home-superpowers-icon-1.svg" + alt="Universal gas tank icon" + /> ... <img src="https://cdn.pillarx.app/home-superpowers-icon-2.svg" /> + <img + src="https://cdn.pillarx.app/home-superpowers-icon-2.svg" + alt="Web3 App Store icon" + />
♻️ Duplicate comments (1)
src/pages/Advertising.jsx (1)
93-123: Restore descriptive alt text for feature icons.These icons label distinct offerings, so empty
altattributes hide that context from assistive tech. Let’s reinstate meaningful descriptions as previously requested.<img src="https://cdn.pillarx.app/advertising-icon-1.svg" - alt="" + alt="Token competitions icon" /> ... <img src="https://cdn.pillarx.app/advertising-icon-2.svg" - alt="" + alt="Newsletter icon" /> ... <img src="https://cdn.pillarx.app/advertising-icon-3.svg" - alt="" + alt="Web3 App Store icon" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/LandingPage/Footer/index.jsx(2 hunks)src/components/LandingPage/MailChimp/index.jsx(1 hunks)src/pages/Advertising.jsx(1 hunks)src/pages/Landing.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/LandingPage/MailChimp/index.jsx
- src/components/LandingPage/Footer/index.jsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/Landing.jsx (1)
src/components/LandingPage/MailChimp/index.jsx (1)
MailChimp(4-56)
src/pages/Advertising.jsx (3)
src/components/LandingPage/GlobalStyles/index.jsx (1)
GlobalStyles(11-3022)src/components/LandingPage/Header/index.jsx (1)
Header(7-120)src/components/LandingPage/Footer/index.jsx (1)
Footer(4-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
| <div className="home_hero__image"> | ||
| <img src="https://cdn.pillarx.app/home-hero.webp" /> | ||
| </div> |
There was a problem hiding this comment.
Add alt text to the hero image.
Without alt, screen readers announce a generic filename. Provide a short description of the hero artwork.
- <div className="home_hero__image">
- <img src="https://cdn.pillarx.app/home-hero.webp" />
+ <div className="home_hero__image">
+ <img
+ src="https://cdn.pillarx.app/home-hero.webp"
+ alt="PillarX mobile app screens"
+ />📝 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.
| <div className="home_hero__image"> | |
| <img src="https://cdn.pillarx.app/home-hero.webp" /> | |
| </div> | |
| <div className="home_hero__image"> | |
| <img | |
| src="https://cdn.pillarx.app/home-hero.webp" | |
| alt="PillarX mobile app screens" | |
| /> | |
| </div> |
🤖 Prompt for AI Agents
In src/pages/Landing.jsx around lines 36 to 38, the hero img lacks an alt
attribute; add a concise alt describing the hero artwork (for example
"Illustration of product benefits" or similar appropriate short description) to
the img element so screen readers get meaningful context.
| <div className="home_feature__detail__content__icon__wrapper"> | ||
| <img src="https://cdn.pillarx.app/home-superpowers-icon-1.svg" /> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div className="home_feature__detail__content home_feature__detail__content--column gradient_border"> | ||
| <h2> | ||
| Web3 App Store | ||
| <br /> | ||
| <span> | ||
| Access dApps Without | ||
| <br /> Leaving PillarX | ||
| </span> | ||
| </h2> | ||
| <p> | ||
| All your favorite dApps are built directly into PillarX, | ||
| meaning you don’t have to leave the platform to connect to any | ||
| of them! Safe, secure, and under your control. | ||
| </p> | ||
| <div className="home_feature__detail__content__icon"> | ||
| <h4>Coming Soon</h4> | ||
| <div className="home_feature__detail__content__icon__wrapper"> | ||
| <img src="https://cdn.pillarx.app/home-superpowers-icon-2.svg" /> | ||
| </div> |
There was a problem hiding this comment.
Provide alt text for feature icons.
These icons reinforce the copy; missing alt leaves assistive tech users without that context.
- <img src="https://cdn.pillarx.app/home-superpowers-icon-1.svg" />
+ <img
+ src="https://cdn.pillarx.app/home-superpowers-icon-1.svg"
+ alt="Universal gas tank icon"
+ />
...
- <img src="https://cdn.pillarx.app/home-superpowers-icon-2.svg" />
+ <img
+ src="https://cdn.pillarx.app/home-superpowers-icon-2.svg"
+ alt="Web3 App Store icon"
+ />🤖 Prompt for AI Agents
In src/pages/Landing.jsx around lines 165 to 188 the <img> tags for the feature
icons are missing alt attributes; add meaningful alt text that reflects each
icon's purpose (e.g., "Feature: Superpowers icon" and "Feature: Web3 App Store
icon" or similar descriptive phrases matching the surrounding copy) to improve
accessibility, ensuring you don't leave alt empty unless the image is purely
decorative.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/pages/Landing.jsx (2)
36-38: Add alt text to the hero image.The hero image still has an empty
alt=""attribute. This was flagged in a previous review. Without meaningful alt text, screen readers cannot convey the image's purpose to visually impaired users.Apply this diff:
- <div className="home_hero__image"> - <img src="https://cdn.pillarx.app/home-hero.webp" alt="" /> + <div className="home_hero__image"> + <img + src="https://cdn.pillarx.app/home-hero.webp" + alt="PillarX mobile app screens showcasing token insights and trading features" + />
166-169: Add alt text to feature icons.The feature icons on lines 168 and 192 have empty
alt=""attributes. This was flagged in a previous review. These icons reinforce the copy and help users understand the features visually.Apply this diff:
<div className="home_feature__detail__content__icon__wrapper"> <img src="https://cdn.pillarx.app/home-superpowers-icon-1.svg" - alt="" + alt="Universal gas tank icon" /> </div> ... <div className="home_feature__detail__content__icon__wrapper"> <img src="https://cdn.pillarx.app/home-superpowers-icon-2.svg" - alt="" + alt="Web3 App Store icon" /> </div>Also applies to: 190-193
🧹 Nitpick comments (1)
src/pages/Landing.jsx (1)
263-284: Consider using separate paragraphs instead of<br />tags.Using
<br /><br />to separate paragraphs (line 277-278) is less semantic than using multiple<p>elements. Separate paragraphs improve accessibility and allow better styling control.Apply this diff to improve semantic structure:
<h2>The Story Behind PillarX</h2> - <p> PillarX began with{' '} <a href="https://pillar.fi/?utm_source=pillarx.app&utm_medium=website&utm_campaign=home_page" target="_blank" rel="noreferrer" > Pillar Wallet </a>{' '} in 2017, one of the first EVM-compatible wallets. The team quickly identified the usability challenges faced by EOAs, leading to the introduction of Smart Accounts. This evolved into a smart contract wallet, unlocking advanced features and setting the stage for innovative use cases that would define the future of Web3, even - before Layer 1s and Layer 2s became mainstream. <br /> - <br /> + before Layer 1s and Layer 2s became mainstream. + </p> + <p> But the team didn't stop there. The goal was to make the Web3 experience even better and more accessible. Enter PillarX, a complete upgrade to the Pillar Wallet, designed to empower users with a seamless, intuitive, and cutting-edge platform for all things Web3. </p>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/LandingPage/Header/index.jsx(2 hunks)src/components/LandingPage/MailChimp/index.jsx(1 hunks)src/pages/Landing.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/LandingPage/MailChimp/index.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/Landing.jsx (1)
src/components/LandingPage/MailChimp/index.jsx (1)
MailChimp(4-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (5)
src/pages/Landing.jsx (5)
1-1: LGTM!The addition of
Linkfromreact-router-domand theMailChimpcomponent import align with the PR's goal to add client-side routing and modularize the signup form.Also applies to: 8-8
29-34: LGTM!Replacing the anchor tag with
Link to="/login"provides proper client-side routing without a full page reload. The Plausible event tracking attribute is correctly preserved.
215-215: LGTM!The MailChimp component is properly integrated, promoting code reusability and separation of concerns.
220-251: Good accessibility practices.All images in the download section have proper
altattributes, and external links includerel="noreferrer"for security.
265-271: LGTM!The external link to Pillar Wallet includes proper security attributes (
target="_blank"andrel="noreferrer") and UTM parameters for tracking.
| /* eslint-disable react/self-closing-comp */ | ||
| /* eslint-disable jsx-a11y/control-has-associated-label */ | ||
| /* eslint-disable jsx-a11y/no-noninteractive-element-interactions */ | ||
| /* eslint-disable jsx-a11y/anchor-is-valid */ |
There was a problem hiding this comment.
Remove ESLint disable and use semantic HTML instead.
Disabling jsx-a11y/anchor-is-valid masks an accessibility issue rather than fixing it. Line 69 uses an <a> tag without an href attribute as a dropdown trigger, which is semantically incorrect and breaks keyboard navigation expectations.
Apply this diff to use a <button> element for the dropdown trigger:
-/* eslint-disable jsx-a11y/anchor-is-valid */And update the markup at line 69:
<li>
- <a>Cooperation</a>
+ <button type="button" className="header__menu-button">Cooperation</button>
<ul>Note: You may need to add CSS for .header__menu-button to style the button consistently with your other menu items (remove default button styles, match font/color, etc.).
📝 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.
| /* eslint-disable jsx-a11y/anchor-is-valid */ | |
| /* Remove the ESLint disable at the top of the file */ | |
| - /* eslint-disable jsx-a11y/anchor-is-valid */ | |
| /* Update the “Cooperation” menu item to use a button for better semantics */ | |
| <li> | |
| <button type="button" className="header__menu-button"> | |
| Cooperation | |
| </button> | |
| <ul> | |
| <li><Link to="/cooperation/for-dapp">For dApp</Link></li> | |
| <li><Link to="/cooperation/advertising">Advertising</Link></li> | |
| </ul> | |
| </li> |
| <NavLink to="/#superpowers">Superpowers</NavLink> | ||
| </li> | ||
| <li> | ||
| <NavLink to="/#about" className="active--no-style"> | ||
| About | ||
| </NavLink> | ||
| </li> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Verify hash navigation behavior with NavLink.
Using NavLink to="/#superpowers" and to="/#about" is unconventional. React Router v5's NavLink is designed for path-based routing, not hash fragments. This pattern may not trigger the activeClassName styling correctly, and the active--no-style class on line 61 suggests manual workarounds are needed.
Consider these alternatives:
- If these are same-page anchors: Use regular
<a href="#superpowers">tags instead ofNavLink, and handle smooth scrolling with JavaScript. - If these are distinct routes: Define proper routes in your router configuration (e.g.,
/superpowers,/about) and useNavLink to="/superpowers".
Run the following script to check if routes for these paths exist:
🏁 Script executed:
#!/bin/bash
# Search for route definitions that might handle /superpowers or /about
rg -n --type=js --type=jsx -C3 'Route.*path.*superpowers|Route.*path.*about'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for route definitions handling /superpowers or /about
rg -n -C3 -g '*.js' -g '*.jsx' 'Route.*path.*superpowers|Route.*path.*about'Length of output: 0
Replace hash-based NavLink with anchor tags or real routes
NavLink won’t match to="/#superpowers" or "/#about" (no /superpowers or /about routes exist), so these links won’t navigate or activate correctly.
• For same-page sections, use <a href="#superpowers">Superpowers</a> and <a href="#about">About</a> (add smooth-scroll logic as needed).
• For distinct pages, define corresponding <Route path="/superpowers"> and <Route path="/about"> in your router, then use NavLink to="/superpowers" and to="/about".
🤖 Prompt for AI Agents
In src/components/LandingPage/Header/index.jsx around lines 58 to 64, the
NavLink components use hash fragments (to="/#superpowers" and to="/#about")
which won’t match router paths and therefore won’t navigate or activate
correctly; replace those NavLink usages with plain anchor tags for same-page
sections (e.g., <a href="#superpowers"> and <a href="#about">) and add
smooth-scroll logic if desired, or alternatively create proper routes
(/superpowers and /about) in your router and change the NavLink to use those
route paths (to="/superpowers" and to="/about") so routing and active state work
as intended.
| <li> | ||
| <a>Cooperation</a> | ||
| <ul> | ||
| <li> | ||
| <NavLink to="/developers">For dApp</NavLink> | ||
| </li> | ||
| <li> | ||
| <NavLink to="/advertising">Advertising</NavLink> | ||
| </li> | ||
| </ul> | ||
| </li> |
There was a problem hiding this comment.
Add keyboard navigation and ARIA attributes to the dropdown menu.
The "Cooperation" dropdown lacks keyboard accessibility and screen reader support, preventing users from accessing the "For dApp" and "Advertising" links via keyboard navigation.
To fix this, implement the following:
- Add ARIA attributes to the parent menu item:
<li>
- <button type="button" className="header__menu-button">Cooperation</button>
+ <button
+ type="button"
+ className="header__menu-button"
+ aria-haspopup="true"
+ aria-expanded={isCooperationMenuOpen}
+ onClick={() => setIsCooperationMenuOpen(!isCooperationMenuOpen)}
+ onKeyDown={(e) => {
+ if (e.key === 'Escape') setIsCooperationMenuOpen(false);
+ }}
+ >
+ Cooperation
+ </button>
- <ul>
+ <ul role="menu" hidden={!isCooperationMenuOpen}>
<li>
- <NavLink to="/developers">For dApp</NavLink>
+ <NavLink to="/developers" role="menuitem">For dApp</NavLink>
</li>
<li>
- <NavLink to="/advertising">Advertising</NavLink>
+ <NavLink to="/advertising" role="menuitem">Advertising</NavLink>
</li>
</ul>
</li>- Add state management for the dropdown in your component:
const [isCooperationMenuOpen, setIsCooperationMenuOpen] = useState(false);- Handle click-outside to close the dropdown when users click elsewhere.
This ensures keyboard users can navigate with Enter/Space (to open) and Escape (to close), and screen readers announce the menu correctly.
🤖 Prompt for AI Agents
In src/components/LandingPage/Header/index.jsx around lines 68-78, the
"Cooperation" dropdown has no ARIA attributes or keyboard/state handling; add a
boolean state (e.g., isCooperationMenuOpen via useState) and a ref for the
parent menu, give the parent trigger an accessible element (button or anchor
with role="button") with aria-haspopup="menu", aria-expanded tied to state,
aria-controls pointing to the submenu id, and the submenu role="menu" while each
NavLink gets role="menuitem". Add key handlers on the trigger to toggle the menu
on Enter/Space and close on Escape, manage focus into the first menu item when
opened, and implement a click-outside listener via useEffect that closes the
menu when clicking outside the ref; ensure proper cleanup of listeners.
| href="https://www.apple.com/app-store/" | ||
| target="_blank" | ||
| rel="noreferrer" | ||
| className="plausible-event-name=Download+App+Store" | ||
| > | ||
| <img | ||
| src="https://cdn.pillarx.app/app-store.svg" | ||
| alt="app-store-logo" | ||
| /> | ||
| </a> | ||
| <a | ||
| href="https://play.google.com/store/apps/" | ||
| target="_blank" | ||
| rel="noreferrer" | ||
| className="plausible-event-name=Download+Google+Play" | ||
| > | ||
| <img | ||
| src="https://cdn.pillarx.app/google-play.svg" | ||
| alt="google-play-logo" | ||
| /> | ||
| </a> |
There was a problem hiding this comment.
Replace placeholder URLs with actual app links.
The App Store and Google Play links point to generic landing pages (apple.com/app-store/ and play.google.com/store/apps/) rather than the actual PillarX app listings. Users clicking these links won't reach the app.
Update the URLs to point to the actual PillarX app pages:
<a
- href="https://www.apple.com/app-store/"
+ href="https://apps.apple.com/app/pillarx/id[APP_ID]"
target="_blank"
rel="noreferrer"
className="plausible-event-name=Download+App+Store"
>
...
<a
- href="https://play.google.com/store/apps/"
+ href="https://play.google.com/store/apps/details?id=com.pillarx.app"
target="_blank"
rel="noreferrer"
className="plausible-event-name=Download+Google+Play"
>📝 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.
| href="https://www.apple.com/app-store/" | |
| target="_blank" | |
| rel="noreferrer" | |
| className="plausible-event-name=Download+App+Store" | |
| > | |
| <img | |
| src="https://cdn.pillarx.app/app-store.svg" | |
| alt="app-store-logo" | |
| /> | |
| </a> | |
| <a | |
| href="https://play.google.com/store/apps/" | |
| target="_blank" | |
| rel="noreferrer" | |
| className="plausible-event-name=Download+Google+Play" | |
| > | |
| <img | |
| src="https://cdn.pillarx.app/google-play.svg" | |
| alt="google-play-logo" | |
| /> | |
| </a> | |
| <a | |
| href="https://apps.apple.com/app/pillarx/id[APP_ID]" | |
| target="_blank" | |
| rel="noreferrer" | |
| className="plausible-event-name=Download+App+Store" | |
| > | |
| <img | |
| src="https://cdn.pillarx.app/app-store.svg" | |
| alt="app-store-logo" | |
| /> | |
| </a> | |
| <a | |
| href="https://play.google.com/store/apps/details?id=com.pillarx.app" | |
| target="_blank" | |
| rel="noreferrer" | |
| className="plausible-event-name=Download+Google+Play" | |
| > | |
| <img | |
| src="https://cdn.pillarx.app/google-play.svg" | |
| alt="google-play-logo" | |
| /> | |
| </a> |
🤖 Prompt for AI Agents
In src/pages/Landing.jsx around lines 228 to 248 the App Store and Google Play
anchor hrefs use generic placeholder URLs; update those href attributes to the
real PillarX app listing URLs (the official Apple App Store and Google Play
store links for PillarX) while preserving target="_blank", rel="noreferrer", and
the plausible-event-name attributes and existing img tags; ensure the new URLs
are the canonical store links (https URLs) and verify they navigate correctly to
the PillarX app pages.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/pages/Landing.jsx (3)
36-38: Hero image still missing meaningful alt text.The
<img>at line 37 has an emptyalt=""attribute, which provides no context for screen readers. This was flagged in previous reviews.Apply this diff to add descriptive alt text:
<div className="home_hero__image"> - <img src="https://cdn.pillarx.app/home-hero.webp" alt="" /> + <img + src="https://cdn.pillarx.app/home-hero.webp" + alt="PillarX mobile app interface showcasing Web3 trading features" + />
166-169: Feature icons still lack alt text.The icon images at lines 167 and 191 have empty
alt=""attributes. These icons reinforce the feature descriptions and should have meaningful alt text for screen reader users. This was flagged in previous reviews.Apply this diff to add descriptive alt text:
<div className="home_feature__detail__content__icon__wrapper"> <img src="https://cdn.pillarx.app/home-superpowers-icon-1.svg" - alt="" + alt="Universal gas tank icon" /> </div> ... <div className="home_feature__detail__content__icon__wrapper"> <img src="https://cdn.pillarx.app/home-superpowers-icon-2.svg" - alt="" + alt="Web3 App Store icon" /> </div>Also applies to: 190-193
228-248: App store links still use placeholder URLs.The App Store and Google Play links point to generic landing pages rather than the actual PillarX app listings. Users won't reach the app. This was flagged in previous reviews.
Update to actual PillarX store URLs:
<a - href="https://www.apple.com/app-store/" + href="https://apps.apple.com/app/pillarx/id[ACTUAL_APP_ID]" target="_blank" rel="noreferrer" className="plausible-event-name=Download+App+Store" > ... <a - href="https://play.google.com/store/apps/" + href="https://play.google.com/store/apps/details?id=com.pillarproject.wallet" target="_blank" rel="noreferrer" className="plausible-event-name=Download+Google+Play" >src/components/LandingPage/Header/index.jsx (3)
58-63: Replace hash NavLink with anchors or real routes (RR v5).NavLink to="/#superpowers" and to="/#about" won’t match routes; active state won’t work. Use same‑page anchors or define real routes. Based on learnings
Apply this diff if these are same‑page sections:
- <NavLink to="/#superpowers">Superpowers</NavLink> + <a href="#superpowers">Superpowers</a> ... - <NavLink to="/#about" className="active--no-style"> - About - </NavLink> + <a href="#about">About</a>Or create routes (/superpowers, /about) and then use NavLink to those paths.
Verify routes exist:
#!/bin/bash # Look for explicit routes for /superpowers or /about rg -n -C3 -P '(<Route[^>]+path=["'\'']/(superpowers|about)["'\''])'
4-4: Remove ESLint disable; fix the root cause.After converting the dropdown trigger to a button, this disable is no longer needed.
-/* eslint-disable jsx-a11y/anchor-is-valid */
68-78: Fix dropdown trigger semantics and add keyboard/ARIA; submenu is inaccessible (esp. mobile).without href is invalid; no state/ARIA; and UL onClick toggles the entire menu, likely preventing opening the submenu on mobile. Use a button trigger, manage open state, stop event bubbling, and add ARIA.
Apply this diff here:
- <li> - <a>Cooperation</a> - <ul> + <li> + <button + type="button" + className="header__menu-button" + aria-haspopup="true" + aria-expanded={isCooperationMenuOpen} + onClick={(e) => { e.stopPropagation(); setIsCooperationMenuOpen(v => !v); }} + onKeyDown={(e) => { if (e.key === 'Escape') setIsCooperationMenuOpen(false); }} + > + Cooperation + </button> + <ul role="menu" hidden={!isCooperationMenuOpen}> <li> - <NavLink to="/developers">For dApp</NavLink> + <NavLink to="/developers" role="menuitem">For dApp</NavLink> </li> <li> - <NavLink to="/advertising">Advertising</NavLink> + <NavLink to="/advertising" role="menuitem">Advertising</NavLink> </li> </ul> </li>And add state at the top of the component:
const [isCooperationMenuOpen, setIsCooperationMenuOpen] = useState(false);
🧹 Nitpick comments (3)
src/pages/Landing.jsx (2)
50-63: Consider adding fallback content for video elements.The video elements lack fallback content for browsers that don't support the
<video>tag or when videos fail to load. While rare in modern browsers, providing fallback content improves robustness.Add fallback text or images inside each video element:
<video width="100%" preload="none" autoPlay loop muted playsInline poster="https://cdn.pillarx.app/home-superpowers-1.webp" > <source src="https://cdn.pillarx.app/home-superpowers-1.mp4" type="video/mp4" /> + <img + src="https://cdn.pillarx.app/home-superpowers-1.webp" + alt="Token Tiles feature preview" + /> </video>Apply similar changes to the other two video elements.
Also applies to: 83-96, 118-131
50-131: Consider performance impact of multiple autoplaying videos.Three videos autoplay simultaneously on page load. While
preload="none"helps, autoplaying multiple videos can still strain lower-end devices and consume bandwidth on mobile connections.Consider implementing Intersection Observer to autoplay videos only when they enter the viewport:
import { useEffect, useRef } from 'react'; // In your component: const videoRef = useRef(null); useEffect(() => { const observer = new IntersectionObserver( (entries) => { entries.forEach((entry) => { if (entry.isIntersecting) { entry.target.play(); } else { entry.target.pause(); } }); }, { threshold: 0.5 } ); if (videoRef.current) { observer.observe(videoRef.current); } return () => observer.disconnect(); }, []); // Then on your video element: <video ref={videoRef} /* other props */>This ensures only visible videos play, improving performance and user experience.
src/components/LandingPage/Header/index.jsx (1)
46-46: Add intrinsic size to logo to reduce CLS.Specify width/height (or ensure via CSS with aspect‑ratio) to prevent layout shift. Optional.
- <img src="https://cdn.pillarx.app/pillarXLogo.png" alt="pillar-x-logo" /> + <img src="https://cdn.pillarx.app/pillarXLogo.png" alt="pillar-x-logo" width="180" height="36" />Adjust dimensions to the actual asset.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/LandingPage/Footer/index.jsx(2 hunks)src/components/LandingPage/Header/index.jsx(3 hunks)src/components/LandingPage/MailChimp/index.jsx(1 hunks)src/pages/Landing.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/LandingPage/MailChimp/index.jsx
- src/components/LandingPage/Footer/index.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/Landing.jsx (1)
src/components/LandingPage/MailChimp/index.jsx (1)
MailChimp(5-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (7)
src/pages/Landing.jsx (6)
1-8: LGTM! Router integration properly implemented.The imports are correct for react-router-dom v5.3.3, and the new MailChimp component integration follows the PR objectives.
29-34: Excellent use of client-side routing.Replacing the anchor tag with
Linkfrom react-router-dom improves navigation performance by avoiding full page reloads. The Plausible tracking is correctly preserved.
205-216: MailChimp integration looks good.The MailChimp component is properly integrated with email validation and Plausible event tracking (as seen in the component implementation). The placement and context are appropriate.
249-251: Good use of client-side routing for web login.Using
Linkinstead of an anchor tag for the web login option provides a better user experience with faster navigation.
258-287: About section is well-structured.The about section uses proper semantic HTML, includes appropriate external link security attributes (
rel="noreferrer"), and implements UTM tracking for analytics. The content flow and heading hierarchy are correct.
57-57: CDN poster images verifiedAll poster URLs (home-superpowers-1/2/3.webp) return HTTP 200 OK; no further action required.
src/components/LandingPage/Header/index.jsx (1)
5-7: Remove ScrollRestoration replacement suggestion
react-router-dom v6.18.0 supports ScrollRestoration; no custom scroll-to-top needed.Likely an incorrect or invalid review comment.
github-actions
bot
temporarily deployed
to
Preview (feat/new-landing-page)
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Changes