Skip to content

Conversation

@charliecreates
Copy link
Contributor

Summary

  • Make marketing primary CTAs platform-aware: macOS users see a download button, other platforms see a waitlist CTA
  • Update header CTA to use request user-agent for initial platform choice
  • Centralize the waitlist URL in @superset/shared/constants

Test plan

  • bun run lint apps/marketing/src apps/marketing/src/lib/platform.ts packages/shared/src/constants.ts
  • bun run typecheck -- --filter=@superset/marketing --filter=@superset/shared
  • bun run test -- --filter=@superset/shared

@coderabbitai
Copy link

coderabbitai bot commented Dec 28, 2025

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


Comment @coderabbitai help to get the list of available commands and usage tips.

@charliecreates
Copy link
Contributor Author

charliecreates bot commented Dec 28, 2025

Expand this to see my work.
  • Confirmed the marketing app is on Next.js ^16.0.10 via apps/marketing/package.json, validating that async server components like CTAButtons and use of next/headers are compatible with the framework version.
  • Verified via a repo-wide scan that apps/marketing/src/lib/platform.ts exposes a single isMacOSUserAgent helper (filtering out iOS) that is now shared between CTAButtons and DownloadButton for consistent platform detection.
  • Reviewed apps/marketing/src/app/components/CTAButtons/CTAButtons.tsx (lines 1–61) to confirm auth(), headers(), and isMacOSUserAgent are composed correctly to derive isMac and drive primaryCtaHref, primaryCtaLabel, and PrimaryCtaIcon with appropriate target/rel only on the non-macOS waitlist path.
  • Checked both signed-in and signed-out branches in CTAButtons to ensure the primary CTA markup remains identical across auth states and only the secondary link differs (Dashboard vs Sign In), keeping duplication low while preserving behavior.
  • Validated that WAITLIST_URL usage is constrained to CTAButtons.tsx and WaitlistModal.tsx, and that the non-Mac primary CTA path consistently uses WAITLIST_URL along with a clock icon to match the waitlist UX.
  • Reviewed apps/marketing/src/app/components/PlatformDropdown/PlatformDropdown.tsx and DownloadButton/DownloadButton.tsx to confirm dropdown-based download flows and the Join waitlist for Windows & Linux entry align behaviorally with the new primary CTA logic without conflicting click paths.
  • Captured line-numbered views for CTAButtons.tsx, DownloadButton.tsx, and platform.ts to reference in inline review comments, ensuring feedback on conditions like isMacOSUserAgent and WAITLIST_URL handling can be precise and actionable in PR #521.

@github-actions
Copy link

🚀 Preview Deployment

🔗 Preview Links

Service Status Link
Neon Database (Neon) View Branch
Fly.io Electric (Fly.io) Failed to deploy
Vercel API (Vercel) Open Preview
Vercel Web (Vercel) Open Preview
Vercel Marketing (Vercel) Open Preview
Vercel Admin (Vercel) Open Preview
Vercel Docs (Vercel) Open Preview

Preview updates automatically with new commits

@charliecreates
Copy link
Contributor Author

Nice change overall: the platform-aware CTA behavior is clear, the waitlist URL is now centralized, and isMacOSUserAgent() being shared between server (CTAButtons) and client (DownloadButton) keeps the logic consistent.

A few specific things I’d tighten up before merge:

  • Security: window.open should use noopener,noreferrer everywhere.
    In apps/marketing/src/app/components/DownloadButton/DownloadButton.tsx, handleJoinWaitlist does this (good), but handleAppleSiliconDownload (line ~40) and handleBuildFromSource (line ~44) don’t. It’s a small change, but it avoids reverse-tabnabbing and keeps things consistent.

  • Potential UX regression: Windows/Linux users lose the “Build from source” path.
    With the new platform === "other" branch (lines ~133–145), users on non-macOS will only see “Join waitlist” and won’t have a way to access the dropdown’s “Build from source on GitHub” option anymore. If that’s intentional per the new product direction, all good; if not, consider keeping the dropdown but making “Join waitlist” the primary/default item.

  • CTA “flash” on initial load for DownloadButton.
    The initial platform state is "unknown" (lines ~25–31), so SSR will always render the dropdown trigger and then swap to “Download” or “Join waitlist” after hydration. If you want to avoid that visual change, consider passing an initialPlatform/userAgent prop from a server component (or reading UA from request headers in a parent and threading it through).

  • Copy consistency.
    CTAButtons uses “Download for macOS” (line ~17) but the mac-only DownloadButton uses “Download for Mac” (line ~126). I’d standardize on one string (“macOS” is probably the more precise marketing wording).

  • Minor typing/readability: add an explicit return type for isMacOSUserAgent.
    In apps/marketing/src/lib/platform.ts, export function isMacOSUserAgent(userAgent: string): boolean is a tiny clarity win and makes the intent explicit.

Optional/nice-to-have: in CTAButtons you currently open the waitlist in a new tab but the macOS download navigates in the same tab. That may be exactly what you want, but if the goal is “don’t navigate away from marketing,” you could also set target="_blank" for the download link (with rel) for parity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants