Vpn frontend/phase 2 #173
Vpn frontend/phase 2 #173
Conversation
Deploying blinklabs-vpn with
|
| Latest commit: |
a7d6fae
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://70e43138.blinklabs-vpn.pages.dev |
| Branch Preview URL: | https://vpn-frontend-phase-2.blinklabs-vpn.pages.dev |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive modal-based UX overhaul replacing toast notifications throughout the application. It adds new modal components (ConfirmModal, WalletPickerModal) and button components (SpinningBorderButton), removes the CustomToast and toast utility module, and refactors the wallet connection and VPN purchase flows to use state-managed modals instead of inline components. Additionally, it adds a new InstallGuide page, restructures existing pages (Home, HowItWorks, PrivacyPolicy, DocsFaqs) with simplified or redesigned layouts, updates the VpnInstance component with new renewal/purchase handlers and state management, and modifies the wallet store to expose modal controls instead of relying on toast notifications. Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 23 files
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/components/ConfirmModal.tsx">
<violation number="1" location="src/components/ConfirmModal.tsx:33">
P2: Modal is missing Escape key support for accessibility. Users should be able to close the modal by pressing Escape, which is a standard expectation for dialog components.</violation>
</file>
<file name="src/stores/walletStore.ts">
<violation number="1" location="src/stores/walletStore.ts:100">
P2: Replacing `showError()` with `console.error()` removes user-facing feedback for network mismatch errors. Users will see a generic "Failed to connect" message instead of the specific network mismatch explanation. Consider returning an error object or re-adding toast notification so users know to switch wallet networks.</violation>
</file>
<file name="src/components/WalletModal.tsx">
<violation number="1" location="src/components/WalletModal.tsx:27">
P2: State `showDisconnectConfirm` is not reset when the modal closes. If the user opens the disconnect confirmation, then closes the main modal via backdrop click, the confirmation modal will unexpectedly appear when the wallet modal is reopened. Add `setShowDisconnectConfirm(false);` to the cleanup function.</violation>
</file>
<file name="src/components/WalletConnection.tsx">
<violation number="1" location="src/components/WalletConnection.tsx:263">
P1: Empty div renders no content when connecting. The "Connecting to {walletName}..." status message was removed, leaving users with no visual feedback during wallet connection.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| const DRAG_DISMISS_THRESHOLD = 200; | ||
| const DRAG_MAX_PULL = 260; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
P2: Modal is missing Escape key support for accessibility. Users should be able to close the modal by pressing Escape, which is a standard expectation for dialog components.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/ConfirmModal.tsx, line 33:
<comment>Modal is missing Escape key support for accessibility. Users should be able to close the modal by pressing Escape, which is a standard expectation for dialog components.</comment>
<file context>
@@ -0,0 +1,130 @@
+ const DRAG_DISMISS_THRESHOLD = 200;
+ const DRAG_MAX_PULL = 260;
+
+ useEffect(() => {
+ if (!isOpen) return;
+
</file context>
| const walletNetworkLabel = formatWalletNetworkLabel(walletNetworkId); | ||
| showError( | ||
| `Wallet network mismatch. This app is configured for ${APP_NETWORK_LABEL}, but your wallet is connected to ${walletNetworkLabel}. Please switch networks in your wallet and try again.`, | ||
| console.error( |
There was a problem hiding this comment.
P2: Replacing showError() with console.error() removes user-facing feedback for network mismatch errors. Users will see a generic "Failed to connect" message instead of the specific network mismatch explanation. Consider returning an error object or re-adding toast notification so users know to switch wallet networks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/stores/walletStore.ts, line 100:
<comment>Replacing `showError()` with `console.error()` removes user-facing feedback for network mismatch errors. Users will see a generic "Failed to connect" message instead of the specific network mismatch explanation. Consider returning an error object or re-adding toast notification so users know to switch wallet networks.</comment>
<file context>
@@ -98,8 +97,8 @@ export const useWalletStore = create<WalletState>()(
const walletNetworkLabel = formatWalletNetworkLabel(walletNetworkId);
- showError(
- `Wallet network mismatch. This app is configured for ${APP_NETWORK_LABEL}, but your wallet is connected to ${walletNetworkLabel}. Please switch networks in your wallet and try again.`,
+ console.error(
+ `Wallet network mismatch. App expects ${APP_NETWORK_LABEL}, but wallet is on ${walletNetworkLabel}.`,
);
</file context>
|
|
||
| return () => { | ||
| cancelAnimationFrame(frame); | ||
| setIsVisible(false); |
There was a problem hiding this comment.
P2: State showDisconnectConfirm is not reset when the modal closes. If the user opens the disconnect confirmation, then closes the main modal via backdrop click, the confirmation modal will unexpectedly appear when the wallet modal is reopened. Add setShowDisconnectConfirm(false); to the cleanup function.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/WalletModal.tsx, line 27:
<comment>State `showDisconnectConfirm` is not reset when the modal closes. If the user opens the disconnect confirmation, then closes the main modal via backdrop click, the confirmation modal will unexpectedly appear when the wallet modal is reopened. Add `setShowDisconnectConfirm(false);` to the cleanup function.</comment>
<file context>
@@ -10,52 +12,107 @@ interface WalletModalProps {
+
+ return () => {
+ cancelAnimationFrame(frame);
+ setIsVisible(false);
+ document.body.style.overflow = previousOverflow;
+ };
</file context>
| setIsVisible(false); | |
| setIsVisible(false); | |
| setShowDisconnectConfirm(false); |
| )} | ||
| {isConnecting && pendingWallet && ( | ||
| <p className={`${statusClasses} animate-pulse`}>Connecting to {pendingWallet}...</p> | ||
| <div className={statusClasses}></div> |
There was a problem hiding this comment.
P1: Empty div renders no content when connecting. The "Connecting to {walletName}..." status message was removed, leaving users with no visual feedback during wallet connection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/WalletConnection.tsx, line 263:
<comment>Empty div renders no content when connecting. The "Connecting to {walletName}..." status message was removed, leaving users with no visual feedback during wallet connection.</comment>
<file context>
@@ -264,42 +249,23 @@ const WalletConnection = ({
)}
{isConnecting && pendingWallet && (
- <p className={`${statusClasses} animate-pulse`}>Connecting to {pendingWallet}...</p>
+ <div className={statusClasses}></div>
)}
</>
</file context>
| <div className={statusClasses}></div> | |
| <p className={`${statusClasses} animate-pulse`}>Connecting to {pendingWallet}...</p> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/stores/walletStore.ts (2)
59-61: Unsafe non-null assertion on regex match result.If
cleanHexis an empty string,match(/.{1,2}/g)returnsnull, and the!assertion will cause a runtime error when calling.map().- const bytes = new Uint8Array( - cleanHex.match(/.{1,2}/g)!.map((byte) => parseInt(byte, 16)), - ); + const matches = cleanHex.match(/.{1,2}/g); + if (!matches) { + return null; + } + const bytes = new Uint8Array( + matches.map((byte) => parseInt(byte, 16)), + );
98-114: Network mismatch errors are now silent to users.Replacing toast notifications with
console.errormeans users receive no visible feedback when their wallet is on the wrong network. They'll simply see the connection fail with no explanation.Consider surfacing this error through the UI—perhaps by returning an error reason from
connect()that the calling component can display, or by setting an error state in the store that components can read.
🧹 Nitpick comments (6)
src/pages/DocsFaqs.tsx (1)
416-432: Consider updating button text for clarity.The link now correctly navigates to "/" (which renders the Account page per the routing changes), but the button text "Go to Account Page" could be clarified to "Go to Home" or simply "Get Started" to better reflect the user experience, as "/" is now the application's main landing route.
If you prefer to keep the current text, no changes are needed—it's a minor wording consideration.
src/components/Navigation.tsx (3)
20-35: Avoid re-subscribing the resize listener on every menu toggle.Including
isMenuOpenin the dependency array causesaddEventListener/removeEventListenerchurn whenever the menu opens/closes. Consider using a ref forisMenuOpen, or usingmatchMedia('(min-width: 768px)')with a stable handler.
36-38: Route-change menu close effect is correct but non-idiomatic.Returning
() => setIsMenuOpen(false)relies on cleanup timing to close the menu; it works, butsetIsMenuOpen(false)in the effect body would be clearer.
83-125: Hamburger/menu accessibility + UX polish.Consider toggling the hamburger image
alt(and/oraria-label) based onisMenuOpen, and optionally closing the menu on outside click /Escapefor mobile usability.src/components/ConfirmModal.tsx (1)
33-47: Scroll lock restoration can break with nested modals.If two modals open, the first cleanup can restore
overflowwhile the second is still open. Consider a small ref-counted scroll-lock helper.src/pages/Account.tsx (1)
201-203: Region selection is effectively fixed toregions[0].Given
payloadRegion = regions[0] ?? "", the “select a region” errors/tooltips may be misleading unless region selection UI exists elsewhere. Either wire region selection intopayloadRegionor drop the “select region” validation/tooltips.Also applies to: 267-270, 381-384
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
public/checks.svgis excluded by!**/*.svgpublic/hamburger.svgis excluded by!**/*.svg
📒 Files selected for processing (21)
src/App.tsx(1 hunks)src/components/ConfirmModal.tsx(1 hunks)src/components/CustomToast.tsx(0 hunks)src/components/HeroSection.tsx(2 hunks)src/components/LoadingOverlay.tsx(1 hunks)src/components/Navigation.tsx(3 hunks)src/components/PurchaseCard.tsx(1 hunks)src/components/VpnInstance.tsx(5 hunks)src/components/WalletConnection.tsx(7 hunks)src/components/WalletModal.tsx(2 hunks)src/components/__tests__/WalletConnection.test.tsx(2 hunks)src/index.css(1 hunks)src/pages/Account.tsx(13 hunks)src/pages/DocsFaqs.tsx(2 hunks)src/pages/Home.tsx(1 hunks)src/pages/HowItWorks.tsx(3 hunks)src/pages/PrivacyPolicy.tsx(7 hunks)src/routes/index.tsx(2 hunks)src/stores/walletStore.ts(1 hunks)src/utils/toast.tsx(0 hunks)tailwind.config.js(1 hunks)
💤 Files with no reviewable changes (2)
- src/components/CustomToast.tsx
- src/utils/toast.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/WalletModal.tsx (1)
src/utils/formatAddress.ts (1)
truncateAddress(1-13)
src/pages/Account.tsx (6)
src/api/hooks/index.ts (2)
useSignup(2-2)useRenewVpn(8-8)src/api/hooks/useSignup.ts (1)
useSignup(5-13)src/api/hooks/useRenewVpn.ts (1)
useRenewVpn(5-12)src/utils/pendingTransactions.ts (2)
addPendingTransaction(31-59)getPendingTransactions(14-25)src/api/types.ts (1)
ClientInfo(45-50)src/utils/instanceSort.ts (3)
sortVpnInstances(17-63)filterOptions(65-70)SortOption(1-5)
⏰ 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: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (27)
src/pages/PrivacyPolicy.tsx (2)
5-7: LGTM! Clean visual enhancement with decorative gradients.The translucent background and blurred gradient blobs create an appealing frosted-glass aesthetic. The
pointer-events-noneensures the decorative elements don't interfere with user interactions.
15-15: LGTM! Consistent translucent styling applied across all sections.The uniform application of
bg-white/10 border border-white/20 backdrop-blur-xlcreates a cohesive frosted-glass aesthetic throughout the page. The styling updates are consistent, maintain readability, and align well with the PR's visual overhaul objectives.Also applies to: 18-18, 40-40, 69-69, 97-97, 143-143, 193-193, 220-220
src/components/__tests__/WalletConnection.test.tsx (2)
135-135: LGTM! Test description updated appropriately.The test name change from toast-specific wording to the more generic "surface error" correctly reflects the shift away from toast notifications while maintaining clear test intent.
153-155: The assertion correctly matches the component's error message.The WalletConnection component at line 202 displays the error message:
"${walletName} wallet is not installed. Please install it from the official website."The test assertion using/wallet is not installed/iwill successfully match this substring in a case-insensitive manner. The test properly simulates the wallet not being installed by removing it fromwindow.cardanoand expects the error to be rendered in the DOM, which aligns with the component implementation.src/index.css (1)
5-5: LGTM!Adding
min-height: 100vhto the body ensures consistent full-viewport layout support across the application, aligning with the min-h-screen utilities used throughout the UI updates in this PR.src/components/LoadingOverlay.tsx (1)
28-28: LGTM!The addition of
my-1vertical margin improves spacing around the top message element when present.src/routes/index.tsx (1)
1-1: Navigate import added correctly.The Navigate component import is properly added to support the redirect from "/account" to "/".
tailwind.config.js (1)
7-9: LGTM!Wrapping font family names with spaces in double quotes is a best practice that ensures proper parsing and prevents potential issues with CSS font declarations.
src/pages/Home.tsx (2)
9-9: Verify navigation target aligns with routing changes.The
handleGetStartedfunction now navigates to "/" (root). However, per the routing changes insrc/routes/index.tsx, the root path now renders the Account component, not Home. This means clicking "Get Started" on the Home page would navigate to the Account page.Please confirm this is the intended user flow. If Home should remain the landing page, consider adjusting the routing structure.
13-13: LGTM!The updated wrapper styling with solid dark background (
bg-[#040617]) and full viewport height (min-h-screen) aligns with the global styling updates across the PR.src/pages/DocsFaqs.tsx (1)
118-118: LGTM!The background styling change from gradient to translucent black overlay (
bg-black/30) is consistent with the visual updates across the PR.src/components/HeroSection.tsx (2)
14-14: LGTM!Simplifying the backdrop image className by removing responsive top adjustments makes the styling more maintainable while maintaining visual consistency.
39-39: LGTM!The simplified flex container removes unnecessary responsive layout complexity while maintaining the centered button layout. The component now delegates all navigation logic to the parent via the
onGetStartedcallback, improving separation of concerns.src/components/PurchaseCard.tsx (4)
1-6: LGTM!The
PurchaseOptiontype is well-structured with clear, descriptive property names for value, label, time display, and price.
8-15: LGTM!The
PurchaseCardPropstype properly defines all required props with appropriate types. TheshowTooltipsandformatPriceprops provide good flexibility for customization and formatting.
25-25: LGTM!The disabled state logic is correct: the card should be disabled when processing a transaction or when the wallet is not connected.
28-55: LGTM!The component renders a well-structured purchase card with:
- Gradient border and background styling
- Clear price display with ADA denomination
- Setup fee notice
- Conditional button states (Processing/Connect Wallet/Buy Now)
- Proper disabled state handling
- Optional tooltip support
The implementation is clean and follows React best practices.
src/components/VpnInstance.tsx (2)
49-54: Region formatting may produce unexpected results for short region codes.The formatting logic
region.slice(0, 2).toUpperCase() + region.slice(2)assumes regions are at least 2 characters. For single-character regions (edge case),slice(2)returns an empty string, which is fine. However, consider if this is the intended display format—it uppercases only the first two characters (e.g., "uswest" → "USwest").If regions are expected to be ISO codes like "us", "eu", etc., this works correctly. If they could be longer identifiers, you may want to uppercase the entire string or just the first character.
49-135: Typography updates look consistent.The responsive text sizing (
text-xs md:text-sm) and font-weight adjustments are applied consistently across all labels and buttons in the component. The "Time Remaining" label change on Line 70 improves clarity over the previous "Expires" wording.src/pages/HowItWorks.tsx (2)
72-72: Dependency onsectionsis technically correct but unnecessary.Since
sectionsis memoized with an empty dependency array ([]), it has a stable reference and will never change. Including it in the effect's dependency array is harmless and satisfies exhaustive-deps linting, but provides no runtime benefit.
75-77: UI restructure with card-based layout looks good.The decorative blur elements and new card-based section layouts with consistent styling (gradients, borders, backdrop blur) align well with the PR's broader visual updates. The external links properly include
target="_blank"withrel="noopener noreferrer"for security.Also applies to: 108-340
src/App.tsx (1)
9-20: Background styling change is acceptable.Using inline styles for complex multi-layer backgrounds is a reasonable approach when Tailwind classes become unwieldy.
One consideration:
backgroundAttachment: "fixed"can cause performance issues (excessive repaints) on mobile devices during scroll. If you notice janky scrolling on mobile, consider removing the fixed attachment or applying it only on larger screens via a media query.src/stores/walletStore.ts (1)
281-295: Good addition of convenience method.The
signAndSubmitTransactionmethod cleanly combines the two-step process and properly propagates errors. This reduces boilerplate in calling code.src/components/WalletModal.tsx (3)
18-30: Scroll lock and animation effect is well-implemented.The effect correctly:
- Stores and restores the previous
overflowvalue- Uses
requestAnimationFramefor smooth animation timing- Cleans up the RAF on unmount
One minor note: calling
setIsVisible(false)in cleanup (line 27) is redundant since the component returnsnullwhen!isOpenanyway, but it doesn't cause harm.
40-65: Good accessibility and responsive modal pattern.The modal implementation includes proper ARIA attributes (
role="dialog",aria-modal,aria-label), a clickable backdrop for dismissal, and responsive behavior (bottom sheet on mobile, centered dialog on desktop). The mobile drag indicator on line 57 is a nice touch for discoverability.
106-114: Disconnect confirmation flow is a good UX addition.Using
ConfirmModalfor the disconnect action prevents accidental disconnections and provides a consistent confirmation pattern across the app.src/components/WalletConnection.tsx (1)
268-280: Props are supported in @cardano-foundation/cardano-connect-with-wallet v0.2.15.customCSS, showUnavailableWallets, limitNetwork, and peerConnectEnabled are all accepted by ConnectWalletList without type errors under strict TypeScript compilation. The customCSS string with '& > span' nesting is correct and properly applied.
| const renderConnectionFeedback = ( | ||
| errorClasses = "mb-3 rounded-md border border-red-400 bg-red-500/10 px-4 py-2 text-sm text-red-200", | ||
| statusClasses = "mb-2 text-sm text-white/80", | ||
| errorClasses = isLightTheme | ||
| ? "mb-3 rounded-md border border-red-500/30 bg-red-50 px-4 py-2 text-sm text-red-700" | ||
| : "mb-3 rounded-md border border-red-400 bg-red-500/10 px-4 py-2 text-sm text-red-200", | ||
| statusClasses = statusTextClass, | ||
| ) => ( | ||
| <> | ||
| {connectionError && ( | ||
| <div className={errorClasses}>{connectionError}</div> | ||
| )} | ||
| {isConnecting && pendingWallet && ( | ||
| <p className={`${statusClasses} animate-pulse`}>Connecting to {pendingWallet}...</p> | ||
| <div className={statusClasses}></div> | ||
| )} | ||
| </> | ||
| ); |
There was a problem hiding this comment.
Connecting state UI renders an empty element.
{isConnecting && pendingWallet && (<div className={statusClasses}></div>)} likely wants actual status text (or a spinner) to avoid a blank gap.
🤖 Prompt for AI Agents
In src/components/WalletConnection.tsx around lines 252 to 266 the
connecting-state branch renders an empty <div> (className={statusClasses}) which
produces a blank gap; replace that empty element with visible feedback by
rendering either a spinner component or text (e.g. "Connecting..." or
`Connecting to ${pendingWallet.name || pendingWallet.label}`) and include
accessibility attributes (aria-live="polite") and a small visually-hidden label
if needed; ensure the statusClasses are applied to the element that contains
this content so users see a non-empty status while connecting.
src/pages/HowItWorks.tsx
Outdated
| const sections = useMemo( | ||
| () => [ | ||
| { id: "overview", title: "Overview", number: null }, | ||
| { id: "signup", title: "Website Signup Process", number: 1 }, | ||
| { id: "validation", title: "Smart Contract Validation", number: 2 }, | ||
| { id: "indexer", title: "Indexer Processing", number: 3 }, | ||
| { | ||
| id: "authentication", | ||
| title: "Profile Download Authentication", | ||
| number: 4, | ||
| }, | ||
| { id: "setup", title: "VPN Client Setup", number: 5 }, | ||
| ], | ||
| [], | ||
| ); |
There was a problem hiding this comment.
Missing "infrastructure" section in the navigation array.
The sections array defines items for scroll tracking and sidebar navigation, but it's missing the "infrastructure" section that's rendered on lines 311-340 with id="infrastructure". This causes:
- No sidebar navigation item for the Infrastructure section
- Scroll tracking won't highlight Infrastructure when scrolled into view
{ id: "setup", title: "VPN Client Setup", number: 5 },
+ { id: "infrastructure", title: "Infrastructure", number: 6 },
],
[],
);📝 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 sections = useMemo( | |
| () => [ | |
| { id: "overview", title: "Overview", number: null }, | |
| { id: "signup", title: "Website Signup Process", number: 1 }, | |
| { id: "validation", title: "Smart Contract Validation", number: 2 }, | |
| { id: "indexer", title: "Indexer Processing", number: 3 }, | |
| { | |
| id: "authentication", | |
| title: "Profile Download Authentication", | |
| number: 4, | |
| }, | |
| { id: "setup", title: "VPN Client Setup", number: 5 }, | |
| ], | |
| [], | |
| ); | |
| const sections = useMemo( | |
| () => [ | |
| { id: "overview", title: "Overview", number: null }, | |
| { id: "signup", title: "Website Signup Process", number: 1 }, | |
| { id: "validation", title: "Smart Contract Validation", number: 2 }, | |
| { id: "indexer", title: "Indexer Processing", number: 3 }, | |
| { | |
| id: "authentication", | |
| title: "Profile Download Authentication", | |
| number: 4, | |
| }, | |
| { id: "setup", title: "VPN Client Setup", number: 5 }, | |
| { id: "infrastructure", title: "Infrastructure", number: 6 }, | |
| ], | |
| [], | |
| ); |
🤖 Prompt for AI Agents
In src/pages/HowItWorks.tsx around lines 9 to 23, the useMemo sections array is
missing the "infrastructure" entry (the page renders an element with
id="infrastructure" at lines 311-340), so add an object into the array with id:
"infrastructure", title: "Infrastructure", and the appropriate number (e.g., 6)
in the correct order to match the rendered sections; ensure the new entry is
placed where it should appear in the sidebar and that numbering remains
consistent for scroll tracking and navigation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/ConfirmModal.tsx (1)
33-47: Add Escape key support for accessibility.The modal is missing keyboard support for closing via Escape key, which is a standard accessibility expectation for dialog components.
useEffect(() => { if (!isOpen) return; + const handleKeyDown = (e: KeyboardEvent) => { + if (e.key === "Escape") onCancel(); + }; + document.addEventListener("keydown", handleKeyDown); + const frame = requestAnimationFrame(() => setIsVisible(true)); const previousOverflow = document.body.style.overflow; document.body.style.overflow = "hidden"; return () => { cancelAnimationFrame(frame); + document.removeEventListener("keydown", handleKeyDown); setIsVisible(false); document.body.style.overflow = previousOverflow; setDragOffset(0); setDragStartY(null); }; - }, [isOpen]); + }, [isOpen, onCancel]);src/components/WalletConnection.tsx (1)
278-280: Empty element renders no visual feedback during wallet connection.When
isConnecting && pendingWalletis true, an empty<div>is rendered. Users have no indication that connection is in progress. This was flagged in a previous review.{isConnecting && pendingWallet && ( - <div className={statusClasses}></div> + <p className={`${statusClasses} animate-pulse`}> + Connecting to {pendingWallet}... + </p> )}
🧹 Nitpick comments (10)
src/pages/HowItWorks.tsx (2)
5-12: Nice simplification of the page layout; consider extracting the repeated “card” wrapper for maintainability.You repeat the same wrapper classes for each section (
bg-white/10 rounded-xl p-8 border ... backdrop-blur-xl). Consider extracting a smallSectionCardcomponent (or mapping over a data array) to reduce duplication and make future edits less error-prone.
41-56: Optional privacy hardening: setreferrerPolicyon outbound links.Given the product’s privacy posture, consider:
<a href="https://github.com/blinklabs-io/vpn-frontend" className="text-blue-400 hover:text-blue-300 underline" target="_blank" rel="noopener noreferrer" + referrerPolicy="no-referrer" >(Repeat for the other external GitHub links if desired.)
Also applies to: 72-90, 123-135, 185-196, 224-240
src/components/Navigation.tsx (2)
92-125: Consider adding click-outside-to-close for mobile menu.The mobile menu closes on link click and route change, but doesn't close when the user taps outside the menu panel. This is a common UX pattern for dropdown menus.
+ const menuRef = useRef<HTMLDivElement>(null); + + useEffect(() => { + if (!isMenuOpen) return; + const handleClickOutside = (e: MouseEvent) => { + if (menuRef.current && !menuRef.current.contains(e.target as Node)) { + setIsMenuOpen(false); + } + }; + document.addEventListener("mousedown", handleClickOutside); + return () => document.removeEventListener("mousedown", handleClickOutside); + }, [isMenuOpen]);Then add
ref={menuRef}to the menu container div at line 93.
36-38: Verify cleanup effect intent on route changes.The effect cleanup (
return () => setIsMenuOpen(false)) runs both on unmount and whenlocation.pathnamechanges. While this works, setting state during cleanup on dependency change is unconventional. A cleaner approach:useEffect(() => { - return () => setIsMenuOpen(false); - }, [location.pathname]); + setIsMenuOpen(false); + }, [location.pathname]);This explicitly closes the menu when the path changes, rather than relying on cleanup semantics.
src/components/ConfirmModal.tsx (1)
92-95: Mouse drag applies to entire modal content, not just drag handle.The
onMouseDown/Move/Uphandlers are attached to the entire modal panel, meaning users dragging to select text or interact with content will trigger the drag-to-dismiss behavior. Consider restricting mouse drag to the drag handle element only (the bar at line 97), while keeping touch gestures on the full panel for mobile UX.<div className={`...`} style={{ transform: transformValue }} onTouchStart={(e) => handleDragStart(e.touches[0]?.clientY)} onTouchMove={(e) => handleDragMove(e.touches[0]?.clientY)} onTouchEnd={handleDragEnd} - onMouseDown={(e) => handleDragStart(e.clientY)} - onMouseMove={(e) => handleDragMove(e.clientY)} - onMouseUp={handleDragEnd} - onMouseLeave={() => dragStartY !== null && handleDragEnd()} > - <div className="absolute top-4 left-1/2 -translate-x-1/2 h-1.5 w-20 rounded-full bg-gray-200 md:hidden" /> + <div + className="absolute top-4 left-1/2 -translate-x-1/2 h-1.5 w-20 rounded-full bg-gray-200 md:hidden cursor-grab" + onMouseDown={(e) => handleDragStart(e.clientY)} + onMouseMove={(e) => handleDragMove(e.clientY)} + onMouseUp={handleDragEnd} + onMouseLeave={() => dragStartY !== null && handleDragEnd()} + />src/components/WalletConnection.tsx (1)
332-344: Unnecessary modal rendering when wallet is connected.
renderWalletModal()is called whenisConnectedis true (line 341), but there's no UI element to open the modal in this state—the only button is "Disconnect". The modal will never be shown here.if (isConnected) { return ( <div className="flex items-center space-x-4"> <button onClick={disconnect} className={buttonClasses} > Disconnect </button> - {renderWalletModal()} </div> ); }src/pages/Account.tsx (3)
448-450: Redundant type assertion and potential zero-duration fallback.The
as numberassertion is unnecessary sinceMath.maxalready returns a number. Additionally, the fallback chain could result in0if bothclient.durationis undefined and the expiration is in the past.const normalizedDurationMs = normalizeDurationMs(client.duration) || - Math.max(expirationTime - now.getTime(), 0) as number; + Math.max(expirationTime - now.getTime(), 0);The
|| 0fallback fromMath.max(..., 0)is fine for expired instances, but you may want to handle this case explicitly in the UI rather than showing "0 hours".
484-488: Sort option is hardcoded; filter UI not implemented.
sortVpnInstancesis called withfilterOptions[0].value("default"), butfilterOptionsis imported and unused for any dropdown/selector. If filtering is planned, consider adding a TODO or removing the unused import.-import { - sortVpnInstances, - filterOptions, - type SortOption, -} from "../utils/instanceSort"; +import { sortVpnInstances, type SortOption } from "../utils/instanceSort";And simplify the call:
- return sortVpnInstances( - allInstances, - filterOptions[0].value as SortOption, - ); + return sortVpnInstances(allInstances, "default");
505-507: Minor: Extra whitespace in className.There's a double space in the className string which appears unintentional.
<div - className="min-h-screen w-full overflow-x-hidden flex flex-col items-center pt-16 pb-16" + className="min-h-screen w-full overflow-x-hidden flex flex-col items-center pt-16 pb-16" >src/components/HeroSection.tsx (1)
23-23: Remove unusedsizesattribute.The
sizesattribute only works withsrcSetfor responsive image loading. Since there's nosrcSetdefined, this attribute has no effect and can be removed for cleaner code.Apply this diff:
<img src={heroCenter} alt="Hero Center" className="absolute top-1/4 sm:top-1/3 left-1/2 -translate-x-1/2 -translate-y-1/2 w-[clamp(11rem,45vw,27.5rem)] max-w-[90vw] h-auto aspect-[439/321] object-contain z-20 flex-shrink-0 pointer-events-none" loading="eager" - sizes="(max-width: 640px) 70vw, (max-width: 1024px) 50vw, 30vw" fetchPriority="high" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/components/ConfirmModal.tsx(1 hunks)src/components/HeroSection.tsx(2 hunks)src/components/Navigation.tsx(3 hunks)src/components/VpnInstance.tsx(5 hunks)src/components/WalletConnection.tsx(8 hunks)src/pages/Account.tsx(13 hunks)src/pages/Home.tsx(1 hunks)src/pages/HowItWorks.tsx(1 hunks)src/routes/index.tsx(1 hunks)src/stores/walletStore.ts(3 hunks)src/utils/pendingTransactions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/routes/index.tsx
- src/stores/walletStore.ts
- src/components/VpnInstance.tsx
- src/pages/Home.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/Account.tsx (6)
src/api/hooks/index.ts (2)
useSignup(2-2)useRenewVpn(8-8)src/api/hooks/useSignup.ts (1)
useSignup(5-13)src/api/hooks/useRenewVpn.ts (1)
useRenewVpn(5-12)src/utils/pendingTransactions.ts (2)
addPendingTransaction(31-59)getPendingTransactions(14-25)src/api/types.ts (1)
ClientInfo(45-50)src/utils/instanceSort.ts (3)
sortVpnInstances(17-63)filterOptions(65-70)SortOption(1-5)
⏰ 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). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
src/utils/pendingTransactions.ts (1)
1-9: LGTM! Type change aligns with numeric duration handling.The
durationfield change fromstringtonumber(in milliseconds) is appropriate and consistent with how durations are used throughout the codebase (e.g.,normalizeDurationMs,formatDuration).src/pages/Account.tsx (3)
112-114: LGTM! Mutation hooks correctly updated.The
useSignupanduseRenewVpnhooks are now called without inline callbacks, with success/error handling done via thependingTxstate andConfirmModal. This aligns with the PR's modal-based flow.
335-358: LGTM! Pending transaction flow correctly stores numeric duration.The
handleConfirmSubmitfunction properly storespendingTx.durationMs(a number) in the pending transaction, which aligns with the updatedPendingTransaction.durationtype. This addresses the previously flagged type mismatch issue.
203-203: [Rewritten review comment]
[Classification tag]src/components/HeroSection.tsx (3)
14-14: LGTM!The backdrop image styling is correct. The classes work together to create a full-screen background that doesn't intercept pointer events.
39-46: LGTM!The simplified button group with a single "Get Started" action is clean and aligns well with the PR's objective to streamline the hero section. The button has proper styling and click handling.
21-21: No action needed. Theaspect-[439/321]property is used withobject-contain, which preserves the actual image aspect ratio and prevents distortion. The aspect ratio value sets a container constraint, butobject-containensures the image scales without distortion regardless.
| <section id="overview"> | ||
| <div className="bg-white/10 rounded-xl p-8 border border-white/20 backdrop-blur-xl"> | ||
| <h1 className="text-3xl font-semibold text-white mb-6"> | ||
| How It Works | ||
| </h1> | ||
| <p className="text-gray-300 text-lg leading-relaxed"> | ||
| Our VPN system consists of multiple components, including | ||
| smart contracts, a custom chain indexer and API, a web | ||
| frontend, and OpenVPN. These pieces work together to | ||
| facilitate signup and management of your VPN subscription | ||
| and access to the VPN tunnel in a manner that focuses on | ||
| privacy. | ||
| </p> | ||
| </div> | ||
| <h2 className="text-2xl font-semibold text-white"> | ||
| Web Frontend | ||
| </h2> | ||
| </div> | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| Our{" "} | ||
| <a | ||
| href="https://github.com/blinklabs-io/vpn-frontend" | ||
| className="text-blue-400 hover:text-blue-300 underline" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| web frontend | ||
| </a>{" "} | ||
| provides a convenient interface over our API for managing your | ||
| VPN subscriptions. It allows connecting a CIP-30 compatible | ||
| wallet, which is used to determine the wallet address to query | ||
| our backend API for subscriptions, as well as for authentication | ||
| and transaction signing. While we take special care to not log | ||
| your IP address, it will be visible to Cloudflare where the web | ||
| frontend is hosted. | ||
| </p> | ||
| </div> | ||
| </section> | ||
| </section> | ||
|
|
||
| <section id="validation" className="mb-16"> | ||
| <div className="bg-[#00000020] rounded-lg p-8 border border-[#ffffff10]"> | ||
| <div className="flex items-center mb-6"> | ||
| <div className="w-12 h-12 bg-transparent border-2 border-white rounded-full flex items-center justify-center text-white font-bold text-lg mr-4"> | ||
| 2 | ||
| <section id="signup"> | ||
| <div className="bg-white/10 rounded-xl p-8 border border-white/20 backdrop-blur-xl"> | ||
| <div className="flex items-center mb-6"> | ||
| <div className="w-12 h-12 bg-transparent border-2 border-white rounded-full flex items-center justify-center text-white font-bold text-lg mr-4"> | ||
| 1 | ||
| </div> | ||
| <h2 className="text-2xl font-semibold text-white"> | ||
| Web Frontend | ||
| </h2> | ||
| </div> | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| Our{" "} | ||
| <a | ||
| href="https://github.com/blinklabs-io/vpn-frontend" | ||
| className="text-blue-400 hover:text-blue-300 underline" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| web frontend | ||
| </a>{" "} | ||
| provides a convenient interface over our API for managing | ||
| your VPN subscriptions. It allows connecting a CIP-30 | ||
| compatible wallet, which is used to determine the wallet | ||
| address to query our backend API for subscriptions, as well | ||
| as for authentication and transaction signing. While we take | ||
| special care to not log your IP address, it will be visible | ||
| to Cloudflare where the web frontend is hosted. | ||
| </p> | ||
| </div> | ||
| <h2 className="text-2xl font-semibold text-white"> | ||
| Smart Contract | ||
| </h2> | ||
| </div> | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| Our{" "} | ||
| <a | ||
| href="https://github.com/blinklabs-io/vpn-contracts" | ||
| className="text-blue-400 hover:text-blue-300 underline" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| smart contract | ||
| </a>{" "} | ||
| facilitates all on-chain operations, such as signups and | ||
| renewals. It validates that all subscriptions conform to | ||
| available regions and plans, the datum matches the expected | ||
| shape, all funds are the correct amounts and going to the | ||
| correct places, and other various sanity checks. All | ||
| subscription information is stored on-chain in the smart | ||
| contract's address. Because all interactions with smart | ||
| contracts are public, the wallet address that you signed up | ||
| with, as well as your chosen plan and region, are visible to | ||
| anybody. | ||
| </p> | ||
| </section> | ||
|
|
||
| <p className="text-gray-300 leading-relaxed"> | ||
| The current regions and plans are provided by a reference input | ||
| containing an asset with an attached datum (refdata) in the TX | ||
| output. Signups consist of a mint operation for a client asset | ||
| with an associated datum containing the owner's payment PKH, the | ||
| chosen region, and the subscription expiration in UNIX epoch | ||
| time. This asset stays within the contract and further | ||
| operations on it are validated by signing the TX with the | ||
| owner's payment PKH. A subscription renewal involves spending | ||
| the current client asset from the contract and sending it back | ||
| to the contract with an updated datum. Any updates to the | ||
| expiration in the client datum are validated against any current | ||
| expiration and the plans defined in the refdata. | ||
| </p> | ||
| </div> | ||
| </section> | ||
| <section id="validation"> | ||
| <div className="bg-white/10 rounded-xl p-8 border border-white/20 backdrop-blur-xl"> | ||
| <div className="flex items-center mb-6"> | ||
| <div className="w-12 h-12 bg-transparent border-2 border-white rounded-full flex items-center justify-center text-white font-bold text-lg mr-4"> | ||
| 2 | ||
| </div> | ||
| <h2 className="text-2xl font-semibold text-white"> | ||
| Smart Contract | ||
| </h2> | ||
| </div> | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| Our{" "} | ||
| <a | ||
| href="https://github.com/blinklabs-io/vpn-contracts" | ||
| className="text-blue-400 hover:text-blue-300 underline" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| smart contract | ||
| </a>{" "} | ||
| facilitates all on-chain operations, such as signups and | ||
| renewals. It validates that all subscriptions conform to | ||
| available regions and plans, the datum matches the expected | ||
| shape, all funds are the correct amounts and going to the | ||
| correct places, and other various sanity checks. All | ||
| subscription information is stored on-chain in the smart | ||
| contract's address. Because all interactions with smart | ||
| contracts are public, the wallet address that you signed up | ||
| with, as well as your chosen plan and region, are visible to | ||
| anybody. | ||
| </p> | ||
|
|
||
| <section id="indexer" className="mb-16"> | ||
| <div className="bg-[#00000020] rounded-lg p-8 border border-[#ffffff10]"> | ||
| <div className="flex items-center mb-6"> | ||
| <div className="w-12 h-12 bg-transparent border-2 border-white rounded-full flex items-center justify-center text-white font-bold text-lg mr-4"> | ||
| 3 | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| The current regions and plans are provided by a reference | ||
| input containing an asset with an attached datum (refdata) in | ||
| the TX output. Signups consist of a mint operation for a | ||
| client asset with an associated datum containing the owner's | ||
| payment PKH, the chosen region, and the subscription | ||
| expiration in UNIX epoch time. This asset stays within the | ||
| contract and further operations on it are validated by | ||
| signing the TX with the owner's payment PKH. A subscription | ||
| renewal involves spending the current client asset from the | ||
| contract and sending it back to the contract with an updated | ||
| datum. Any updates to the expiration in the client datum are | ||
| validated against any current expiration and the plans | ||
| defined in the refdata. | ||
| </p> | ||
| </div> | ||
| <h2 className="text-2xl font-semibold text-white"> | ||
| Indexer and API | ||
| </h2> | ||
| </div> | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| Once the signup TX makes it into a block and on-chain, it will | ||
| get picked up by our{" "} | ||
| <a | ||
| href="https://github.com/blinklabs-io/vpn-indexer" | ||
| className="text-blue-400 hover:text-blue-300 underline" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| custom indexer | ||
| </a> | ||
| . The client datum will be extracted and its information written | ||
| to a SQLite database. A new client TLS certificate is generated | ||
| and signed by our CA certificate, and a new VPN client config | ||
| built and uploaded to a private S3 bucket. | ||
| </p> | ||
| </section> | ||
|
|
||
| <p className="text-gray-300 leading-relaxed"> | ||
| The on-chain data processed by our indexer is made available for | ||
| querying via our API. We also provide endpoints for building | ||
| transactions (for operations such as signup and renewal) and | ||
| fetching generated client profiles. We explicitly do not log | ||
| client IP addresses in our API. | ||
| </p> | ||
| </div> | ||
| </section> | ||
| <section id="indexer"> | ||
| <div className="bg-white/10 rounded-xl p-8 border border-white/20 backdrop-blur-xl"> | ||
| <div className="flex items-center mb-6"> | ||
| <div className="w-12 h-12 bg-transparent border-2 border-white rounded-full flex items-center justify-center text-white font-bold text-lg mr-4"> | ||
| 3 | ||
| </div> | ||
| <h2 className="text-2xl font-semibold text-white"> | ||
| Indexer and API | ||
| </h2> | ||
| </div> | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| Once the signup TX makes it into a block and on-chain, it | ||
| will get picked up by our{" "} | ||
| <a | ||
| href="https://github.com/blinklabs-io/vpn-indexer" | ||
| className="text-blue-400 hover:text-blue-300 underline" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| custom indexer | ||
| </a> | ||
| . The client datum will be extracted and its information | ||
| written to a SQLite database. A new client TLS certificate is | ||
| generated and signed by our CA certificate, and a new VPN | ||
| client config built and uploaded to a private S3 bucket. | ||
| </p> | ||
|
|
||
| <section id="authentication" className="mb-16"> | ||
| <div className="bg-[#00000020] rounded-lg p-8 border border-[#ffffff10]"> | ||
| <div className="flex items-center mb-6"> | ||
| <div className="w-12 h-12 bg-transparent border-2 border-white rounded-full flex items-center justify-center text-white font-bold text-lg mr-4"> | ||
| 4 | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| The on-chain data processed by our indexer is made available | ||
| for querying via our API. We also provide endpoints for | ||
| building transactions (for operations such as signup and | ||
| renewal) and fetching generated client profiles. We | ||
| explicitly do not log client IP addresses in our API. | ||
| </p> | ||
| </div> | ||
| <h2 className="text-2xl font-semibold text-white"> | ||
| Profile Download Authentication | ||
| </h2> | ||
| </div> | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| Once a client profile has been generated and uploaded to S3 by | ||
| our indexer, our API will allow fetching it by validating | ||
| ownership of the wallet used to do the signup. This is done by | ||
| generating a challenge string (the hex-encoded client ID and the | ||
| current UNIX epoch time), signing this message with your wallet | ||
| using the CIP-8 message signing format, and passing it to our | ||
| API. We validate the signature of the challenge message against | ||
| the wallet PKH provided at signup, and respond with a pre-signed | ||
| S3 URL to fetch the client config. A particular signed challenge | ||
| string is valid for a limited period of time to help prevent | ||
| replay attacks. | ||
| </p> | ||
| </div> | ||
| </section> | ||
| </section> | ||
|
|
||
| <section id="setup" className="mb-16"> | ||
| <div className="bg-[#00000020] rounded-lg p-8 border border-[#ffffff10]"> | ||
| <div className="flex items-center mb-6"> | ||
| <div className="w-12 h-12 bg-transparent border-2 border-white rounded-full flex items-center justify-center text-white font-bold text-lg mr-4"> | ||
| 5 | ||
| <section id="authentication"> | ||
| <div className="bg-white/10 rounded-xl p-8 border border-white/20 backdrop-blur-xl"> | ||
| <div className="flex items-center mb-6"> | ||
| <div className="w-12 h-12 bg-transparent border-2 border-white rounded-full flex items-center justify-center text-white font-bold text-lg mr-4"> | ||
| 4 | ||
| </div> | ||
| <h2 className="text-2xl font-semibold text-white"> | ||
| Profile Download Authentication | ||
| </h2> | ||
| </div> | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| Once a client profile has been generated and uploaded to S3 | ||
| by our indexer, our API will allow fetching it by validating | ||
| ownership of the wallet used to do the signup. This is done | ||
| by generating a challenge string (the hex-encoded client ID | ||
| and the current UNIX epoch time), signing this message with | ||
| your wallet using the CIP-8 message signing format, and | ||
| passing it to our API. We validate the signature of the | ||
| challenge message against the wallet PKH provided at signup, | ||
| and respond with a pre-signed S3 URL to fetch the client | ||
| config. A particular signed challenge string is valid for a | ||
| limited period of time to help prevent replay attacks. | ||
| </p> | ||
| </div> | ||
| <h2 className="text-2xl font-semibold text-white"> | ||
| OpenVPN Server | ||
| </h2> | ||
| </div> | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| We run our OpenVPN server instances from a{" "} | ||
| <a | ||
| href="https://github.com/blinklabs-io/docker-openvpn" | ||
| className="text-blue-400 hover:text-blue-300 underline" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| custom docker image | ||
| </a>{" "} | ||
| running in Kubernetes. Our image explicitly disables any logging | ||
| from OpenVPN, which means that we cannot see the IP address that | ||
| you connect with. | ||
| </p> | ||
| </section> | ||
|
|
||
| <p className="text-gray-300 leading-relaxed"> | ||
| When connecting to the VPN server, the user's client TLS | ||
| certificate from their downloaded profile will be validated | ||
| against our CA certificate when authenticating to the VPN | ||
| server. The client certificate will also be checked against a | ||
| CRL (certificate revocation list) maintained by our custom | ||
| indexer to enforce expiration. By default, you will be provided | ||
| with our hosted DNS servers a default route through the VPN, | ||
| which prevents your ISP from being able to see what you are | ||
| doing on the VPN. | ||
| </p> | ||
| </div> | ||
| </section> | ||
| <section id="setup"> | ||
| <div className="bg-white/10 rounded-xl p-8 border border-white/20 backdrop-blur-xl"> | ||
| <div className="flex items-center mb-6"> | ||
| <div className="w-12 h-12 bg-transparent border-2 border-white rounded-full flex items-center justify-center text-white font-bold text-lg mr-4"> | ||
| 5 | ||
| </div> | ||
| <h2 className="text-2xl font-semibold text-white"> | ||
| OpenVPN Server | ||
| </h2> | ||
| </div> | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| We run our OpenVPN server instances from a{" "} | ||
| <a | ||
| href="https://github.com/blinklabs-io/docker-openvpn" | ||
| className="text-blue-400 hover:text-blue-300 underline" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| custom docker image | ||
| </a>{" "} | ||
| running in Kubernetes. Our image explicitly disables any | ||
| logging from OpenVPN, which means that we cannot see the IP | ||
| address that you connect with. | ||
| </p> | ||
|
|
||
| <section id="infrastructure" className="mb-16"> | ||
| <div className="bg-[#00000020] rounded-lg p-8 border border-[#ffffff10]"> | ||
| <div className="flex items-center mb-6"> | ||
| <div className="w-12 h-12 bg-transparent border-2 border-white rounded-full flex items-center justify-center text-white font-bold text-lg mr-4"> | ||
| 6 | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| When connecting to the VPN server, the user's client TLS | ||
| certificate from their downloaded profile will be validated | ||
| against our CA certificate when authenticating to the VPN | ||
| server. The client certificate will also be checked against a | ||
| CRL (certificate revocation list) maintained by our custom | ||
| indexer to enforce expiration. By default, you will be | ||
| provided with our hosted DNS servers a default route through | ||
| the VPN, which prevents your ISP from being able to see what | ||
| you are doing on the VPN. | ||
| </p> | ||
| </div> | ||
| <h2 className="text-2xl font-semibold text-white"> | ||
| Infrastructure | ||
| </h2> | ||
| </div> | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| Our infrastructure is based in AWS and is managed via a{" "} | ||
| <a | ||
| href="https://github.com/blinklabs-io/vpn-infrastructure" | ||
| className="text-blue-400 hover:text-blue-300 underline" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| custom Terraform and helmfile setup | ||
| </a> | ||
| . Secrets are encrypted at rest using SOPS and stored within the | ||
| git repo. We utilize EKS for running containers, with the AWS | ||
| load balancer controller for managing ingress. We use S3 for | ||
| storage of generated client configs. We purposely do not | ||
| configure access logging on any load balancer or S3 bucket to | ||
| prevent storing information about people accessing our services. | ||
| </p> | ||
| </section> | ||
|
|
||
| <section id="infrastructure"> | ||
| <div className="bg-white/10 rounded-xl p-8 border border-white/20 backdrop-blur-xl"> | ||
| <div className="flex items-center mb-6"> | ||
| <div className="w-12 h-12 bg-transparent border-2 border-white rounded-full flex items-center justify-center text-white font-bold text-lg mr-4"> | ||
| 6 | ||
| </div> | ||
| <h2 className="text-2xl font-semibold text-white"> | ||
| Infrastructure | ||
| </h2> | ||
| </div> | ||
| <p className="text-gray-300 leading-relaxed"> | ||
| Our infrastructure is based in AWS and is managed via a{" "} | ||
| <a | ||
| href="https://github.com/blinklabs-io/vpn-infrastructure" | ||
| className="text-blue-400 hover:text-blue-300 underline" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| > | ||
| custom Terraform and helmfile setup | ||
| </a> | ||
| . Secrets are encrypted at rest using SOPS and stored within | ||
| the git repo. We utilize EKS for running containers, with the | ||
| AWS load balancer controller for managing ingress. We use S3 | ||
| for storage of generated client configs. We purposely do not | ||
| configure access logging on any load balancer or S3 bucket to | ||
| prevent storing information about people accessing our | ||
| services. | ||
| </p> | ||
| </div> | ||
| </section> |
There was a problem hiding this comment.
Add aria-labelledby (and heading ids) to make each <section> more accessible.
Right now the <section id="..."> blocks don’t explicitly associate to their visible heading for screen readers. A small tweak helps:
- <section id="overview">
+ <section id="overview" aria-labelledby="overview-title">
...
- <h1 className="text-3xl font-semibold text-white mb-6">
+ <h1 id="overview-title" className="text-3xl font-semibold text-white mb-6">
How It Works
</h1>Repeat for signup, validation, etc.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/pages/HowItWorks.tsx around lines 13 to 241, each <section id="...">
currently lacks an explicit aria-labelledby reference to its visible heading;
add an id to each heading (e.g., id="overview-heading", "signup-heading",
"validation-heading", etc.) and set the matching aria-labelledby on the
corresponding <section> (e.g., <section id="signup"
aria-labelledby="signup-heading">) so screen readers associate the section with
its heading; ensure the top-level overview H1 also has an id and its section
uses aria-labelledby, and keep ids unique and descriptive.
| <div className="text-center mt-12"> | ||
| <Link | ||
| to="/" | ||
| className="inline-flex items-center px-8 py-4 text-white border border-white/20 backdrop-blur-sm font-semibold rounded-xl shadow-lg hover:bg-gray-800 transition-colors" | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| strokeWidth={2} | ||
| d="M13 7l5 5m0 0l-5 5m5-5H6" | ||
| /> | ||
| </svg> | ||
| </Link> | ||
| <span className="mr-2">Back Home</span> | ||
| <svg | ||
| className="w-5 h-5" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| viewBox="0 0 24 24" | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| strokeWidth={2} | ||
| d="M13 7l5 5m0 0l-5 5m5-5H6" | ||
| /> | ||
| </svg> | ||
| </Link> |
There was a problem hiding this comment.
Mark the decorative SVG as hidden from assistive tech.
The link already has text (“Back Home”), so the SVG should be decorative:
<svg
className="w-5 h-5"
fill="none"
stroke="currentColor"
viewBox="0 0 24 24"
+ aria-hidden="true"
+ focusable="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.
| <div className="text-center mt-12"> | |
| <Link | |
| to="/" | |
| className="inline-flex items-center px-8 py-4 text-white border border-white/20 backdrop-blur-sm font-semibold rounded-xl shadow-lg hover:bg-gray-800 transition-colors" | |
| > | |
| <path | |
| strokeLinecap="round" | |
| strokeLinejoin="round" | |
| strokeWidth={2} | |
| d="M13 7l5 5m0 0l-5 5m5-5H6" | |
| /> | |
| </svg> | |
| </Link> | |
| <span className="mr-2">Back Home</span> | |
| <svg | |
| className="w-5 h-5" | |
| fill="none" | |
| stroke="currentColor" | |
| viewBox="0 0 24 24" | |
| > | |
| <path | |
| strokeLinecap="round" | |
| strokeLinejoin="round" | |
| strokeWidth={2} | |
| d="M13 7l5 5m0 0l-5 5m5-5H6" | |
| /> | |
| </svg> | |
| </Link> | |
| <div className="text-center mt-12"> | |
| <Link | |
| to="/" | |
| className="inline-flex items-center px-8 py-4 text-white border border-white/20 backdrop-blur-sm font-semibold rounded-xl shadow-lg hover:bg-gray-800 transition-colors" | |
| > | |
| <span className="mr-2">Back Home</span> | |
| <svg | |
| className="w-5 h-5" | |
| fill="none" | |
| stroke="currentColor" | |
| viewBox="0 0 24 24" | |
| aria-hidden="true" | |
| focusable="false" | |
| > | |
| <path | |
| strokeLinecap="round" | |
| strokeLinejoin="round" | |
| strokeWidth={2} | |
| d="M13 7l5 5m0 0l-5 5m5-5H6" | |
| /> | |
| </svg> | |
| </Link> |
🤖 Prompt for AI Agents
In src/pages/HowItWorks.tsx around lines 244 to 263, the inline SVG inside the
"Back Home" Link is purely decorative but not marked for assistive tech; update
the SVG element to include accessibility attributes (aria-hidden="true" and
focusable="false") so screen readers ignore it and it cannot receive keyboard
focus, keeping the visible text as the accessible label.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
src/components/WalletPickerModal.tsx (3)
18-96: Consider removing unused grid layout code.The
buildWalletListCssfunction contains comprehensive grid layout logic (lines 55-96), but line 262 always passestrue(dropdown layout). If grid layout is not needed, remove it to reduce complexity.Apply this diff if grid layout is not needed:
-const buildWalletListCss = (isDropdownLayout: boolean) => { +const buildWalletListCss = () => { const textColor = "#0f172a"; const borderColor = "rgba(15, 23, 42, 0.15)"; const hoverBg = "rgba(15, 23, 42, 0.06)"; const hoverBorder = "rgba(15, 23, 42, 0.25)"; - if (isDropdownLayout) { return ` font-family: Helvetica Light, sans-serif; font-size: 0.875rem; ... `; - } - - // Match WalletConnection "flex" modal list styling (grid) - return ` - display: grid; - ... - `; };Then update the call site:
- customCSS={buildWalletListCss(true)} + customCSS={buildWalletListCss()}
158-173: Refine wallet not-installed detection logic.When
connect()returnsfalse, the code checks!window.cardano[walletName]to determine if the wallet is not installed. However,connect()can fail for other reasons (user rejection, network issues) even when the wallet is installed. The generic fallback message (lines 169-171) mitigates this, but the not-installed check might fire incorrectly if the wallet extension temporarily fails or is disabled.Consider moving the not-installed check before calling
connect():const onConnectWallet = async (walletName: string) => { setIsConnecting(true); setConnectionError(null); setPendingWallet(walletName); try { + // Check wallet availability before attempting connection + if (!window.cardano || !window.cardano[walletName]) { + showErrorOnce( + `${walletName} wallet is not installed. Please install it from the official website.`, + ); + return; + } + const success = await connect(walletName); if (success) { handleSuccessfulConnect(); } else { - if (!window.cardano || !window.cardano[walletName]) { - showErrorOnce( - `${walletName} wallet is not installed. Please install it from the official website.`, - ); - } else { setConnectionError( `Failed to connect to ${walletName}. Please try again.`, ); - } }
228-239: Empty connecting feedback element serves no purpose.Lines 235-236 render an empty
<div>when connecting. If this is intentional spacing, add a comment or use CSS margin instead. If connection feedback was intended, add the message.Remove the empty div or add meaningful feedback:
{isConnecting && pendingWallet && ( - <div className="mb-2 text-sm text-gray-700"></div> + <div className="mb-2 text-sm text-gray-700"> + Connecting to {pendingWallet}... + </div> )}src/components/SpinningBorderButton.tsx (1)
40-53: Consider adding adisabledprop and cleaner CSS variable typing.The render logic is functional, but consider these improvements:
- Add
disabled?: booleanprop for better button state management- The CSS custom property type assertion on Line 45 works but could be cleaner:
style={{ "--button-radius": radius, borderRadius: radius, cursor: "pointer", } as React.CSSProperties}Example implementation:
type SpinningBorderButtonProps = { children: React.ReactNode; className?: string; spin?: boolean; onClick?: () => void; radius?: string; useBorder?: boolean; + disabled?: boolean; }; const SpinningBorderButton = ({ children, className = "", spin = true, radius = "12px", useBorder = true, onClick, + disabled = false, }: SpinningBorderButtonProps) => { // ... useEffect code ... return ( <button ref={ref} className={`${useBorder ? "border-gradient-button" : ""} ${className}`.trim()} style={{ - ["--button-radius" as string]: radius, + "--button-radius": radius, borderRadius: radius, cursor: "pointer", - }} + } as React.CSSProperties} onClick={onClick} + disabled={disabled} > {children} </button> ); };src/pages/Account.tsx (5)
448-450: Remove unnecessary type assertion and verify fallback duration logic.Line 450 contains an unnecessary type assertion
as numbersinceMath.max()already returns a number. More importantly, the fallback calculationMath.max(expirationTime - now.getTime(), 0)computes remaining time rather than original duration. For expired instances, this yields 0, which may not represent the actual purchase duration. Consider storing the original duration value more reliably or document this behavior if intentional.Apply this diff to remove the unnecessary type assertion:
const normalizedDurationMs = normalizeDurationMs(client.duration) || - Math.max(expirationTime - now.getTime(), 0) as number; + Math.max(expirationTime - now.getTime(), 0);
484-487: Consider exposing sort options to users.The VPN instances are always sorted using the hardcoded
filterOptions[0].value("default"). While this may be intentional for Phase 2, consider adding UI controls to let users choose different sort options (e.g., "most recent", "longest duration") as defined infilterOptions.
491-492: Consider using.some()for clarity.Line 492 uses
.findIndex() !== -1to check for existence. Using.some()would be more idiomatic and clearer:const hasActiveInstance = - vpnInstances.findIndex((instance) => instance.status === "Active") !== -1; + vpnInstances.some((instance) => instance.status === "Active");
620-664: Consider extracting the mobile duration selector into a reusable component.The sliding selector UI (lines 621-646) is well-implemented but adds significant complexity to the render function. Extracting it into a separate component (e.g.,
DurationSelector) would improve maintainability, enable isolated testing, and make the Account component easier to follow.
685-694: Minor duplication in skeleton loading states.The mobile and desktop skeleton loaders have similar structure. While the different layouts may justify the duplication here, consider whether a shared skeleton component could reduce repetition if more skeleton states are added in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/App.tsx(1 hunks)src/components/Navigation.tsx(2 hunks)src/components/PurchaseCard.tsx(1 hunks)src/components/SpinningBorderButton.tsx(1 hunks)src/components/VpnInstance.tsx(8 hunks)src/components/WalletPickerModal.tsx(1 hunks)src/index.css(2 hunks)src/pages/Account.tsx(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/Navigation.tsx
- src/App.tsx
- src/components/PurchaseCard.tsx
- src/index.css
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/Account.tsx (6)
src/api/hooks/index.ts (2)
useSignup(2-2)useRenewVpn(8-8)src/api/hooks/useSignup.ts (1)
useSignup(5-13)src/api/hooks/useRenewVpn.ts (1)
useRenewVpn(5-12)src/utils/pendingTransactions.ts (2)
addPendingTransaction(31-59)getPendingTransactions(14-25)src/api/types.ts (1)
ClientInfo(45-50)src/utils/instanceSort.ts (3)
sortVpnInstances(17-63)filterOptions(65-70)SortOption(1-5)
⏰ 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). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (20)
src/components/WalletPickerModal.tsx (2)
241-273: Well-structured wallet picker modal.The component properly integrates
ConfirmModalandConnectWalletListwith comprehensive error handling for installation, rate limiting, and network mismatches. The throttled error logging vialastErrorRefprevents duplicate messages.
147-151: No changes needed — "/account" is a valid route with no redirect configured.The routing configuration shows "/account" remains a legitimate route pointing to the Account component. There is no redirect from "/account" to "/" in the codebase, so navigating to "/account" (line 150) is correct and does not need modification.
Likely an incorrect or invalid review comment.
src/components/SpinningBorderButton.tsx (2)
1-10: LGTM! Props interface is well-structured.The imports and type definitions are correct. The props interface provides good flexibility with sensible optional parameters.
21-38: LGTM! Animation logic is correctly implemented.The
useEffectproperly manages therequestAnimationFrameloop lifecycle. The cleanup function correctly cancels the animation when the component unmounts or whenspinchanges. The optional chaining onref.currentsafely guards against edge cases.src/components/VpnInstance.tsx (4)
1-1: LGTM! Clean integration of SpinningBorderButton.The import and new
shouldSpinRenewprop are properly implemented with a sensible default value.Also applies to: 19-19, 33-33
51-63: LGTM! Typography and layout improvements enhance mobile usability.The updated label structure with bold values, smaller responsive text sizes, and the grid column adjustment from 4 to 3 (md:4) align well with the PR's mobile usability objectives.
Also applies to: 80-81
122-122: LGTM! Consistent typography updates.The button text size adjustments maintain consistency with the rest of the component's responsive typography.
Also applies to: 133-133, 141-141
51-56: Remove unnecessary region validation; instead standardize formatting with RegionSelect component.The current formatting logic that only uppercases the first 2 characters is inconsistent with how regions are displayed elsewhere. Region codes from AWS follow ISO 3166 standards and are always at least 2 characters, making the edge case handling unnecessary. More importantly, RegionSelect displays regions as
region.toUpperCase(), while VpnInstance usesregion.slice(0, 2).toUpperCase() + region.slice(2), creating inconsistent formatting (e.g., "US-EAST-1" vs "US-east-1"). Standardize to match the RegionSelect approach:{region ? region.toUpperCase() : ""}src/pages/Account.tsx (12)
1-29: LGTM!The new component imports (PurchaseCard, ConfirmModal, WalletConnection) align well with the modal-driven flow described in the PR objectives.
39-62: LGTM!The new state variables (
pendingTx,errorModal,showAdditionalPurchaseCards,selectedDurationIndex) properly support the modal-driven purchase and error handling flows.
112-114: LGTM!The mutations are correctly initialized without callbacks, as outcomes are now managed via
pendingTxstate and confirmation modals, aligning with the new modal-driven architecture.
188-200: LGTM!The
selectedDurationOptioncomputation and the bounds-checking effect properly handle dynamic changes todurationOptions, preventing index-out-of-bounds errors.
249-300: LGTM!The purchase flow correctly uses
selectedDurationOption(fixing the previous issue whereselectedOptionwas hardcoded) and properly implements the new modal-driven confirmation pattern withpendingTxstate and error handling viaerrorModal.
335-359: LGTM!The transaction submission flow correctly stores
pendingTx.durationMs(a number) into thedurationfield at line 344, addressing the previous type mismatch issue where a string label was stored instead.
366-417: LGTM!The renewal confirmation flow mirrors the purchase pattern, correctly building a
pendingTxobject for modal confirmation and routing errors toerrorModal.
527-547: LGTM!The
ConfirmModalimplementation for purchase/renewal confirmation is well-structured, displaying relevant transaction details and properly wiring confirm/cancel actions.
549-559: LGTM!The error modal correctly uses
ConfirmModalwithshowConfirm={false}to display errors as dismissible alerts, replacing the previous toast-based error handling.
778-800: LGTM!The VPN instance rendering logic properly handles all instance states (pending, active, expired) and correctly passes down all necessary props including renewal-related state and callbacks. The conditional
shouldSpinRenewprop elegantly highlights renewal opportunities when all instances are expired.
733-740: CSS classspinning-gradient-borderis properly defined.The class is defined in
src/index.csswith a complete implementation including a rotating gradient animation that runs for 6 seconds. The styling correctly applies the class whenareAllInstancesExpiredis true at line 735.
130-133: Clarify the duration unit contract forclient.durationor add defensive validation.The normalization threshold assumes values below 1 hour are in seconds. While VPN durations realistically start at days (well above the threshold), document this assumption or add explicit validation to prevent silent failures if the API contract changes or includes sub-hour durations in milliseconds.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/components/WalletConnection.tsx (2)
125-134: Modal state is local; store-based close operations won't affect it.The wallet modal open state is now managed locally (
localModalOpenat line 134) rather than viauseWalletStore().isWalletModalOpen. This means:
walletStore.closeWalletModal()calls from other components won't close this modalwalletStore.openWalletModal()won't open it eitherLooking at the store snippet,
openWalletModal()exists and is presumably called elsewhere. If other parts of the app expect centralized modal control, this breaks that contract.Options to resolve:
- Lift state to store (recommended if central control is needed):
- const [localModalOpen, setLocalModalOpen] = useState(initiallyOpen); + const { isWalletModalOpen, openWalletModal: setLocalModalOpen } = useWalletStore(); + + useEffect(() => { + if (initiallyOpen) setLocalModalOpen(); + }, [initiallyOpen, setLocalModalOpen]);
- Sync with store (if you must keep local state):
+ const { isWalletModalOpen: storeModalOpen } = useWalletStore(); + + useEffect(() => { + if (!storeModalOpen && localModalOpen) { + setLocalModalOpen(false); + } + }, [storeModalOpen, localModalOpen]);
- Remove store methods if no central control is needed (update store and remove
openWalletModal,closeWalletModal,toggleWalletModal).Based on learnings from past reviews, this was already flagged.
278-280: Empty div provides no connection feedback.Line 279 renders an empty
<div className={statusClasses}></div>when connecting, leaving users without visual feedback. The previous "Connecting to {pendingWallet}..." message was removed.Apply this diff to restore connection feedback:
{isConnecting && pendingWallet && ( - <div className={statusClasses}></div> + <p className={`${statusClasses} animate-pulse`}>Connecting to {pendingWallet}...</p> )}Based on learnings from past reviews, this issue was already identified.
🧹 Nitpick comments (5)
src/components/WalletConnection.tsx (3)
28-109: Consider extracting theme constants and splitting layout logic.The
buildWalletListCssfunction is 80+ lines and mixes theme color definitions with layout CSS generation. While functional, splitting this into separate helpers (e.g.,getThemeColors,buildDropdownCss,buildGridCss) would improve readability and testability.Example refactor:
const getThemeColors = (isLightTheme: boolean) => ({ textColor: isLightTheme ? "#0f172a" : "#ffffff", borderColor: isLightTheme ? "rgba(15, 23, 42, 0.15)" : "rgba(255, 255, 255, 0.2)", hoverBg: isLightTheme ? "rgba(15, 23, 42, 0.06)" : "rgba(255, 255, 255, 0.1)", hoverBorder: isLightTheme ? "rgba(15, 23, 42, 0.25)" : "rgba(255, 255, 255, 0.3)", }); const buildWalletListCss = ( isDropdownLayout: boolean, isLightTheme: boolean, fullWidth = false, ) => { const colors = getThemeColors(isLightTheme); return isDropdownLayout ? buildDropdownCss(colors) : buildGridCss(colors, fullWidth); };
163-166: Verify that syncinglocalModalOpenwithinitiallyOpenmatches intended UX.This effect will close the modal if
initiallyOpenchanges fromtruetofalseafter mount. IfinitiallyOpenis meant to only control initial state (as the name suggests), consider removing this effect or checking if it's the first render:const isFirstRender = useRef(true); useEffect(() => { if (isFirstRender.current) { setLocalModalOpen(initiallyOpen); isFirstRender.current = false; } }, [initiallyOpen]);
332-344: Modal renders even when connected and closed.Line 341 renders
{renderWalletModal()}even when the user is connected and the modal is closed (localModalOpenis false). WhileConfirmModallikely handles this gracefully by renderingnull, it adds a component to the tree unnecessarily.Consider rendering the modal only when needed:
<button onClick={disconnect} className={buttonClasses} > Disconnect </button> - {renderWalletModal()} + {localModalOpen && renderWalletModal()} </div> ); }Or keep it if you need the modal to be mount-persisted for animation reasons.
src/components/Navigation.tsx (2)
31-33: Menu-close-on-navigation uses cleanup pattern.The effect
useEffect(() => { return () => setIsMenuOpen(false); }, [location.pathname]);closes the mobile menu whenever the route changes. While functionally correct, using the cleanup function for side effects (rather than cleanup) is slightly unconventional.More idiomatic alternative:
useEffect(() => { - return () => setIsMenuOpen(false); - }, [location.pathname]); + setIsMenuOpen(false); + }, [location.pathname]);This makes the intent clearer: "when pathname changes, close the menu."
78-86: Hamburger button has good accessibility.The hamburger button includes proper ARIA attributes (
aria-label,aria-expanded) for screen readers. The implementation is solid.Minor enhancement: The
<img>alt text "Open menu" could be empty (alt="") since the button already hasaria-label="Toggle navigation menu", avoiding redundancy:- <img src="/hamburger.svg" alt="Open menu" className="w-7 h-7" /> + <img src="/hamburger.svg" alt="" className="w-7 h-7" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ConfirmModal.tsx(1 hunks)src/components/Navigation.tsx(2 hunks)src/components/WalletConnection.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ConfirmModal.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/WalletConnection.tsx (1)
src/stores/walletStore.ts (1)
useWalletStore(72-352)
⏰ 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). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
src/components/WalletConnection.tsx (2)
240-248: Good addition for rate-limiting UX.The enhanced error handling for wallet rate-limiting provides clear, actionable guidance (wait 30-60s, close other tabs, restart extension). This is a significant UX improvement.
299-320: Modal-based wallet selection improves UX.The
renderWalletModalimplementation provides a cleaner, more accessible wallet selection flow compared to the previous dropdown approach. Using a light theme in the modal (renderWalletList(true, true)) ensures good contrast and readability.src/components/Navigation.tsx (3)
2-2: Clean mobile menu state management.The
isMenuOpenstate and helper functions (toggleMenu,closeMenu) provide a straightforward implementation for mobile navigation control.Also applies to: 8-8, 35-36
16-29: Resize handler properly closes mobile menu on desktop.The resize listener correctly closes the mobile menu when transitioning to desktop viewport, preventing orphaned menu state. Cleanup is properly implemented.
87-120: Mobile menu panel is well-implemented.The mobile dropdown menu includes:
- Proper positioning and styling with backdrop blur
- All navigation links close the menu on click (good UX)
- Consistent hover states
- Responsive design with proper breakpoint handling
|
@cubic-dev-ai review this PR |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 26 files
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/components/SpinningBorderButton.tsx">
<violation number="1" location="src/components/SpinningBorderButton.tsx:41">
P2: Button is missing explicit `type="button"` attribute. Without it, the button defaults to `type="submit"`, which can cause unexpected form submissions if used inside a `<form>` element.</violation>
</file>
<file name="src/components/ConfirmModal.tsx">
<violation number="1" location="src/components/ConfirmModal.tsx:88">
P2: Inline `transform` style overrides Tailwind `translate-y-*` classes, breaking responsive behavior. The `md:translate-y-0` class intended to disable slide animation on desktop won't work. Consider conditionally applying the inline transform only on mobile, or removing the dead Tailwind classes.</violation>
</file>
<file name="src/components/WalletConnection.tsx">
<violation number="1" location="src/components/WalletConnection.tsx:279">
P2: Empty div renders when connecting - the "Connecting to {walletName}..." status message was removed. Users no longer see feedback about which wallet connection is in progress.</violation>
</file>
<file name="src/stores/walletStore.ts">
<violation number="1" location="src/stores/walletStore.ts:105">
P2: Network mismatch errors are now only logged to console. Users won't understand why their wallet connection failed unless they check developer tools. Consider implementing an alternative feedback mechanism (modal, inline error, etc.) to inform users they need to switch networks.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| }, [spin]); | ||
|
|
||
| return ( | ||
| <button |
There was a problem hiding this comment.
P2: Button is missing explicit type="button" attribute. Without it, the button defaults to type="submit", which can cause unexpected form submissions if used inside a <form> element.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/SpinningBorderButton.tsx, line 41:
<comment>Button is missing explicit `type="button"` attribute. Without it, the button defaults to `type="submit"`, which can cause unexpected form submissions if used inside a `<form>` element.</comment>
<file context>
@@ -0,0 +1,57 @@
+ }, [spin]);
+
+ return (
+ <button
+ ref={ref}
+ className={`${useBorder ? "border-gradient-button" : ""} ${className}`.trim()}
</file context>
✅ Addressed in fea25ba
| className={`${ | ||
| isVisible ? "translate-y-0" : "translate-y-full" | ||
| } md:translate-y-0 absolute bottom-0 left-0 right-0 md:relative md:bottom-auto w-full md:max-w-lg bg-white text-slate-900 rounded-t-2xl md:rounded-xl shadow-2xl transition-transform duration-300 ease-out transform overflow-hidden pb-[env(safe-area-inset-bottom)] max-h-[90vh] min-h-[70vh] md:min-h-0 md:max-h-none flex flex-col`} | ||
| style={{ transform: transformValue }} |
There was a problem hiding this comment.
P2: Inline transform style overrides Tailwind translate-y-* classes, breaking responsive behavior. The md:translate-y-0 class intended to disable slide animation on desktop won't work. Consider conditionally applying the inline transform only on mobile, or removing the dead Tailwind classes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/ConfirmModal.tsx, line 88:
<comment>Inline `transform` style overrides Tailwind `translate-y-*` classes, breaking responsive behavior. The `md:translate-y-0` class intended to disable slide animation on desktop won't work. Consider conditionally applying the inline transform only on mobile, or removing the dead Tailwind classes.</comment>
<file context>
@@ -0,0 +1,131 @@
+ className={`${
+ isVisible ? "translate-y-0" : "translate-y-full"
+ } md:translate-y-0 absolute bottom-0 left-0 right-0 md:relative md:bottom-auto w-full md:max-w-lg bg-white text-slate-900 rounded-t-2xl md:rounded-xl shadow-2xl transition-transform duration-300 ease-out transform overflow-hidden pb-[env(safe-area-inset-bottom)] max-h-[90vh] min-h-[70vh] md:min-h-0 md:max-h-none flex flex-col`}
+ style={{ transform: transformValue }}
+ onTouchStart={(e) => handleDragStart(e.touches[0]?.clientY)}
+ onTouchMove={(e) => handleDragMove(e.touches[0]?.clientY)}
</file context>
| )} | ||
| {isConnecting && pendingWallet && ( | ||
| <p className={`${statusClasses} animate-pulse`}>Connecting to {pendingWallet}...</p> | ||
| <div className={statusClasses}></div> |
There was a problem hiding this comment.
P2: Empty div renders when connecting - the "Connecting to {walletName}..." status message was removed. Users no longer see feedback about which wallet connection is in progress.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/WalletConnection.tsx, line 279:
<comment>Empty div renders when connecting - the "Connecting to {walletName}..." status message was removed. Users no longer see feedback about which wallet connection is in progress.</comment>
<file context>
@@ -264,42 +265,23 @@ const WalletConnection = ({
)}
{isConnecting && pendingWallet && (
- <p className={`${statusClasses} animate-pulse`}>Connecting to {pendingWallet}...</p>
+ <div className={statusClasses}></div>
)}
</>
</file context>
| <div className={statusClasses}></div> | |
| <p className={`${statusClasses} animate-pulse`}>Connecting to {pendingWallet}...</p> |
| const walletNetworkLabel = formatWalletNetworkLabel(walletNetworkId); | ||
| showError( | ||
| `Wallet network mismatch. This app is configured for ${APP_NETWORK_LABEL}, but your wallet is connected to ${walletNetworkLabel}. Please switch networks in your wallet and try again.`, | ||
| console.error( |
There was a problem hiding this comment.
P2: Network mismatch errors are now only logged to console. Users won't understand why their wallet connection failed unless they check developer tools. Consider implementing an alternative feedback mechanism (modal, inline error, etc.) to inform users they need to switch networks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/stores/walletStore.ts, line 105:
<comment>Network mismatch errors are now only logged to console. Users won't understand why their wallet connection failed unless they check developer tools. Consider implementing an alternative feedback mechanism (modal, inline error, etc.) to inform users they need to switch networks.</comment>
<file context>
@@ -98,8 +102,8 @@ export const useWalletStore = create<WalletState>()(
const walletNetworkLabel = formatWalletNetworkLabel(walletNetworkId);
- showError(
- `Wallet network mismatch. This app is configured for ${APP_NETWORK_LABEL}, but your wallet is connected to ${walletNetworkLabel}. Please switch networks in your wallet and try again.`,
+ console.error(
+ `Wallet network mismatch. App expects ${APP_NETWORK_LABEL}, but wallet is on ${walletNetworkLabel}.`,
);
</file context>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/WalletPickerModal.tsx (1)
229-231: Empty div renders no connection feedback.The empty div at line 230 provides no visual feedback to users during wallet connection. This is similar to the issue flagged in WalletConnection.tsx.
Suggested fix
{isConnecting && pendingWallet && ( - <div className="mb-2 text-sm text-gray-700"></div> + <p className="mb-2 text-sm text-gray-700 animate-pulse"> + Connecting to {pendingWallet}... + </p> )}src/components/WalletConnection.tsx (1)
273-275: Empty div renders no connection status.This issue was previously flagged by other reviewers but remains unresolved. The empty
divat line 274 should display connection status text like "Connecting to {pendingWallet}..." to provide user feedback.Suggested fix (as previously recommended)
{isConnecting && pendingWallet && ( - <div className={statusClasses}></div> + <p className={`${statusClasses} animate-pulse`}> + Connecting to {pendingWallet}... + </p> )}
🧹 Nitpick comments (2)
src/components/PurchaseCard.tsx (1)
22-79: LGTM! Well-structured purchase card component.The component correctly handles wallet connection state and purchase flow. Logic guards prevent actions during processing, and the UI provides clear feedback.
Optional: Enhance accessibility
Consider adding
aria-busyandaria-liveattributes to improve screen reader experience during processing:<button className={`mt-2 w-full rounded-full py-2 text-black font-semibold bg-white transition-all cursor-pointer ${ disabled ? "opacity-60 cursor-not-allowed" : "hover:scale-[1.01]" }`} onClick={() => { if (isProcessing) return; if (!isConnected) { openWalletModal(); return; } onPurchase(option.value); }} disabled={disabled} + aria-busy={isProcessing} + aria-live="polite" {...(showTooltips && highlightPurchase && { "data-tooltip-id": "purchase-tooltip" })} > {isProcessing ? "Processing..." : "Buy Now"} </button>src/pages/Account.tsx (1)
124-127: Consider explicit duration units over heuristic normalization.The
normalizeDurationMsfunction uses a threshold-based heuristic (< 1 hour = seconds, >= 1 hour = milliseconds) to normalize durations. While this likely works for current use cases, it's fragile and could break if duration ranges change.Recommendation
If possible, ensure the API returns consistent units (e.g., always milliseconds) or include an explicit unit field. Alternatively, document the expected input format clearly and consider renaming to
normalizeSecondsToMsif the function specifically handles the seconds-to-milliseconds conversion for values under 1 hour.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/PurchaseCard.tsx(1 hunks)src/components/TooltipGuide.tsx(1 hunks)src/components/WalletConnection.tsx(7 hunks)src/components/WalletPickerModal.tsx(1 hunks)src/pages/Account.tsx(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/components/WalletPickerModal.tsx (1)
src/stores/walletStore.ts (1)
useWalletStore(72-352)
src/components/PurchaseCard.tsx (1)
src/stores/walletStore.ts (1)
useWalletStore(72-352)
src/pages/Account.tsx (5)
src/api/hooks/useSignup.ts (1)
useSignup(5-13)src/api/hooks/useRenewVpn.ts (1)
useRenewVpn(5-12)src/utils/pendingTransactions.ts (2)
addPendingTransaction(31-59)getPendingTransactions(14-25)src/api/types.ts (1)
ClientInfo(45-50)src/utils/instanceSort.ts (3)
sortVpnInstances(17-63)filterOptions(65-70)SortOption(1-5)
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (2)
src/pages/Account.tsx (2)
197-197: Verify single-region default is intentional.The code always uses the first region (
regions[0]) without user selection. Confirm this is the intended behavior, especially if multiple regions are available inrefData.regions.If multiple regions should be supported, consider adding region selection UI or documenting why the first region is always used.
243-294: Well-structured purchase flow with two-step confirmation.The purchase flow correctly:
- Builds the transaction and validates inputs
- Shows a confirmation modal with transaction details
- Only signs/submits after user confirmation
- Handles errors gracefully via the error modal
- Adds to pending transactions only after successful submission
Also applies to: 329-358
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/VpnInstance.tsx (1)
41-47: Replace non-standardbackdrop-blur-xsutility class.The utility class
backdrop-blur-xsis not a standard Tailwind utility. Standard Tailwind v4 providesbackdrop-blur-smas the smallest blur variant. This non-standard class will not apply the intended blur effect.🔎 Recommended fix
Replace all instances of
backdrop-blur-xswithbackdrop-blur-sm:- className={`flex p-4 flex-col justify-center items-start gap-3 w-full rounded-md backdrop-blur-xs ${ + className={`flex p-4 flex-col justify-center items-start gap-3 w-full rounded-md backdrop-blur-sm ${- className="flex items-center justify-center gap-3 py-1.5 px-3.5 backdrop-blur-xs box-shadow-sm bg-white text-black hover:bg-white/90 transition-all" + className="flex items-center justify-center gap-3 py-1.5 px-3.5 backdrop-blur-sm shadow-sm bg-white text-black hover:bg-white/90 transition-all"- className="flex items-center justify-center gap-3 rounded-md py-1.5 px-3.5 backdrop-blur-xs box-shadow-sm cursor-pointer bg-white/50 text-white hover:bg-white/70 transition-all flex-1" + className="flex items-center justify-center gap-3 rounded-md py-1.5 px-3.5 backdrop-blur-sm shadow-sm cursor-pointer bg-white/50 text-white hover:bg-white/70 transition-all flex-1"- className={`flex items-center justify-center gap-3 rounded-md py-1.5 px-3.5 backdrop-blur-xs box-shadow-sm transition-all flex-1 ${ + className={`flex items-center justify-center gap-3 rounded-md py-1.5 px-3.5 backdrop-blur-sm shadow-sm transition-all flex-1 ${- <div className="flex items-center justify-center gap-2 rounded-md py-1.5 px-3.5 backdrop-blur-xs bg-gray-400 text-gray-600"> + <div className="flex items-center justify-center gap-2 rounded-md py-1.5 px-3.5 backdrop-blur-sm bg-gray-400 text-gray-600">Also applies to: 107-107, 119-119, 125-125, 139-139
♻️ Duplicate comments (1)
src/components/VpnInstance.tsx (1)
107-107: Replace non-standardbox-shadow-smutility class.As noted in the previous review,
box-shadow-smis not a standard Tailwind utility. The correct utility isshadow-sm. This issue remains unaddressed across lines 107, 119, and 125.🔎 Recommended fix
Replace
box-shadow-smwithshadow-smin all three locations:- className="flex items-center justify-center gap-3 py-1.5 px-3.5 backdrop-blur-xs box-shadow-sm bg-white text-black hover:bg-white/90 transition-all" + className="flex items-center justify-center gap-3 py-1.5 px-3.5 backdrop-blur-sm shadow-sm bg-white text-black hover:bg-white/90 transition-all"- className="flex items-center justify-center gap-3 rounded-md py-1.5 px-3.5 backdrop-blur-xs box-shadow-sm cursor-pointer bg-white/50 text-white hover:bg-white/70 transition-all flex-1" + className="flex items-center justify-center gap-3 rounded-md py-1.5 px-3.5 backdrop-blur-sm shadow-sm cursor-pointer bg-white/50 text-white hover:bg-white/70 transition-all flex-1"- className={`flex items-center justify-center gap-3 rounded-md py-1.5 px-3.5 backdrop-blur-xs box-shadow-sm transition-all flex-1 ${ + className={`flex items-center justify-center gap-3 rounded-md py-1.5 px-3.5 backdrop-blur-sm shadow-sm transition-all flex-1 ${Also applies to: 119-119, 125-125
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/VpnInstance.tsx(8 hunks)
⏰ 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). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
src/components/VpnInstance.tsx (4)
1-1: LGTM!The import of
SpinningBorderButtonis appropriate for the new button component introduced in this PR.
19-19: LGTM!The
shouldSpinRenewprop is well-typed with a sensible default value and correctly controls the spinning behavior of the SpinningBorderButton.Also applies to: 33-33
51-59: LGTM!The typography updates and responsive grid changes improve mobile usability as intended. The consistent use of
text-xs md:text-smensures good readability across screen sizes, and the responsive grid (grid-cols-2 sm:grid-cols-3 md:grid-cols-4) provides better layout on smaller devices.Also applies to: 63-63, 80-81, 122-122, 133-133, 141-141
103-113: LGTM!The integration of
SpinningBorderButtonis well-implemented with appropriate conditional logic. The button only spins whenshouldSpinRenewis true AND the status is "Expired", providing clear visual feedback during renewal operations.
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/components/VpnInstance.tsx (1)
41-41: Replace non-standardbackdrop-blur-xswithbackdrop-blur-sm.The
backdrop-blur-xsutility doesn't exist in standard Tailwind CSS. The smallest standard backdrop-blur utility isbackdrop-blur-sm. This non-standard utility appears on lines 41, 107, 119, 125, and 139.🔎 Proposed fix for all occurrences
- className={`flex p-4 flex-col justify-center items-start gap-3 w-full rounded-md backdrop-blur-xs ${ + className={`flex p-4 flex-col justify-center items-start gap-3 w-full rounded-md backdrop-blur-sm ${ - className="flex items-center justify-center gap-3 py-1.5 px-3.5 backdrop-blur-xs shadow-sm bg-white text-black hover:bg-white/90 transition-all" + className="flex items-center justify-center gap-3 py-1.5 px-3.5 backdrop-blur-sm shadow-sm bg-white text-black hover:bg-white/90 transition-all" - className="flex items-center justify-center gap-3 rounded-md py-1.5 px-3.5 backdrop-blur-xs shadow-sm cursor-pointer bg-white/50 text-white hover:bg-white/70 transition-all flex-1" + className="flex items-center justify-center gap-3 rounded-md py-1.5 px-3.5 backdrop-blur-sm shadow-sm cursor-pointer bg-white/50 text-white hover:bg-white/70 transition-all flex-1" - className={`flex items-center justify-center gap-3 rounded-md py-1.5 px-3.5 backdrop-blur-xs shadow-sm transition-all flex-1 ${ + className={`flex items-center justify-center gap-3 rounded-md py-1.5 px-3.5 backdrop-blur-sm shadow-sm transition-all flex-1 ${ - <div className="flex items-center justify-center gap-2 rounded-md py-1.5 px-3.5 backdrop-blur-xs bg-gray-400 text-gray-600"> + <div className="flex items-center justify-center gap-2 rounded-md py-1.5 px-3.5 backdrop-blur-sm bg-gray-400 text-gray-600">Also applies to: 107-107, 119-119, 125-125, 139-139
♻️ Duplicate comments (1)
src/components/WalletConnection.tsx (1)
269-283: Critical: Empty div provides no user feedback during wallet connection.Line 280 renders an empty
<div>when connecting, removing the previous "Connecting to {pendingWallet}..." status message. Users receive no visual feedback about which wallet connection is in progress.🔎 Restore the connection status message
{isConnecting && pendingWallet && ( - <div className={statusClasses}></div> + <p className={`${statusClasses} animate-pulse`}> + Connecting to {pendingWallet}... + </p> )}
🧹 Nitpick comments (1)
src/components/WalletConnection.tsx (1)
28-104: Consider migrating inline CSS strings to Tailwind classes or CSS modules.The
buildWalletListCssfunction generates CSS via template strings, which can be harder to maintain in a Tailwind-based codebase. Consider refactoring to use conditional Tailwind classes or CSS modules for better type safety, editor support, and consistency with the rest of the project.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/SpinningBorderButton.tsx(1 hunks)src/components/TooltipGuide.tsx(2 hunks)src/components/VpnInstance.tsx(6 hunks)src/components/WalletConnection.tsx(7 hunks)src/pages/Account.tsx(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/SpinningBorderButton.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/Account.tsx (5)
src/api/hooks/useSignup.ts (1)
useSignup(5-13)src/api/hooks/useRenewVpn.ts (1)
useRenewVpn(5-12)src/utils/pendingTransactions.ts (2)
addPendingTransaction(31-59)getPendingTransactions(14-25)src/api/types.ts (1)
ClientInfo(45-50)src/utils/instanceSort.ts (3)
sortVpnInstances(17-63)filterOptions(65-70)SortOption(1-5)
src/components/WalletConnection.tsx (1)
src/stores/walletStore.ts (1)
useWalletStore(72-352)
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (19)
src/components/TooltipGuide.tsx (1)
98-98: Good implementation:anchorSelectcorrectly links tooltips to anchors.The
anchorSelectprop with CSS attribute selector is the correct pattern for react-tooltip v5.29.1. Combined with the validation effect (lines 52-63), this ensures tooltips anchor properly and warns developers when data-tooltip-id attributes are missing.src/components/VpnInstance.tsx (6)
19-19: LGTM! Optional prop with sensible default.The
shouldSpinRenewprop is well-defined with an appropriate optional boolean type and default value.
51-56: LGTM! Region formatting logic is safe.The ternary guard properly handles falsy values, and the slice operations work correctly for all realistic region string lengths.
81-81: LGTM! Responsive grid layout.The responsive grid structure (2/3/4 columns) provides good mobile-first UX for the renewal duration options.
103-113: LGTM! Conditional spinning logic is appropriate.The conditional logic for
spinanduseBorderprops (active only whenshouldSpinRenewis true AND status is "Expired") is well-reasoned and provides good UX feedback.
51-51: LGTM! Consistent responsive typography.The responsive text sizing pattern (
text-xs md:text-sm) is consistently applied throughout and aligns with the PR's focus on mobile usability.Also applies to: 57-57, 63-63, 80-80, 110-110, 122-122, 133-133, 141-141
1-1: No issues found. The SpinningBorderButton component exists and supports all the props being used in VpnInstance.tsx (spin,useBorder,radius,className,onClick).src/components/WalletConnection.tsx (5)
160-166: LGTM - Modal synchronization logic is sound.The effect correctly syncs the wallet modal state with the
initiallyOpenprop. Zustand store actions are stable references, so this won't cause unnecessary re-renders.
241-248: Excellent rate-limiting detection and user guidance.The expanded error handling for rate-limiting scenarios provides clear, actionable instructions to users. This significantly improves the connection experience.
310-331: Verify hardcoded light theme in wallet modal.Line 323 forces
renderWalletList(true, true, "grid")with a hardcoded light theme, while the component accepts athemeprop for other UI elements. Confirm whether the wallet modal should always use a light theme or respect the component's theme prop for consistency.
343-355: Wallet modal rendered when connected enables wallet switching.The
renderWalletModal()call at line 352 makes the wallet selection modal available even when a wallet is already connected. This appears intentional to support wallet switching, though the modal won't be visible unless explicitly opened via the store'sisWalletModalOpenstate.
416-424: LGTM - Dropdown layout implementation is clean.The simplified dropdown layout correctly renders the connect button and modal.
src/pages/Account.tsx (7)
243-294: Purchase flow correctly implements modal-based confirmation.The function properly uses
selectedDurationOption(addressing previous review feedback), builds the transaction payload, and stores both numeric and formatted duration values inpendingTxfor the confirmation modal. Error handling is appropriate.
329-359: Transaction submission flow is well-structured.The confirmation handler correctly stores numeric duration values in the pending transaction (line 340), addressing previous type mismatch issues. The modal closes immediately while processing continues, providing good UX. Error handling is appropriate.
361-412: Renewal flow mirrors purchase flow consistently.The renewal confirmation handler follows the same modal-based pattern as purchases, maintaining consistency across the UX. Error handling and state management are appropriate.
436-483: VPN instances computation correctly handles active and pending states.The instance computation properly merges active clients from the API with pending transactions from localStorage, deduplicates by ID, and sorts the combined list. The formatting and state determination logic is sound.
609-695: Excellent responsive purchase card implementation.The mobile and desktop layouts are well-differentiated: mobile uses a sliding tab selector with animated transitions, while desktop displays all options simultaneously. Loading states are properly handled for both layouts. The implementation demonstrates thoughtful UX design for different screen sizes.
522-554: Modal-based confirmation flow improves UX clarity.Replacing toast notifications with dedicated confirmation and error modals provides clearer user feedback and explicit confirmation steps. The pendingTx modal displays transaction details before submission, and the error modal handles failures gracefully.
698-814: Instances section handles all states comprehensively.The rendering logic appropriately handles loading, empty, active, and expired states with clear visual feedback. The "+ Add New" button with spinning gradient for expired instances is a nice touch to encourage renewal. Loading skeletons maintain layout stability during data fetches.
| const normalizeDurationMs = (duration?: number) => { | ||
| if (!duration) return 0; | ||
| return duration < 1000 * 60 * 60 ? duration * 1000 : duration; | ||
| }; |
There was a problem hiding this comment.
Problematic threshold in duration normalization logic.
The normalizeDurationMs function uses a threshold of 1 hour (3,600,000 ms) to determine if a duration value is in seconds vs. milliseconds. This creates incorrect conversions for any duration between 1000 and 3,600,000 milliseconds (1 second to 1 hour).
Example: A 30-minute duration stored as 1,800,000 ms would be detected as "less than 1 hour", then multiplied by 1000 to become 1,800,000,000 ms (≈ 500 hours).
🔎 Use a more reliable threshold
const normalizeDurationMs = (duration?: number) => {
if (!duration) return 0;
- return duration < 1000 * 60 * 60 ? duration * 1000 : duration;
+ // If duration < 1000, assume it's in seconds and convert to ms
+ // Otherwise assume it's already in milliseconds
+ return duration < 1000 ? duration * 1000 : duration;
};Or if the API can return durations in seconds that are > 1000 seconds, use a day threshold:
const normalizeDurationMs = (duration?: number) => {
if (!duration) return 0;
- return duration < 1000 * 60 * 60 ? duration * 1000 : duration;
+ // If duration < 86400 (1 day in seconds), assume seconds; else milliseconds
+ return duration < 86400 ? duration * 1000 : duration;
};📝 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 normalizeDurationMs = (duration?: number) => { | |
| if (!duration) return 0; | |
| return duration < 1000 * 60 * 60 ? duration * 1000 : duration; | |
| }; | |
| const normalizeDurationMs = (duration?: number) => { | |
| if (!duration) return 0; | |
| // If duration < 1000, assume it's in seconds and convert to ms | |
| // Otherwise assume it's already in milliseconds | |
| return duration < 1000 ? duration * 1000 : duration; | |
| }; |
| const normalizeDurationMs = (duration?: number) => { | |
| if (!duration) return 0; | |
| return duration < 1000 * 60 * 60 ? duration * 1000 : duration; | |
| }; | |
| const normalizeDurationMs = (duration?: number) => { | |
| if (!duration) return 0; | |
| // If duration < 86400 (1 day in seconds), assume seconds; else milliseconds | |
| return duration < 86400 ? duration * 1000 : duration; | |
| }; |
🤖 Prompt for AI Agents
In src/pages/Account.tsx around lines 124-127, the normalizeDurationMs function
uses a 1‑hour threshold which wrongly treats many millisecond values as seconds;
change the logic to assume values < 1000 are seconds (multiply by 1000) and
values >= 1000 are already milliseconds, or if the API can return very large
second counts, use a day threshold (e.g. values < 86400 treated as seconds) –
implement one of these thresholds and update the function accordingly so
durations are normalized reliably to milliseconds.
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all 1 issue)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/components/WalletConnection.tsx">
<violation number="1" location="src/components/WalletConnection.tsx:163">
P1: This effect will close the global wallet modal when `initiallyOpen` is `false` (the default). Since multiple `WalletConnection` components exist in the app, any of them mounting or re-rendering will call `closeWalletModal()`, potentially closing a modal that was opened by user interaction or another component. Consider only opening the modal when `initiallyOpen` becomes `true`, without closing it when `false`.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
src/components/WalletConnection.tsx
Outdated
| useEffect(() => { | ||
| if (initiallyOpen) { | ||
| openWalletModal(); | ||
| } else { |
There was a problem hiding this comment.
P1: This effect will close the global wallet modal when initiallyOpen is false (the default). Since multiple WalletConnection components exist in the app, any of them mounting or re-rendering will call closeWalletModal(), potentially closing a modal that was opened by user interaction or another component. Consider only opening the modal when initiallyOpen becomes true, without closing it when false.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/WalletConnection.tsx, line 163:
<comment>This effect will close the global wallet modal when `initiallyOpen` is `false` (the default). Since multiple `WalletConnection` components exist in the app, any of them mounting or re-rendering will call `closeWalletModal()`, potentially closing a modal that was opened by user interaction or another component. Consider only opening the modal when `initiallyOpen` becomes `true`, without closing it when `false`.</comment>
<file context>
@@ -156,28 +158,32 @@ const WalletConnection = ({
- }, [initiallyOpen]);
+ if (initiallyOpen) {
+ openWalletModal();
+ } else {
+ closeWalletModal();
+ }
</file context>
✅ Addressed in 9df8026
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/components/WalletConnection.tsx (2)
167-173: Effect closes global modal wheninitiallyOpenis false.This effect will call
closeWalletModal()whenever anyWalletConnectioninstance mounts or re-renders withinitiallyOpen={false}(the default). Since multiple instances exist across Navigation, Account, and WalletModal, this can close modals opened by user interaction in other components. Consider only opening the modal wheninitiallyOpenbecomestrue, without closing whenfalse.🔎 Suggested fix
useEffect(() => { if (initiallyOpen) { openWalletModal(); - } else { - closeWalletModal(); } - }, [initiallyOpen, openWalletModal, closeWalletModal]); + }, [initiallyOpen, openWalletModal]);
286-300: Empty div renders no connecting feedback.Line 297 renders an empty
<div className={statusClasses}></div>when connecting. The previous "Connecting to {pendingWallet}..." message was removed, leaving users without visual feedback during wallet connection.🔎 Restore connecting message
{isConnecting && pendingWallet && ( - <div className={statusClasses}></div> + <p className={`${statusClasses} animate-pulse`}>Connecting to {pendingWallet}...</p> )}
🧹 Nitpick comments (1)
src/components/WalletConnection.tsx (1)
28-104: Consider extracting CSS builder to a utility file.The
buildWalletListCssfunction is well-implemented and generates theme-aware styles correctly. However, at 76 lines, it comprises a significant portion of the component. Consider extracting it to a separate utility file (e.g.,utils/walletListStyles.ts) to improve component readability and testability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/WalletConnection.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/WalletConnection.tsx (1)
src/stores/walletStore.ts (1)
useWalletStore(72-352)
⏰ 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: cubic · AI code reviewer
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
src/components/WalletConnection.tsx (4)
147-165: Excellent error handling improvements.The
showErrorOncededuplication helper andisUserCanceledErrordetection are valuable additions that improve the user experience by reducing noise and providing clearer feedback during wallet connection flows.
202-240: Improved connection error handling.The enhanced error handling in
onConnectWalletprovides better user feedback by distinguishing between wallet-not-installed, user cancellation, and other connection failures. The error messages are actionable and help users resolve issues.
242-284: Excellent rate-limit detection and guidance.The addition of rate-limit detection (lines 258-265) is particularly valuable. The error message provides specific, actionable steps: waiting 30-60 seconds, closing other dApp tabs, and restarting the wallet extension. This addresses a common user pain point with wallet extensions and significantly improves the UX.
15-15: Clean theme prop implementation.The
themeprop is well-integrated throughout the component. Theme-aware classes are computed once and used consistently for headings, subtext, and status messages. The implementation provides good flexibility for embedding this component in different contexts.Also applies to: 118-118, 140-145
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/components/WalletConnection.tsx (1)
295-297: Empty div renders no content when connecting.The connecting state feedback was removed, leaving users with no visual indication that a wallet connection is in progress. The empty
<div>should display the pending wallet name or a spinner.🔎 Proposed fix
{isConnecting && pendingWallet && ( - <div className={statusClasses}></div> + <p className={`${statusClasses} animate-pulse`}>Connecting to {pendingWallet}...</p> )}src/pages/Account.tsx (1)
124-127: **Problematic threshold in duration normalization logic.**The threshold logic assumes durations under 1 hour are in seconds and need to be multiplied by 1000. However, this creates incorrect conversions for any duration between 1000 ms and 3,600,000 ms. For example, a 30-minute duration stored as 1,800,000 ms would become 1,800,000,000 ms (~500 hours).🔎 Proposed fix: Use a more reliable threshold
const normalizeDurationMs = (duration?: number) => { if (!duration) return 0; - return duration < 1000 * 60 * 60 ? duration * 1000 : duration; + // If duration < 1000, assume it's in seconds and convert to ms + // Otherwise assume it's already in milliseconds + return duration < 1000 ? duration * 1000 : duration; };Alternatively, if the API can return durations in seconds that are > 1000 seconds:
const normalizeDurationMs = (duration?: number) => { if (!duration) return 0; - return duration < 1000 * 60 * 60 ? duration * 1000 : duration; + // If duration < 86400 (1 day in seconds), assume seconds; else ms + return duration < 86400 ? duration * 1000 : duration; };
🧹 Nitpick comments (2)
src/pages/Account.tsx (2)
403-412: Renewal state cleared even on error.Lines 410-411 clear
renewingInstanceIdandselectedRenewDurationregardless of success or failure. If the mutation fails, users will need to re-expand the renewal panel and re-select the duration. Consider moving these lines inside the try block if you want to preserve the selection on error.🔎 Proposed fix to preserve state on error
setPendingTx({ type: "renew", txCbor: data.txCbor, clientId: renewingInstanceId, durationMs: selectedRenewDuration, durationLabel: formatDuration(selectedRenewDuration), region: payloadRegion, }); + setRenewingInstanceId(null); + setSelectedRenewDuration(null); } catch (error) { console.error("Renew failed:", error); setErrorModal("Failed to build renewal transaction"); } finally { setIsPurchaseLoading(false); } - - setRenewingInstanceId(null); - setSelectedRenewDuration(null); };
443-445: Unnecessary type assertion.The
as numbercast on line 445 is redundant sinceMath.max()always returns a number. This can be safely removed.🔎 Proposed fix
const normalizedDurationMs = normalizeDurationMs(client.duration) || - Math.max(expirationTime - now.getTime(), 0) as number; + Math.max(expirationTime - now.getTime(), 0);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/TooltipGuide.tsx(3 hunks)src/components/WalletConnection.tsx(8 hunks)src/pages/Account.tsx(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/TooltipGuide.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/Account.tsx (6)
src/api/hooks/index.ts (2)
useSignup(2-2)useRenewVpn(8-8)src/api/hooks/useSignup.ts (1)
useSignup(5-13)src/api/hooks/useRenewVpn.ts (1)
useRenewVpn(5-12)src/utils/pendingTransactions.ts (2)
addPendingTransaction(31-59)getPendingTransactions(14-25)src/api/types.ts (1)
ClientInfo(45-50)src/utils/instanceSort.ts (3)
sortVpnInstances(17-63)filterOptions(65-70)SortOption(1-5)
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (13)
src/components/WalletConnection.tsx (5)
28-104: Well-structured dynamic CSS builder.The
buildWalletListCssfunction cleanly handles theme-aware styling for both dropdown and grid layouts. The logic is clear and the CSS variables are well-organized.
167-172: Good fix for the modal closing issue.The effect now correctly only opens the modal when
initiallyOpenis true, avoiding the previous bug where mounting components withinitiallyOpen=falsewould close modals opened by other components.
160-165: LGTM!The
isUserCanceledErrorhelper correctly identifies user cancellation by checking both error code and message.
326-347: LGTM!The
renderWalletModalhelper cleanly encapsulates the wallet selection modal with appropriate theming and structure.
359-371: Consider whether the wallet modal is needed when connected.The wallet modal is rendered even when the user is already connected. While it's controlled by
isWalletModalOpen, consider if this is intentional behavior or if the modal should only be available when disconnected.src/pages/Account.tsx (8)
39-47: Well-structured pending transaction state.The
pendingTxstate now correctly stores bothdurationMs(numeric) anddurationLabel(formatted string), addressing the previous type mismatch issue.
106-108: Good migration to mutateAsync pattern.Using
mutateAsyncwith try/catch allows the component to manage the pending transaction state before submission, which aligns well with the new modal-driven confirmation flow.
249-258: Good fix for duration selection.The code now correctly uses
selectedDurationOptionas the primary source for the target duration, addressing the previous issue where the selection index was ignored.
329-354: Well-structured confirmation handler.The function correctly captures
pendingTxbefore clearing the modal state, preventing race conditions. The pending transaction is properly stored and polling initiated.
518-538: LGTM!The confirmation modal appropriately displays transaction details and provides clear confirm/cancel actions.
540-550: LGTM!Error modal properly displays error messages and allows dismissal via either action.
610-654: Well-designed mobile duration selector.The sliding indicator animation is performant using CSS transforms. The responsive design cleanly separates mobile and desktop layouts.
805-806: LGTM!The
WalletConnectioncomponent is correctly integrated for the mobile disconnect state with appropriate props.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/components/ConfirmModal.tsx">
<violation number="1" location="src/components/ConfirmModal.tsx:39">
P2: Cleanup unconditionally restores `document.body.style.overflow` even when `isOpen` is `false` and the effect never modified it. This can incorrectly overwrite overflow settings made by other components. Consider only restoring overflow in cleanup if it was actually modified.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| }); | ||
| const previousOverflow = document.body.style.overflow; | ||
| if (isOpen) { | ||
| document.body.style.overflow = "hidden"; |
There was a problem hiding this comment.
P2: Cleanup unconditionally restores document.body.style.overflow even when isOpen is false and the effect never modified it. This can incorrectly overwrite overflow settings made by other components. Consider only restoring overflow in cleanup if it was actually modified.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/ConfirmModal.tsx, line 39:
<comment>Cleanup unconditionally restores `document.body.style.overflow` even when `isOpen` is `false` and the effect never modified it. This can incorrectly overwrite overflow settings made by other components. Consider only restoring overflow in cleanup if it was actually modified.</comment>
<file context>
@@ -31,11 +31,13 @@ const ConfirmModal = ({
const previousOverflow = document.body.style.overflow;
- document.body.style.overflow = "hidden";
+ if (isOpen) {
+ document.body.style.overflow = "hidden";
+ }
</file context>
✅ Addressed in 6dbc18d
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/components/ConfirmModal.tsx (2)
33-49: Add Escape key handler for accessibility.The modal lacks keyboard support for closing with the Escape key, which is a standard accessibility expectation for dialogs.
🔎 Proposed fix
Add a keyboard event listener inside the useEffect:
useEffect(() => { + const handleKeyDown = (e: KeyboardEvent) => { + if (e.key === "Escape" && isOpen) { + onCancel(); + } + }; + const frame = requestAnimationFrame(() => { if (isOpen) setIsVisible(true); }); const previousOverflow = document.body.style.overflow; if (isOpen) { document.body.style.overflow = "hidden"; + window.addEventListener("keydown", handleKeyDown); } return () => { cancelAnimationFrame(frame); setIsVisible(false); document.body.style.overflow = previousOverflow; + window.removeEventListener("keydown", handleKeyDown); setDragOffset(0); setDragStartY(null); }; }, [isOpen]);
87-90: Inline transform style overrides Tailwind responsive classes.The
style={{ transform: transformValue }}on line 90 overrides the Tailwindmd:translate-y-0class on line 89, which means the slide animation will apply on desktop even though the responsive class intends to disable it. This breaks the intended responsive behavior.🔎 Proposed fix
Apply the drag transform conditionally only on mobile, or use a media query to disable it on desktop:
+ const isMobile = typeof window !== 'undefined' && window.innerWidth < 768; + const transformValue = `${ isVisible ? "translateY(0%)" : "translateY(100%)" - } translateY(${dragOffset}px)`; + } ${isMobile ? `translateY(${dragOffset}px)` : ''}`;Or remove the conflicting Tailwind classes if the inline transform is intended for all breakpoints:
className={`${ isVisible ? "translate-y-0" : "translate-y-full" - } md:translate-y-0 absolute bottom-0 left-0 right-0 md:relative md:bottom-auto w-full md:max-w-lg bg-white text-slate-900 rounded-t-2xl md:rounded-xl shadow-2xl transition-transform duration-300 ease-out transform overflow-hidden pb-[env(safe-area-inset-bottom)] max-h-[90vh] min-h-[70vh] md:min-h-0 md:max-h-none flex flex-col`} + } absolute bottom-0 left-0 right-0 md:relative md:bottom-auto w-full md:max-w-lg bg-white text-slate-900 rounded-t-2xl md:rounded-xl shadow-2xl duration-300 ease-out overflow-hidden pb-[env(safe-area-inset-bottom)] max-h-[90vh] min-h-[70vh] md:min-h-0 md:max-h-none flex flex-col`} style={{ transform: transformValue }}
🧹 Nitpick comments (1)
src/components/ConfirmModal.tsx (1)
108-125: Consider distinguishing confirm and cancel button styles.Both buttons use identical styling (
bg-[#9400FF]), making it difficult for users to quickly identify the primary action. Consider using different colors or visual weights to improve UX.💡 Example approach
{showCancel && ( <button type="button" onClick={onCancel} - className="h-11 md:h-9 md:rounded-md w-full md:w-auto rounded-full bg-[#9400FF] px-4 py-2 text-sm font-semibold text-white transition hover:brightness-110 cursor-pointer" + className="h-11 md:h-9 md:rounded-md w-full md:w-auto rounded-full bg-gray-200 px-4 py-2 text-sm font-semibold text-gray-900 transition hover:bg-gray-300 cursor-pointer" > {cancelLabel} </button> )}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ConfirmModal.tsx(1 hunks)
⏰ 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: cubic · AI code reviewer
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
src/components/ConfirmModal.tsx (4)
1-14: LGTM!The imports and interface definition are clean and well-typed.
16-31: LGTM!Component setup and state initialization are appropriate.
51-72: LGTM!The drag handlers properly guard against undefined values and implement the drag-to-dismiss logic correctly.
74-76: LGTM!Transform calculation correctly combines visibility animation with drag offset.
wolf31o2
left a comment
wolf31o2
left a comment
There was a problem hiding this comment.
When buying for the very first time, the app page says "you're no longer protected" where we probably need an initial state. Once the user has an account, it says connected and secure, but that's not really true. They haven't connected, yet. For the non-app pages, use the app page's background "smoked glass" look vs the current "grey"
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
…hase Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
…irst time and if no instances Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
JaeBrian
force-pushed
the
vpn-frontend/phase-2
branch
from
24b0f7d to
b8fe08f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/VpnInstance.tsx (1)
46-53: Remove or replacebackdrop-blur-xswith a valid Tailwind utility.The class
backdrop-blur-xson line 47 (and used throughout the component) is not a standard Tailwind CSS utility and is not defined in yourtailwind.config.js. Standard options arebackdrop-blur-sm,backdrop-blur,backdrop-blur-md, etc. This utility will have no effect. Replace withbackdrop-blur-sm.src/pages/DocsFaqs.tsx (1)
136-162: Accessibility concern:outline-noneremoves keyboard focus indicators.The FAQ toggle buttons use
outline-nonewhich removes the default focus ring for keyboard users. This makes keyboard navigation difficult as users cannot see which item has focus.🔎 Recommended fix
Replace
outline-nonewith a custom focus style that maintains visibility:<button onClick={() => toggleItem(faq.id)} - className="w-full text-left p-6 hover:bg-[#ffffff08] transition-colors duration-200 outline-none" + className="w-full text-left p-6 hover:bg-[#ffffff08] transition-colors duration-200 focus:outline-none focus:ring-2 focus:ring-white/40 focus:ring-inset" >This preserves the clean look while maintaining keyboard accessibility.
♻️ Duplicate comments (5)
src/pages/HowItWorks.tsx (2)
13-27: Consider adding accessibility attributes to sections.The sections lack
aria-labelledbyassociations to their headings. For better screen reader support, consider addingidattributes to headings and correspondingaria-labelledbyto sections.
250-262: Mark decorative SVG as hidden from assistive tech.The SVG inside the "Back Home" link is decorative since the link already has text content.
🔎 Suggested fix
<svg className="w-5 h-5" fill="none" stroke="currentColor" viewBox="0 0 24 24" + aria-hidden="true" + focusable="false" >src/components/WalletModal.tsx (1)
25-29: State cleanup incomplete:showDisconnectConfirmpersists across modal sessions.When the modal closes (via backdrop click or otherwise),
showDisconnectConfirmis not reset. If a user opens the disconnect confirmation then closes the main modal, the confirmation will unexpectedly appear when the modal reopens.🔎 Proposed fix
return () => { cancelAnimationFrame(frame); setIsVisible(false); + setShowDisconnectConfirm(false); document.body.style.overflow = previousOverflow; };src/pages/Account.tsx (1)
128-131: Duration normalization threshold issue: values between 1000ms and 1 hour are incorrectly converted.The 1-hour threshold causes incorrect conversions. For example, a 30-minute duration stored as 1,800,000ms would be detected as "less than 1 hour" and incorrectly multiplied by 1000 to become 1,800,000,000ms (≈500 hours).
🔎 Recommended fix
Use a threshold of 1000 to distinguish seconds from milliseconds:
const normalizeDurationMs = (duration?: number) => { if (!duration) return 0; - return duration < 1000 * 60 * 60 ? duration * 1000 : duration; + // If duration < 1000, assume it's in seconds; otherwise assume milliseconds + return duration < 1000 ? duration * 1000 : duration; };Alternatively, if the API can return large second values (>1000), use a day threshold (86400 seconds).
src/components/WalletConnection.tsx (1)
285-299: Missing user feedback: empty div renders during wallet connection.The connecting state renders an empty
<div>with no content. Users see no feedback about which wallet is connecting or that a connection is in progress. The original "Connecting to {pendingWallet}..." message was removed.🔎 Proposed fix
Restore the connecting message to provide user feedback:
{isConnecting && pendingWallet && ( - <div className={statusClasses}></div> + <p className={`${statusClasses} animate-pulse`}> + Connecting to {pendingWallet}... + </p> )}
🧹 Nitpick comments (3)
src/components/TooltipGuide.tsx (1)
104-105: Optional: remove redundantenabledcheck.Since
effectiveShowTooltipsis derived asenabled && showTooltips(line 29), explicitly checkingenabled && effectiveShowTooltipsis redundant. You can simplify to justeffectiveShowTooltips.🔎 Proposed simplification
- {enabled && - effectiveShowTooltips && + {effectiveShowTooltips && steps.map((step, index) => {src/components/Navigation.tsx (1)
16-29: Consider removingisMenuOpenfrom useEffect dependency.Including
isMenuOpenin the dependency array causes the resize listener to be re-attached every time the menu opens/closes. This is unnecessary since the handler only reads the current value.🔎 Suggested optimization
useEffect(() => { const handleResize = () => { const isDesktop = window.innerWidth >= 768; - if (isDesktop && isMenuOpen) setIsMenuOpen(false); + if (isDesktop) setIsMenuOpen(false); }; window.addEventListener("resize", handleResize); - handleResize(); return () => { window.removeEventListener("resize", handleResize); }; - }, [isMenuOpen]); + }, []);The condition
isDesktop && isMenuOpenisn't needed sincesetIsMenuOpen(false)is idempotent when already false.src/pages/InstallGuide.tsx (1)
294-312: Consider linking directly to "/" instead of "/account" for consistency.According to the PR objectives, the Account page is now at "/" and "/account" redirects there. While the redirect will work, linking directly to "/" would be clearer and avoid the extra redirect.
🔎 Proposed change
<Link - to="/account" + to="/" className="inline-flex items-center gap-2 px-6 py-3 bg-white text-black font-semibold rounded-lg hover:bg-gray-100 transition-all duration-200" >
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/checks.svgis excluded by!**/*.svgpublic/hamburger.svgis excluded by!**/*.svg
📒 Files selected for processing (27)
src/App.tsxsrc/api/client.tssrc/components/ConfirmModal.tsxsrc/components/CustomToast.tsxsrc/components/HeroSection.tsxsrc/components/LoadingOverlay.tsxsrc/components/Navigation.tsxsrc/components/PurchaseCard.tsxsrc/components/SpinningBorderButton.tsxsrc/components/TooltipGuide.tsxsrc/components/VpnInstance.tsxsrc/components/WalletConnection.tsxsrc/components/WalletModal.tsxsrc/components/WalletPickerModal.tsxsrc/components/__tests__/WalletConnection.test.tsxsrc/index.csssrc/pages/Account.tsxsrc/pages/DocsFaqs.tsxsrc/pages/Home.tsxsrc/pages/HowItWorks.tsxsrc/pages/InstallGuide.tsxsrc/pages/PrivacyPolicy.tsxsrc/routes/index.tsxsrc/stores/walletStore.tssrc/utils/pendingTransactions.tssrc/utils/toast.tsxtailwind.config.js
💤 Files with no reviewable changes (2)
- src/components/CustomToast.tsx
- src/utils/toast.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- src/components/LoadingOverlay.tsx
- src/components/WalletPickerModal.tsx
- src/routes/index.tsx
- src/utils/pendingTransactions.ts
- src/components/ConfirmModal.tsx
- src/components/tests/WalletConnection.test.tsx
- src/components/PurchaseCard.tsx
- src/components/SpinningBorderButton.tsx
- src/stores/walletStore.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/WalletModal.tsx (1)
src/utils/formatAddress.ts (1)
truncateAddress(1-13)
⏰ 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). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (16)
src/components/TooltipGuide.tsx (3)
16-16: LGTM! Clean API design for the enabled flag.The optional
enabledprop with atruedefault and the derivedeffectiveShowTooltipsstate provide clear control over tooltip behavior without breaking existing consumers.Also applies to: 23-23, 29-29
31-40: LGTM! Proper guarding of initial reveal logic.The early return when
!enabledcorrectly prevents the localStorage check and initial tooltip reveal. The dependency array properly includesenabledto handle dynamic changes.
56-77: LGTM! Validation effect correctly addresses the race condition.The use of
requestAnimationFrameproperly defers the anchor validation until after the consumer's DOM updates commit, resolving the race condition flagged in previous reviews. Cleanup and guards are correctly implemented.src/pages/PrivacyPolicy.tsx (1)
5-7: Consistent frosted glass styling applied.The decorative blurred blobs and translucent section styling align well with the broader UI modernization across the app. The use of
bg-white/10,backdrop-blur-xl, andborder-white/20creates a cohesive visual language with other pages like HowItWorks.Also applies to: 15-18
src/pages/HowItWorks.tsx (1)
5-11: Simplified layout looks good.The removal of scroll-based navigation and the shift to a unified card-like layout simplifies the component significantly. The decorative blobs and centered content wrapper are consistent with the PrivacyPolicy page styling.
src/components/VpnInstance.tsx (2)
57-65: Region formatting handles empty string but may produce unexpected output.The condition
region ? ...passes for empty strings (""is falsy), but consider documenting expected region format. The slice operation assumes at least 2 characters exist for proper display.
110-139: Action buttons implementation looks good.The conditional rendering for Active and Expired states with appropriate SpinningBorderButton usage is well-structured. The
shouldSpinRenewprop correctly controls bothspinanduseBorderfor visual feedback during pending operations.src/pages/Home.tsx (1)
13-19: LGTM!Clean component structure with consistent navigation handling passed to both HeroSection and WhatIsNabuSection. The background styling update aligns with the app-wide changes.
src/App.tsx (1)
10-24: Background styling and modal integration look good.The inline styles approach for the complex layered background (gradient + image) is reasonable when Tailwind classes become verbose. The
backgroundAttachment: "fixed"ensures consistent appearance during scrolling.WalletPickerModal placement at the app root level enables centralized wallet selection flow accessible from any component via store state.
src/components/HeroSection.tsx (2)
14-25: Responsive image sizing improvements.Good use of
clamp()for fluid sizing and explicitaspect-ratiofor consistent image proportions. The updatedsizesattribute helps browsers select appropriate image resolutions.
39-46: Simplified action area.Removing auxiliary navigation buttons in favor of a single "Get Started" CTA simplifies the hero section. Navigation to other pages is now handled by the global Navigation component.
src/index.css (3)
17-54: Spinning gradient border implementation looks solid.The mask-composite technique for creating a border-only gradient effect is well-implemented. Including both
-webkit-mask-composite: xorand standardmask-composite: excludeensures cross-browser compatibility for WebKit and standards-compliant browsers.
71-103: Complex gradient button styling.The layered conic gradient approach creates an interesting animated border effect. The use of CSS custom properties (
--c,--p,--r) provides flexibility for theming.
65-69:@propertyis now widely supported across modern browsers, including Firefox.CSS @Property is supported in Firefox 128+ (as of 2024) and other major browsers. The gradient animation will interpolate smoothly across current browser versions.
Likely an incorrect or invalid review comment.
src/components/Navigation.tsx (1)
40-47: Mobile navigation implementation looks good.The hamburger menu with proper ARIA attributes (
aria-label,aria-expanded) and the responsive hiding/showing logic are well-implemented. ThecloseMenucallback on the logo link ensures consistent UX.Also applies to: 84-91
tailwind.config.js (1)
7-9: LGTM! Font family quoting follows Tailwind CSS v4 best practices.Wrapping font names containing spaces in double quotes ensures proper parsing and is the recommended approach in Tailwind CSS v4.
src/api/client.ts
Outdated
| const bodyHex = signedTxCbor.trim(); | ||
| console.info("[API] submitTransaction hex length:", bodyHex.length); | ||
|
|
||
| return fetch(url, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/cbor", | ||
| // Send raw hex as plain text for compatibility with submit endpoint expectations. | ||
| "Content-Type": "text/plain", | ||
| accept: "application/json", | ||
| }, | ||
| body: bodyBytes, | ||
| body: bodyHex, | ||
| }).then(async (response) => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for backend API endpoint definitions and /tx/submit references
echo "=== Searching for /tx/submit endpoint references ==="
rg -n "tx/submit" -i
echo ""
echo "=== Searching for text/plain Content-Type handling ==="
rg -n "text/plain" -i
echo ""
echo "=== Searching for CBOR-related code ==="
rg -n "cbor" -i
echo ""
echo "=== Checking for API documentation files ==="
fd -e md -e txt -e rst | head -20Repository: blinklabs-io/vpn-frontend
Length of output: 2867
🏁 Script executed:
#!/bin/bash
# Search for related changes to submitTransaction or transaction submission
echo "=== Searching for submitTransaction function usage ==="
rg -n "submitTransaction" --type ts --type js
echo ""
echo "=== Searching for Content-Type headers ==="
rg -n "Content-Type" --type ts --type js -B 2 -A 2
echo ""
echo "=== Looking for test files related to transaction submission ==="
fd -e test.ts -e test.js -e spec.ts -e spec.js | xargs rg -l "submitTransaction\|/tx/submit" 2>/dev/null || echo "No test files found"Repository: blinklabs-io/vpn-frontend
Length of output: 4977
🏁 Script executed:
#!/bin/bash
# Check for backend or API documentation in the repo
echo "=== Looking for backend configuration or API docs ==="
find . -maxdepth 2 -type f \( -name "*backend*" -o -name "*api*" -o -name "README*" \) | head -20
echo ""
echo "=== Checking main README for API information ==="
if [ -f "README.md" ]; then
cat README.md | head -100
fiRepository: blinklabs-io/vpn-frontend
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Examine the test file for submitTransaction
echo "=== Examining client.test.ts around line 40 ==="
sed -n '25,60p' src/api/__tests__/client.test.ts
echo ""
echo "=== Looking for submitTransaction-specific tests ==="
rg -n "submitTransaction" src/api/__tests__/client.test.ts -A 10 -B 2Repository: blinklabs-io/vpn-frontend
Length of output: 1204
🏁 Script executed:
#!/bin/bash
# Examine the backend serverless function
echo "=== Examining functions/api/[[path]].ts ==="
head -100 functions/api/[[path]].ts
echo ""
echo "=== Searching for /tx/submit handling in the serverless function ==="
rg -n "tx/submit" functions/api/[[path]].ts -B 5 -A 10Repository: blinklabs-io/vpn-frontend
Length of output: 2407
🏁 Script executed:
#!/bin/bash
# Look for deployment or environment configuration
echo "=== Checking for deployment documentation ==="
find . -maxdepth 2 -type f \( -name "*deploy*" -o -name "*vercel*" -o -name "*.env*" \) 2>/dev/null
echo ""
echo "=== Checking for git history of the change ==="
git log --oneline -20 -- src/api/client.ts 2>/dev/null || echo "Git history not available"Repository: blinklabs-io/vpn-frontend
Length of output: 219
🏁 Script executed:
#!/bin/bash
# Search for all test files and look for transaction submission tests
echo "=== Searching for all test files ==="
fd -e test.ts -e test.js -e spec.ts -e spec.js
echo ""
echo "=== Searching for submitTransaction in all test files ==="
rg -n "submitTransaction" . --include='*.test.*' --include='*.spec.*'
echo ""
echo "=== Checking entire client.test.ts file ==="
wc -l src/api/__tests__/client.test.tsRepository: blinklabs-io/vpn-frontend
Length of output: 738
🏁 Script executed:
#!/bin/bash
# Examine the full test file to understand test coverage
echo "=== Full client.test.ts file ==="
cat src/api/__tests__/client.test.tsRepository: blinklabs-io/vpn-frontend
Length of output: 4444
🏁 Script executed:
#!/bin/bash
# Check for any wallet store tests that might test submitTransaction
echo "=== Searching for wallet store tests ==="
fd -name "*wallet*" --include='*.test.*' --include='*.spec.*'
echo ""
echo "=== Check if there are integration tests ==="
fd -type d -name "__tests__" -o -name "test" -o -name "tests" | head -10Repository: blinklabs-io/vpn-frontend
Length of output: 574
🏁 Script executed:
#!/bin/bash
# Look for environment configuration and API documentation
echo "=== Searching for environment/config files ==="
find . -maxdepth 3 -type f \( -name ".env*" -o -name "vercel.json" -o -name "wrangler.toml" -o -name "tsconfig.json" \) 2>/dev/null | xargs ls -la
echo ""
echo "=== Check for deployment or API documentation ==="
find . -maxdepth 2 -type f -name "*.md" -o -name "*.txt" | xargs grep -l "tx/submit\|preprod-api\|API_URL" 2>/dev/null || echo "No matches found"
echo ""
echo "=== Look for any comments about API changes in git ==="
git log --all --grep="tx/submit\|submitTransaction\|text/plain" --oneline 2>/dev/null || echo "No git history available"Repository: blinklabs-io/vpn-frontend
Length of output: 340
🏁 Script executed:
#!/bin/bash
# Check the actual submitTransaction implementation more carefully
echo "=== Full submitTransaction function ==="
sed -n '206,230p' src/api/client.ts
echo ""
echo "=== Check what the trim() operation does and if there's error handling ==="
rg -n "submitTransaction" src/api/client.ts -A 30Repository: blinklabs-io/vpn-frontend
Length of output: 2003
🏁 Script executed:
#!/bin/bash
# Look for any comments about the API endpoint or related issues
echo "=== Search for comments about compatibility or backend ==="
rg -n "compatibility\|backend\|preprod-api\|submit endpoint" -i
echo ""
echo "=== Check if there are any PRs or commit messages referencing this ==="
git log --all -30 --oneline 2>/dev/null | head -30Repository: blinklabs-io/vpn-frontend
Length of output: 2160
🏁 Script executed:
#!/bin/bash
# Get details about commit 12c29ab
echo "=== Commit details for tx/submit endpoint change ==="
git show 12c29ab --stat 2>/dev/null || echo "Cannot access full commit details"
echo ""
echo "=== Check for any test changes in that commit ==="
git show 12c29ab -- "*.test.ts" "*.spec.ts" 2>/dev/null | head -100 || echo "No test changes in that commit"Repository: blinklabs-io/vpn-frontend
Length of output: 738
🏁 Script executed:
#!/bin/bash
# Verify the exact state of functions/api to understand proxy behavior
echo "=== Check how functions/api handles Content-Type ==="
sed -n '1,80p' functions/api/[[path]].ts | grep -A 5 -B 5 "headers"Repository: blinklabs-io/vpn-frontend
Length of output: 1649
Add test coverage for submitTransaction to verify the endpoint accepts text/plain with hex string payload.
The code change to use Content-Type: text/plain with a raw hex string body was introduced in PR #53 as an intentional modification for the /tx/submit endpoint. However, the submitTransaction function lacks test coverage in src/api/__tests__/client.test.ts. Add a test case to verify:
Content-Type: text/plainheader is sent- Raw hex string body (after trim operation) is passed correctly
- Successful transaction hash response is handled
| useEffect(() => { | ||
| return () => setIsMenuOpen(false); | ||
| }, [location.pathname]); |
There was a problem hiding this comment.
Effect cleanup runs on unmount, not on pathname change.
The cleanup function () => setIsMenuOpen(false) runs when the component unmounts or before the effect re-runs, not specifically when the pathname changes. Since the effect body is empty, consider moving the state update to the effect body instead:
🔎 Suggested fix
useEffect(() => {
- return () => setIsMenuOpen(false);
+ setIsMenuOpen(false);
}, [location.pathname]);📝 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.
| useEffect(() => { | |
| return () => setIsMenuOpen(false); | |
| }, [location.pathname]); | |
| useEffect(() => { | |
| setIsMenuOpen(false); | |
| }, [location.pathname]); |
🤖 Prompt for AI Agents
In src/components/Navigation.tsx around lines 31 to 33, the current effect
returns a cleanup that calls setIsMenuOpen(false) which only runs on unmount or
before the effect re-runs; to close the menu when pathname changes move the
setIsMenuOpen(false) into the effect body (remove the cleanup), so the effect
executes setIsMenuOpen(false) whenever location.pathname changes and keeps the
dependency array as [location.pathname].
| {isMenuOpen && ( | ||
| <div className="md:hidden absolute right-4 top-[4.5rem] w-64 rounded-2xl bg-[#0c0f14e6] backdrop-blur-xl shadow-[0_24px_70px_-32px_rgba(0,0,0,0.8)] border border-white/10"> | ||
| <div className="flex flex-col gap-1 px-3 py-3"> | ||
| <Link | ||
| to="/account" | ||
| onClick={closeMenu} | ||
| className="flex justify-between items-center rounded-xl px-3 py-2 font-ibm-plex text-white font-bold text-sm hover:bg-white/5 transition-colors" | ||
| > | ||
| VPN Access | ||
| </Link> | ||
| <Link | ||
| to="/how-it-works" | ||
| onClick={closeMenu} | ||
| className="flex justify-between items-center rounded-2xl px-3 py-2 font-ibm-plex text-white font-normal text-sm hover:bg-white/5 transition-colors" | ||
| > | ||
| How It Works | ||
| </Link> | ||
| <Link | ||
| to="/install" | ||
| onClick={closeMenu} | ||
| className="flex justify-between items-center rounded-2xl px-3 py-2 font-ibm-plex text-white font-normal text-sm hover:bg-white/5 transition-colors" | ||
| > | ||
| How to Install | ||
| </Link> | ||
| <Link | ||
| to="/docs-faqs" | ||
| onClick={closeMenu} | ||
| className="flex justify-between items-center rounded-2xl px-3 py-2 font-ibm-plex text-white font-normal text-sm hover:bg-white/5 transition-colors" | ||
| > | ||
| FAQs | ||
| </Link> | ||
| <Link | ||
| to="/privacy-policy" | ||
| onClick={closeMenu} | ||
| className="flex justify-between items-center rounded-2xl px-3 py-2 font-ibm-plex text-white font-normal text-sm hover:bg-white/5 transition-colors" | ||
| > | ||
| Privacy Policy | ||
| </Link> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if WalletConnection is accessible elsewhere for mobile users
echo "=== Searching for WalletConnection usage ==="
rg "WalletConnection" --type=tsx --type=ts -l
echo -e "\n=== Check Account page for wallet connection ==="
rg -A 5 "WalletConnection" src/pages/Account.tsx 2>/dev/null || echo "Not found in Account.tsx"
echo -e "\n=== Check full Navigation.tsx for desktop WalletConnection ==="
rg -B 5 -A 5 "WalletConnection" src/components/Navigation.tsx 2>/dev/null || echo "Not found in Navigation.tsx"
echo -e "\n=== Search for WalletConnection in all component files ==="
rg "WalletConnection" src/components/ --type=tsx -B 2 -A 2 2>/dev/null | head -50Repository: blinklabs-io/vpn-frontend
Length of output: 1444
Mobile menu could expose wallet connection more prominently.
The desktop navigation includes <WalletConnection /> in the main navbar (hidden md:block), but the mobile dropdown menu doesn't. While mobile users can access wallet functionality via the /account link ("VPN Access"), wallet connection is less discoverable on mobile compared to desktop.
| <Link | ||
| to="/account" | ||
| onClick={closeMenu} | ||
| className="flex justify-between items-center rounded-xl px-3 py-2 font-ibm-plex text-white font-bold text-sm hover:bg-white/5 transition-colors" | ||
| > | ||
| VPN Access | ||
| </Link> | ||
| <Link | ||
| to="/how-it-works" | ||
| onClick={closeMenu} | ||
| className="flex justify-between items-center rounded-2xl px-3 py-2 font-ibm-plex text-white font-normal text-sm hover:bg-white/5 transition-colors" | ||
| > | ||
| How It Works | ||
| </Link> | ||
| <Link | ||
| to="/install" | ||
| onClick={closeMenu} | ||
| className="flex justify-between items-center rounded-2xl px-3 py-2 font-ibm-plex text-white font-normal text-sm hover:bg-white/5 transition-colors" | ||
| > | ||
| How to Install | ||
| </Link> | ||
| <Link | ||
| to="/docs-faqs" | ||
| onClick={closeMenu} | ||
| className="flex justify-between items-center rounded-2xl px-3 py-2 font-ibm-plex text-white font-normal text-sm hover:bg-white/5 transition-colors" | ||
| > | ||
| FAQs | ||
| </Link> | ||
| <Link | ||
| to="/privacy-policy" | ||
| onClick={closeMenu} | ||
| className="flex justify-between items-center rounded-2xl px-3 py-2 font-ibm-plex text-white font-normal text-sm hover:bg-white/5 transition-colors" | ||
| > | ||
| Privacy Policy | ||
| </Link> |
There was a problem hiding this comment.
Inconsistent border-radius on mobile menu items.
The first link (VPN Access) uses rounded-xl while all other links use rounded-2xl. This appears unintentional.
🔎 Suggested fix
<Link
to="/account"
onClick={closeMenu}
- className="flex justify-between items-center rounded-xl px-3 py-2 font-ibm-plex text-white font-bold text-sm hover:bg-white/5 transition-colors"
+ className="flex justify-between items-center rounded-2xl px-3 py-2 font-ibm-plex text-white font-bold text-sm hover:bg-white/5 transition-colors"
>🤖 Prompt for AI Agents
In src/components/Navigation.tsx around lines 96 to 130 the first mobile menu
Link uses className "rounded-xl" while the other menu items use "rounded-2xl",
causing an inconsistent border-radius; update the first Link's className to use
"rounded-2xl" (or change all to a single chosen radius) so all menu items share
the same border-radius utility class and remain visually consistent.
Signed-off-by: JaeBrian <[email protected]>
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/components/WalletModal.tsx">
<violation number="1" location="src/components/WalletModal.tsx:21">
P2: Setting `overflow = ""` when `!isOpen` will overwrite the correct restoration done by the cleanup function. The cleanup already runs before this code and restores `previousOverflow`. This line then unconditionally resets it to empty string, losing the original overflow value if it wasn't empty.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
src/components/WalletModal.tsx
Outdated
| useEffect(() => { | ||
| if (!isOpen) { | ||
| // Ensure overflow is restored when modal closes | ||
| document.body.style.overflow = ""; |
There was a problem hiding this comment.
P2: Setting overflow = "" when !isOpen will overwrite the correct restoration done by the cleanup function. The cleanup already runs before this code and restores previousOverflow. This line then unconditionally resets it to empty string, losing the original overflow value if it wasn't empty.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/WalletModal.tsx, line 21:
<comment>Setting `overflow = ""` when `!isOpen` will overwrite the correct restoration done by the cleanup function. The cleanup already runs before this code and restores `previousOverflow`. This line then unconditionally resets it to empty string, losing the original overflow value if it wasn't empty.</comment>
<file context>
@@ -16,16 +16,20 @@ const WalletModal = ({ isOpen, onDisconnect }: WalletModalProps) => {
- if (!isOpen) return;
+ if (!isOpen) {
+ // Ensure overflow is restored when modal closes
+ document.body.style.overflow = "";
+ return;
+ }
</file context>
✅ Addressed in a5f7e44
Signed-off-by: JaeBrian <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/components/ConfirmModal.tsx (2)
33-49: Missing Escape key support for accessibility.Users expect to close modal dialogs by pressing Escape. Consider adding a
keydownlistener:Suggested implementation
useEffect(() => { + if (!isOpen) return; + const handleEscape = (e: KeyboardEvent) => { + if (e.key === "Escape") onCancel(); + }; + document.addEventListener("keydown", handleEscape); const frame = requestAnimationFrame(() => { - if (isOpen) setIsVisible(true); + setIsVisible(true); }); const previousOverflow = document.body.style.overflow || ""; - if (isOpen) { - document.body.style.overflow = "hidden"; - } + document.body.style.overflow = "hidden"; return () => { + document.removeEventListener("keydown", handleEscape); cancelAnimationFrame(frame); setIsVisible(false); document.body.style.overflow = previousOverflow || ""; setDragOffset(0); setDragStartY(null); }; - }, [isOpen]); + }, [isOpen, onCancel]);
87-90: Inlinetransformstyle overrides Tailwindtranslate-y-*classes.The
style={{ transform: transformValue }}on line 90 will always override the Tailwind classes (translate-y-0,translate-y-full,md:translate-y-0) on lines 87-89, making the responsive desktop behavior ineffective. Themd:translate-y-0intended to disable slide animation on desktop won't work.Consider conditionally applying the inline transform only on mobile viewports or computing the transform value based on viewport size.
src/pages/HowItWorks.tsx (1)
250-262: Mark decorative SVG as hidden from assistive technology.The "Back Home" link already has visible text, so the arrow SVG is decorative. Add accessibility attributes to prevent it from being announced by screen readers.
Add aria-hidden and focusable attributes
<svg className="w-5 h-5" fill="none" stroke="currentColor" viewBox="0 0 24 24" + aria-hidden="true" + focusable="false" >src/components/WalletModal.tsx (1)
29-33: StateshowDisconnectConfirmnot reset when modal closes.If a user opens the disconnect confirmation, then closes the main modal via backdrop click,
showDisconnectConfirmremainstrue. When the wallet modal reopens, the confirmation modal will unexpectedly appear.Add state reset to cleanup
return () => { cancelAnimationFrame(frame); setIsVisible(false); + setShowDisconnectConfirm(false); document.body.style.overflow = previousOverflow || ""; };src/pages/Account.tsx (1)
128-131: Problematic threshold innormalizeDurationMscauses incorrect conversions.The 1-hour threshold (3,600,000 ms) incorrectly converts any millisecond duration under 1 hour. For example, a 30-minute duration stored as 1,800,000 ms would be multiplied by 1000, becoming ~500 hours.
A safer approach is to use a much smaller threshold (e.g., values under 1000 are likely seconds):
Suggested fix
const normalizeDurationMs = (duration?: number) => { if (!duration) return 0; - return duration < 1000 * 60 * 60 ? duration * 1000 : duration; + // Values < 1000 are likely in seconds; otherwise assume milliseconds + return duration < 1000 ? duration * 1000 : duration; };Alternatively, if API durations can exceed 1000 seconds but are still in seconds, use a day threshold:
return duration < 86400 ? duration * 1000 : duration;
🧹 Nitpick comments (2)
src/components/ConfirmModal.tsx (1)
108-125: Identical button styling reduces visual distinction between actions.Both Confirm and Cancel buttons use the same
bg-[#9400FF]purple styling, making it difficult for users to visually distinguish primary from secondary actions. Consider differentiating the cancel button with a lighter or outline style.Suggested styling differentiation
{showCancel && ( <button type="button" onClick={onCancel} - className="h-11 md:h-9 md:rounded-md w-full md:w-auto rounded-full bg-[#9400FF] px-4 py-2 text-sm font-semibold text-white transition hover:brightness-110 cursor-pointer" + className="h-11 md:h-9 md:rounded-md w-full md:w-auto rounded-full border border-gray-300 bg-white px-4 py-2 text-sm font-semibold text-gray-700 transition hover:bg-gray-50 cursor-pointer" > {cancelLabel} </button> )}src/pages/Account.tsx (1)
471-473: Unnecessary type assertion.The
as numberassertion is redundant sinceMath.maxalready returns a number.- Math.max(expirationTime - now.getTime(), 0) as number; + Math.max(expirationTime - now.getTime(), 0);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/ConfirmModal.tsxsrc/components/WalletModal.tsxsrc/components/WalletPickerModal.tsxsrc/pages/Account.tsxsrc/pages/DocsFaqs.tsxsrc/pages/HowItWorks.tsxsrc/pages/InstallGuide.tsxsrc/pages/PrivacyPolicy.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pages/InstallGuide.tsx
- src/pages/PrivacyPolicy.tsx
- src/components/WalletPickerModal.tsx
- src/pages/DocsFaqs.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/WalletModal.tsx (1)
src/utils/formatAddress.ts (1)
truncateAddress(1-13)
src/pages/Account.tsx (5)
src/api/hooks/useSignup.ts (1)
useSignup(5-13)src/api/hooks/useRenewVpn.ts (1)
useRenewVpn(5-12)src/utils/pendingTransactions.ts (2)
addPendingTransaction(31-59)getPendingTransactions(14-25)src/api/types.ts (1)
ClientInfo(45-50)src/utils/instanceSort.ts (3)
sortVpnInstances(17-63)filterOptions(65-70)SortOption(1-5)
⏰ 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: cubic · AI code reviewer
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
src/components/WalletModal.tsx (1)
43-119: LGTM on the modal structure and ConfirmModal integration.The two-step disconnect confirmation flow is well implemented. The modal correctly uses
role="dialog",aria-modal="true", andaria-labelfor accessibility. The responsive design with slide-up animation on mobile and centered layout on desktop is appropriate.src/pages/HowItWorks.tsx (1)
5-268: LGTM on the page restructure.The new card-based layout with translucent panels and gradient backdrop provides a clean, cohesive design. All external links correctly use
target="_blank"withrel="noopener noreferrer". The infrastructure section (previously missing) is now properly included.src/pages/Account.tsx (3)
507-510: Sort option is hardcoded to "default".The
sortVpnInstancescall always usesfilterOptions[0].value("default"), ignoring any user-selected sort preference. If a sort/filter UI is planned, this should be wired to state.return sortVpnInstances( allInstances, filterOptions[0].value as SortOption, // always "default" );If filtering is intentionally disabled for now, consider adding a comment or removing the unused
filterOptionsimport.
340-364: LGTM on the transaction confirmation flow.The
handleConfirmSubmitcorrectly:
- Clears pendingTx immediately for responsive UI
- Signs and submits the transaction
- Adds to pending transactions storage with numeric
durationMs- Starts polling for the new client
- Handles errors via
errorModal
546-590: LGTM on the ConfirmModal integration for purchase/error flows.The modal-driven approach cleanly replaces the previous toast-based UX. The purchase confirmation shows duration, region, and new expiration (for renewals). Error handling via a dedicated error modal provides clear user feedback.
Signed-off-by: JaeBrian <[email protected]>
Signed-off-by: JaeBrian <[email protected]>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/api/client.ts">
<violation number="1">
P2: Input is no longer trimmed before hex parsing. Whitespace in the input could cause incorrect byte values. Consider adding `.trim()` to the input: `const hexString = signedTxCbor.trim();`</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
Summary by cubic
Phase 2 front-end update focusing on the Account experience and mobile usability. New mobile nav, purchase cards, confirm modals, and a unified wallet picker improve wallet and purchase flows.
New Features
Migration
Written for commit a7d6fae. Summary will update automatically on new commits.
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.