fix(marketing): normalize download page System B primitives#9935
Merged
Conversation
|
Collaborator
Author
|
No description provided. |
|
✅ All tests passed. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
📝 WalkthroughWalkthroughThis PR migrates the marketing download page to a new "System B" design system by introducing semantic design tokens, comprehensive CSS styling, refactoring the page JSX to use token-based classes, updating header styling to match, and adding a visual contract test to enforce compliance. ChangesSystem B Download Page Styling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
apps/web/app/(marketing)/download/page.tsx (1)
251-296: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider semantic markup for the inactive iPhone action.
Line 287 renders the inactive iPhone action as a
<div>. While this prevents interaction, it loses semantic meaning. Consider using a<button disabled>or maintaining the anchor witharia-disabled="true"to preserve the control semantics for assistive technologies.♻️ Proposed semantic alternative
- <div className='system-b-download-platform-inactive'> - {platform.action} - </div> + <button + disabled + className='system-b-download-platform-inactive' + > + {platform.action} + </button>Or preserve the anchor with aria attributes:
- <div className='system-b-download-platform-inactive'> + <a + href={platform.href} + aria-disabled="true" + onClick={(e) => e.preventDefault()} + className='system-b-download-platform-inactive' + > {platform.action} - </div> + </a>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/`(marketing)/download/page.tsx around lines 251 - 296, The inactive platform action currently renders as a non-semantic <div> (see PLATFORM_ROWS mapping and the isMac conditional rendering in page.tsx), which should be a real control for accessibility; replace the <div className='system-b-download-platform-inactive'> with a semantic control—either render a <button disabled> preserving the same classes or keep the <a className='system-b-download-platform-action'> but add aria-disabled="true" and tabindex="-1" (and remove href) so assistive tech sees it as a disabled control; update any analytics attributes accordingly and keep visual styling via the existing class names (system-b-download-platform-inactive / system-b-download-platform-action).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/tests/unit/marketing/download-system-b-style-guard.test.ts`:
- Line 19: The test uses resolve(process.cwd(), sourcePath) which is brittle;
change it to resolve(__dirname, sourcePath) (or otherwise compute the
module-relative path) so readFileSync reads the fixture relative to the test
module. Update the call that constructs the path (the expression passed to
readFileSync in this test) to use __dirname (or an equivalent module-relative
base) instead of process.cwd(); keep the same readFileSync and sourcePath
variables.
- Line 14: The current regex literal in the unit test that guards System B color
utilities is too narrow—replace the existing
/\b(?:emerald|...|white)-(?:[0-9]|\[|\/)/ pattern with a broader pattern that
matches utilities like text-<color>, bg-<color>, and slash-opacity variants
(e.g. the suggested
/\b(?:bg|border|text|ring|shadow|decoration)-(?:emerald|fuchsia|amber|sky|indigo|orange|rose|cyan|violet|red|black|white)\b(?:-\d+|-\[|\/(?:\d+|\[))?/);
also replace the test’s process.cwd() based path resolution with the
fileURLToPath(dirname(import.meta.url)) approach used in neighboring tests to
make path resolution more robust.
---
Outside diff comments:
In `@apps/web/app/`(marketing)/download/page.tsx:
- Around line 251-296: The inactive platform action currently renders as a
non-semantic <div> (see PLATFORM_ROWS mapping and the isMac conditional
rendering in page.tsx), which should be a real control for accessibility;
replace the <div className='system-b-download-platform-inactive'> with a
semantic control—either render a <button disabled> preserving the same classes
or keep the <a className='system-b-download-platform-action'> but add
aria-disabled="true" and tabindex="-1" (and remove href) so assistive tech sees
it as a disabled control; update any analytics attributes accordingly and keep
visual styling via the existing class names (system-b-download-platform-inactive
/ system-b-download-platform-action).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b26ea5c4-fbf2-4dd6-8fa0-3d37418d7894
📒 Files selected for processing (4)
apps/web/app/(marketing)/download/page.tsxapps/web/components/organisms/HeaderNav.cssapps/web/styles/design-system.cssapps/web/tests/unit/marketing/download-system-b-style-guard.test.ts
| /linear-gradient|radial-gradient/, | ||
| /\b(?:bg|border|text|ring|shadow|decoration)-\[/, | ||
| /\b(?:rounded|text|h|w|max-w|min-h|tracking|leading|px|py|pt|pb|z)-\[/, | ||
| /\b(?:emerald|fuchsia|amber|sky|indigo|orange|rose|cyan|violet|red|black|white)-(?:[0-9]|\[|\/)/, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for standalone Tailwind color utilities in download page
rg -n '\b(bg|text|border|ring|shadow|decoration)-(emerald|fuchsia|amber|sky|indigo|orange|rose|cyan|violet|red|black|white)(?!-)' \
apps/web/app/\(marketing\)/download/page.tsxRepository: JovieInc/Jovie
Length of output: 499
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the new test file to see how it resolves/reads the download source
echo "=== download-system-b-style-guard.test.ts (first ~120 lines) ==="
sed -n '1,140p' apps/web/tests/unit/marketing/download-system-b-style-guard.test.ts
# 2) Also inspect the similar existing guard test to compare patterns
echo
echo "=== artist-notifications-hero-system-b-style-guard.test.ts (first ~140 lines) ==="
sed -n '1,160p' apps/web/tests/unit/marketing/artist-notifications-hero-system-b-style-guard.test.ts
# 3) Find any standalone color utilities in the download page.
# Use PCRE2 so we can use lookarounds.
echo
echo "=== standalone Tailwind color utilities on download page (PCRE2) ==="
rg -n --pcre2 '\b(?:bg|text|border|ring|shadow|decoration)-(?:emerald|fuchsia|amber|sky|indigo|orange|rose|cyan|violet|red|black|white)\b(?!-)' \
apps/web/app/(marketing)/download/page.tsx || true
# 4) Also collect all occurrences of direct color utility references for context
echo
echo "=== any direct color utility references on download page ==="
rg -n --pcre2 '\b(?:bg|text|border|ring|shadow|decoration)-(?:emerald|fuchsia|amber|sky|indigo|orange|rose|cyan|violet|red|black|white)-(?:\d+|\[|/)' \
apps/web/app/(marketing)/download/page.tsx || trueRepository: JovieInc/Jovie
Length of output: 2315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
DOWNLOAD_PAGE_PATH='apps/web/app/(marketing)/download/page.tsx'
echo "=== standalone Tailwind color utilities on download page (PCRE2) ==="
rg -n --pcre2 '\b(?:bg|text|border|ring|shadow|decoration)-(?:emerald|fuchsia|amber|sky|indigo|orange|rose|cyan|violet|red|black|white)\b(?!-)' \
"$DOWNLOAD_PAGE_PATH" || true
echo
echo "=== any direct color utility references on download page (with shade or bracket/opacity) ==="
rg -n --pcre2 '\b(?:bg|text|border|ring|shadow|decoration)-(?:emerald|fuchsia|amber|sky|indigo|orange|rose|cyan|violet|red|black|white)-(?:\d+|\[|/)' \
"$DOWNLOAD_PAGE_PATH" || true
echo
echo "=== direct references found without requiring the shade suffix (broad scan) ==="
rg -n --pcre2 '\b(?:bg|text|border|ring|shadow|decoration)-(?:emerald|fuchsia|amber|sky|indigo|orange|rose|cyan|violet|red|black|white)' \
"$DOWNLOAD_PAGE_PATH" || trueRepository: JovieInc/Jovie
Length of output: 302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "process\.cwd\(\)" apps/web/tests/unit/marketing --glob '*.test.{ts,tsx}' || true
rg -n "fileURLToPath|dirname\(" apps/web/tests/unit/marketing --glob '*.test.{ts,tsx}' || true
ls -la apps/web/tests/unit/marketingRepository: JovieInc/Jovie
Length of output: 1698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find other System B style guard tests and inspect any color utility regexes they use.
rg -n "system-b-style-guard|System B source contract|forbiddenVisualPatterns" apps/web/tests/unit --glob '*.test.{ts,tsx}'
echo
echo "=== Find regexes matching 'text-white' style patterns in marketing guard tests ==="
rg -n --pcre2 "text-\w+|bg-\w+|border-\w+|ring-\w+|shadow-\w+|decoration-\w+|-(?:\\[|/)|\\b(?:bg|text|border|ring|shadow|decoration)-\\(?:" apps/web/tests/unit/marketing --glob '*.test.{ts,tsx}' || trueRepository: JovieInc/Jovie
Length of output: 922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node <<'NODE'
const pattern = /\b(?:emerald|fuchsia|amber|sky|indigo|orange|rose|cyan|violet|red|black|white)-(?:[0-9]|\[|\/)/;
const samples = [
'text-white',
'bg-black',
'text-red',
'text-white-500',
'bg-black-50',
'text-red/50',
'bg-black/50',
'text-red-[custom]',
'text-red-/50', // not valid tailwind, but test anyway
];
for (const s of samples) {
console.log(s.padEnd(18), '=>', pattern.test(s));
}
NODERepository: JovieInc/Jovie
Length of output: 306
🏁 Script executed:
#!/bin/bash
set -euo pipefail
DOWNLOAD_PAGE_PATH='apps/web/app/(marketing)/download/page.tsx'
echo "=== grep candidates: white/black/red tokens ==="
rg -n --pcre2 '\b(?:white|black|red)\b' "$DOWNLOAD_PAGE_PATH" || true
echo
echo "=== grep candidates: text-white / bg-black / text-red/50 ==="
rg -n --pcre2 'text-white|bg-black|text-red/50|bg-black/50' "$DOWNLOAD_PAGE_PATH" || true
echo
echo "=== grep tailwind color-looking tokens (color-name substrings) ==="
rg -n --pcre2 '\b(?:emerald|fuchsia|amber|sky|indigo|orange|rose|cyan|violet|red|black|white)\b(?:-\d+)?' "$DOWNLOAD_PAGE_PATH" || trueRepository: JovieInc/Jovie
Length of output: 402
Fix System B color guard regex to catch standalone + slash-opacity utilities
- The line 14 regex
/\b(?:emerald|...|white)-(?:[0-9]|\[|\/)/won’t catchtext-<color>/bg-<color>(no shade suffix) and also won’t catch slash-opacity variants liketext-red/50(it requires-before/). - Since this page currently has no
text-white/bg-black/text-red/50-style occurrences, it won’t fail today, but the guard is overly narrow for future violations—update the pattern to allow slash-opacity directly after the color, e.g.:/\b(?:bg|border|text|ring|shadow|decoration)-(?:emerald|fuchsia|amber|sky|indigo|orange|rose|cyan|violet|red|black|white)\b(?:-\d+|-\[|\/(?:\d+|\[))?/ - The test’s
process.cwd()-based path resolution is less robust than the neighboringfileURLToPath(dirname(...))approach; aligning would reduce fragility.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/tests/unit/marketing/download-system-b-style-guard.test.ts` at line
14, The current regex literal in the unit test that guards System B color
utilities is too narrow—replace the existing
/\b(?:emerald|...|white)-(?:[0-9]|\[|\/)/ pattern with a broader pattern that
matches utilities like text-<color>, bg-<color>, and slash-opacity variants
(e.g. the suggested
/\b(?:bg|border|text|ring|shadow|decoration)-(?:emerald|fuchsia|amber|sky|indigo|orange|rose|cyan|violet|red|black|white)\b(?:-\d+|-\[|\/(?:\d+|\[))?/);
also replace the test’s process.cwd() based path resolution with the
fileURLToPath(dirname(import.meta.url)) approach used in neighboring tests to
make path resolution more robust.
|
|
||
| describe('download page System B source contract', () => { | ||
| it('keeps download page visuals on named System B primitives', () => { | ||
| const source = readFileSync(resolve(process.cwd(), sourcePath), 'utf8'); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use module-relative path resolution for test robustness.
The current approach using process.cwd() depends on the working directory when the test runs. This can cause failures if tests are executed from a different location.
The similar guard test in artist-notifications-hero-system-b-style-guard.test.ts uses a more robust pattern:
♻️ Recommended fix: Use module-relative path resolution
-import { readFileSync } from 'node:fs';
-import { resolve } from 'node:path';
+import { readFileSync } from 'node:fs';
+import { dirname, resolve } from 'node:path';
+import { fileURLToPath } from 'node:url';
import { describe, expect, it } from 'vitest';
const sourcePath = 'app/(marketing)/download/page.tsx';
+const appRoot = resolve(dirname(fileURLToPath(import.meta.url)), '../../..');
const forbiddenVisualPatterns = [
// ... patterns
] as const;
describe('download page System B source contract', () => {
it('keeps download page visuals on named System B primitives', () => {
- const source = readFileSync(resolve(process.cwd(), sourcePath), 'utf8');
+ const source = readFileSync(resolve(appRoot, sourcePath), 'utf8');
for (const pattern of forbiddenVisualPatterns) {
expect(source, `${sourcePath} matched ${pattern}`).not.toMatch(pattern);
}
});
});Based on learnings from similar guard test pattern in artist-notifications-hero-system-b-style-guard.test.ts which uses module-relative path resolution.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/tests/unit/marketing/download-system-b-style-guard.test.ts` at line
19, The test uses resolve(process.cwd(), sourcePath) which is brittle; change it
to resolve(__dirname, sourcePath) (or otherwise compute the module-relative
path) so readFileSync reads the fixture relative to the test module. Update the
call that constructs the path (the expression passed to readFileSync in this
test) to use __dirname (or an equivalent module-relative base) instead of
process.cwd(); keep the same readFileSync and sourcePath variables.
Contributor
CI SummaryA crisp snapshot of this PR run — executed vs. skipped, with timing for every key lane.
Run: 26871049862 |
![jovie-bot[bot]](https://avatars.githubusercontent.com/in/2934433?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Auto-Approved by Agent Pipeline
All quality gates passed:
- CI checks (typecheck, lint, unit tests, a11y)
- Scope judge (diff aligns with ticket intent)
- CodeRabbit review (no blocking feedback)
- Sensitive path check (no auth/billing/migration/CI changes)
This PR was auto-approved because it meets all criteria for low-risk agent changes.
It will now enter the merge queue for final validation.
If this approval is incorrect, dismiss it and add the
needs-humanlabel.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| width: 100%; | ||
| max-width: 760px; | ||
| margin-inline: auto; | ||
| padding: var(--homepage-section-space) var(--homepage-page-gutter); |
There was a problem hiding this comment.
Bug: The CSS variables --homepage-section-space and --homepage-page-gutter are undefined on the download page, causing the FAQ section to render without padding.
Severity: MEDIUM
Suggested Fix
Define --homepage-section-space and --homepage-page-gutter in a globally accessible scope, such as :root in design-system.css. Alternatively, provide fallback values in the .system-b-download-faq class definition using var(--variable, <fallback>) syntax.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: apps/web/styles/design-system.css#L2039
Potential issue: The CSS class `.system-b-download-faq` applies padding using the CSS
custom properties `--homepage-section-space` and `--homepage-page-gutter`. These
variables are defined exclusively within the `.home-viewport` class, which is only
present on the homepage. On other pages, such as the `/download` page where
`.system-b-download-faq` is used, these variables are undefined. This causes the
`padding` to resolve to an invalid value, effectively becoming `0`, which will result in
a visually broken layout for the FAQ section with no spacing.
Did we get this right? 👍 / 👎 to inform future reviews.
This was referenced Jun 3, 2026
This was referenced Jun 5, 2026
This was referenced Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/downloadroute-local visual values onto namedsystem-b-download-*primitives./downloadhas one clear first-viewport primary action./downloadagainst raw colors, gradients, arbitrary visual utilities, and negative tracking.Verification
./scripts/setup.shpnpm biome check --write apps/web/app/(marketing)/download/page.tsx apps/web/components/organisms/HeaderNav.css apps/web/styles/design-system.css apps/web/tests/unit/marketing/download-system-b-style-guard.test.tspnpm --filter @jovie/web exec vitest run tests/unit/marketing/download-system-b-style-guard.test.tspnpm --filter @jovie/web run typecheck -- --pretty falsegit diff --checkScreenshot Proof
.gstack/design-reports/screenshots/download-system-b-desktop-after-20260603.png.gstack/design-reports/screenshots/download-system-b-narrow-after-20260603.png.gstack/design-reports/screenshots/download-system-b-mobile-after-20260603.pngRuntime proof on mobile:
CLS 0, no horizontal overflow, one visibleDownload for Macprimary, and the header CTA computed as transparent secondary chrome with no shadow.Summary by CodeRabbit
New Features
Style
Tests