Improve apex accessibility and add a route-wide a11y gate#573
Open
isuttell wants to merge 1 commit into
Open
Conversation
Issue: AP-387
📝 WalkthroughWalkthroughAdds accessibility skip-to-content infrastructure (skip link in ChangesApex Accessibility, Design Tokens, and Lighthouse Gate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.0)packages/ui/src/styles/globals.cssFile contains syntax errors that prevent linting: Line 165: Tailwind-specific syntax is disabled. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| createReadStream(file).pipe(response); | ||
| } catch (error) { | ||
| response.writeHead(500, { "content-type": "text/plain; charset=utf-8" }); | ||
| response.end(error instanceof Error ? error.message : String(error)); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/apex/src/app/Shell.tsx`:
- Around line 88-93: The className attribute on the skip-to-content anchor
element contains focus:not-sr-only, which is not a valid Tailwind v4 utility
class and will be silently ignored with no effect. Remove the focus:not-sr-only
class from the className string, keeping the other focus variants (focus:fixed,
focus:top-4, focus:left-4, etc.) which properly override the sr-only positioning
when the link receives focus.
In `@apps/apex/src/pages/HomePage.tsx`:
- Line 27: The important modifier syntax in the string on line 27 uses Tailwind
v3 format where the exclamation mark prefix appears before the utility name
(e.g., !text-primary-foreground). In Tailwind v4, the important modifier must be
placed after the utility name instead. Change !text-primary-foreground to
text-primary-foreground! so the important modifier correctly applies the high
specificity to the primary foreground text color in the className string that
includes bg-primary and border utilities.
In `@scripts/lighthouse-apex-a11y.mjs`:
- Line 12: The failure condition around line 136 (which checks if audits.length
> 0) always fails regardless of the minScore threshold set on line 12,
effectively bypassing the AGENT_PASTE_LIGHTHOUSE_APEX_A11Y_MIN_SCORE environment
variable override. Modify the failure condition to respect the minScore
threshold so that lowering the threshold value actually allows the check to pass
when audit scores meet the specified minimum score rather than always failing
when audits exist.
- Around line 80-83: The file stream created by
createReadStream(file).pipe(response) is not handling stream-level errors, which
can cause unhandled errors if the file becomes unreadable after path resolution.
Store the stream returned by createReadStream in a variable and attach an error
event listener to it to handle any stream errors that occur (such as file access
issues). The error handler should respond with an appropriate error status code
and message to the client, similar to the existing try-catch error handling, to
prevent the process from terminating unexpectedly.
🪄 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 Plus
Run ID: ea9d6429-d683-4bef-b330-c341df449219
📒 Files selected for processing (18)
apps/apex/src/app/Shell.tsxapps/apex/src/app/chrome.tsxapps/apex/src/components/TranscriptDemo.tsxapps/apex/src/pages/AboutPage.tsxapps/apex/src/pages/DocsIndexPage.tsxapps/apex/src/pages/DocsPageView.tsxapps/apex/src/pages/HomePage.tsxapps/apex/src/pages/HowItWorksPage.tsxapps/apex/src/pages/LegalPage.tsxapps/apex/src/pages/PricingPage.tsxapps/apex/src/render.test.tsxdocs/development.mdpackage.jsonpackages/brand/src/index.tspackages/ui/src/components/buttonClasses.tspackages/ui/src/styles/globals.cssscripts/README.mdscripts/lighthouse-apex-a11y.mjs
Comment on lines
+88
to
+93
| <a | ||
| className="sr-only focus:not-sr-only focus:fixed focus:top-4 focus:left-4 focus:z-[60] focus:rounded-xs focus:border focus:border-rule-strong focus:bg-surface focus:px-3 focus:py-2 focus:font-mono focus:text-mono-sm focus:text-foreground" | ||
| href="#main-content" | ||
| > | ||
| Skip to content | ||
| </a> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for not-sr-only utility definition in CSS files
rg -n 'not-sr-only' --type=css --type=scss -g '!node_modules/**'
# Also check globals.css and any Tailwind config
fd -e css -e ts -e js globals tailwind.config | xargs rg -n 'not-sr-only'Repository: zaks-io/agent-paste
Length of output: 493
🏁 Script executed:
# Find tailwind config file
fd -type f -name "tailwind.config*"
# Check package.json for Tailwind version
cat package.json | grep -i "tailwind\|postcss"
# Search for sr-only related definitions
rg -n "sr-only" --type=ts --type=js --type=css -A 2
# Look for custom utility definitions
rg -n "extend.*utilities\|addUtilities" --type=ts --type=jsRepository: zaks-io/agent-paste
Length of output: 1155
🏁 Script executed:
# Find tailwind config file
find . -maxdepth 3 -name "tailwind.config.*" -type f 2>/dev/null
# Check package.json for Tailwind version and dependencies
rg "tailwind|postcss" package.json -A 1 -B 1
# Search more broadly for custom utilities or not-sr-only definition
rg "not-sr-only|addUtilities|sr-only.*:" --type=ts --type=js -A 2 -B 2
# Check for any CSS definitions of sr-only or not-sr-only
rg "\.sr-only|`@apply`.*sr-only" --type=cssRepository: zaks-io/agent-paste
Length of output: 616
🏁 Script executed:
# List all config files in the repo
ls -la | grep -E "tailwind|postcss|package"
# Find tailwind.config in any location
find . -type f -name "tailwind.config*" 2>/dev/null | head -20
# Check for package.json files
find . -type f -name "package.json" -path "*/apps/*" | head -5
# Read the main or apex package.json
cat apps/apex/package.json 2>/dev/null | grep -A 5 -B 5 "tailwind\|postcss" || echo "Not in apex package.json, checking root"
# Check root package.json
cat package.json 2>/dev/null | grep -A 5 -B 5 "tailwind\|postcss"
# Look for any CSS that might define sr-only or utilities
find . -type f \( -name "*.css" -o -name "*.scss" \) ! -path "*/node_modules/*" | head -20Repository: zaks-io/agent-paste
Length of output: 895
🏁 Script executed:
# Find tailwind.config file - should be in root or apps directory
find . -maxdepth 3 -type f -name "tailwind.config*" 2>/dev/null
# Check the apex CSS files for any custom utilities
cat apps/apex/src/styles/apex.css
# Check the ui package globals.css
cat packages/ui/src/styles/globals.css
# Check root globals.css if it exists
cat apps/web/src/styles/globals.css | head -50
# Look for any tailwind config in the root
ls -la tailwind* 2>/dev/null || echo "No tailwind config in root"
# Check if there's a postcss config
cat postcss.config.* 2>/dev/null || echo "No postcss config found"Repository: zaks-io/agent-paste
Length of output: 19960
🏁 Script executed:
# Final check: search for any Tailwind plugin or extend configuration
rg "addUtilities|extend.*utilities|plugin" packages/ui/ --type=ts --type=js -A 3 -B 1
# Check if there's a vite config that might extend Tailwind
cat apps/apex/vite.config.* 2>/dev/null || echo "No explicit vite config in apex"
# Check packages/ui for any Tailwind config
find packages/ui -name "vite.config.*" -o -name "tailwind.config.*" 2>/dev/nullRepository: zaks-io/agent-paste
Length of output: 628
Remove focus:not-sr-only — it's not a valid Tailwind utility.
The class focus:not-sr-only is not defined anywhere in the codebase and is not part of Tailwind v4's standard utilities. In Tailwind v4, the not-* variant is for negating state selectors (e.g., not-hover:), not for negating utility classes. Since it doesn't exist, this class will be silently ignored and have no effect.
The subsequent focus variants (focus:fixed, focus:top-4, etc.) will override most sr-only positioning, but they don't fully reverse all constraints like clip, overflow, and dimensions. Remove the unused class or define it as a custom utility if not-sr-only is intentionally needed.
🤖 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/apex/src/app/Shell.tsx` around lines 88 - 93, The className attribute on
the skip-to-content anchor element contains focus:not-sr-only, which is not a
valid Tailwind v4 utility class and will be silently ignored with no effect.
Remove the focus:not-sr-only class from the className string, keeping the other
focus variants (focus:fixed, focus:top-4, focus:left-4, etc.) which properly
override the sr-only positioning when the link receives focus.
| const HERO_CTA = | ||
| "group inline-flex items-center gap-2 font-ui font-semibold text-base " + | ||
| "text-accent-foreground bg-accent border border-accent " + | ||
| "!text-primary-foreground bg-primary border border-primary " + |
There was a problem hiding this comment.
Fix the Tailwind v4 important modifier syntax.
The important modifier !text-primary-foreground uses Tailwind v3 syntax with the ! prefix. In Tailwind v4, the important modifier must appear after the utility name. This line will not apply the important text color as intended.
🔧 Proposed fix
- "!text-primary-foreground bg-primary border border-primary " +
+ "text-primary-foreground! bg-primary border border-primary " +📝 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.
Suggested change
| "!text-primary-foreground bg-primary border border-primary " + | |
| "text-primary-foreground! bg-primary border border-primary " + |
🤖 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/apex/src/pages/HomePage.tsx` at line 27, The important modifier syntax
in the string on line 27 uses Tailwind v3 format where the exclamation mark
prefix appears before the utility name (e.g., !text-primary-foreground). In
Tailwind v4, the important modifier must be placed after the utility name
instead. Change !text-primary-foreground to text-primary-foreground! so the
important modifier correctly applies the high specificity to the primary
foreground text color in the className string that includes bg-primary and
border utilities.
| const root = resolve(import.meta.dirname, ".."); | ||
| const clientDir = resolve(root, "apps/apex/dist/client"); | ||
| const serverEntry = resolve(root, "apps/apex/dist/server/entry-server.js"); | ||
| const minScore = Number(process.env.AGENT_PASTE_LIGHTHOUSE_APEX_A11Y_MIN_SCORE ?? "100"); |
There was a problem hiding this comment.
Threshold override is effectively bypassed by the failure condition.
Line 136 always fails when audits.length > 0, so lowering AGENT_PASTE_LIGHTHOUSE_APEX_A11Y_MIN_SCORE on Line 12 does not actually relax pass criteria as documented.
Proposed fix
const minScore = Number(process.env.AGENT_PASTE_LIGHTHOUSE_APEX_A11Y_MIN_SCORE ?? "100");
+if (!Number.isFinite(minScore) || minScore < 0 || minScore > 100) {
+ throw new Error("AGENT_PASTE_LIGHTHOUSE_APEX_A11Y_MIN_SCORE must be a number between 0 and 100");
+}
@@
- const score = Math.round((lhr?.categories?.accessibility?.score ?? 0) * 100);
+ const score = (lhr?.categories?.accessibility?.score ?? 0) * 100;
+ const roundedScore = Math.round(score);
const audits = lhr ? failedAudits(lhr) : ["missing-lighthouse-result"];
- process.stdout.write(`${path}: ${score}${audits.length ? ` (${audits.join(", ")})` : ""}\n`);
- if (score < minScore || audits.length > 0) {
- failures.push({ path, score, audits });
+ process.stdout.write(`${path}: ${roundedScore}${audits.length ? ` (${audits.join(", ")})` : ""}\n`);
+ if (score < minScore) {
+ failures.push({ path, score: roundedScore, audits });
}Also applies to: 133-137
🤖 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 `@scripts/lighthouse-apex-a11y.mjs` at line 12, The failure condition around
line 136 (which checks if audits.length > 0) always fails regardless of the
minScore threshold set on line 12, effectively bypassing the
AGENT_PASTE_LIGHTHOUSE_APEX_A11Y_MIN_SCORE environment variable override. Modify
the failure condition to respect the minScore threshold so that lowering the
threshold value actually allows the check to pass when audit scores meet the
specified minimum score rather than always failing when audits exist.
Comment on lines
+80
to
+83
| createReadStream(file).pipe(response); | ||
| } catch (error) { | ||
| response.writeHead(500, { "content-type": "text/plain; charset=utf-8" }); | ||
| response.end(error instanceof Error ? error.message : String(error)); |
There was a problem hiding this comment.
Handle file stream errors to prevent unexpected process termination.
Line 80 pipes a file stream without handling stream-level errors. If the file becomes unreadable after path resolution, this can emit an unhandled error and fail the gate run abruptly.
Proposed fix
import { createReadStream } from "node:fs";
import { access, stat } from "node:fs/promises";
import { createServer } from "node:http";
import { extname, join, normalize, resolve, sep } from "node:path";
+import { pipeline } from "node:stream/promises";
@@
- response.writeHead(200, { "content-type": TYPES.get(extname(file)) ?? "application/octet-stream" });
- createReadStream(file).pipe(response);
+ response.writeHead(200, { "content-type": TYPES.get(extname(file)) ?? "application/octet-stream" });
+ await pipeline(createReadStream(file), response);
} catch (error) {
- response.writeHead(500, { "content-type": "text/plain; charset=utf-8" });
- response.end(error instanceof Error ? error.message : String(error));
+ if (!response.headersSent) {
+ response.writeHead(500, { "content-type": "text/plain; charset=utf-8" });
+ response.end("Internal Server Error");
+ } else {
+ response.destroy(error instanceof Error ? error : new Error(String(error)));
+ }
}🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 83-83: Information exposure through a stack trace
This information exposed to the user depends on stack trace information.
🤖 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 `@scripts/lighthouse-apex-a11y.mjs` around lines 80 - 83, The file stream
created by createReadStream(file).pipe(response) is not handling stream-level
errors, which can cause unhandled errors if the file becomes unreadable after
path resolution. Store the stream returned by createReadStream in a variable and
attach an error event listener to it to handle any stream errors that occur
(such as file access issues). The error handler should respond with an
appropriate error status code and message to the client, similar to the existing
try-catch error handling, to prevent the process from terminating unexpectedly.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review (AP-387)
Risk: medium
Decision: approve
Ticket triage
- Intended change: Fix apex Lighthouse accessibility issues (contrast, skip link, landmark targets, transcript link name, command-box labels) and add a local route-wide
pnpm lighthouse:apex-a11ygate. - Scope match: Yes — all listed scope items and acceptance criteria are addressed.
Review summary
Focused a11y improvements with a sensible local gate. I verified pnpm lighthouse:apex-a11y passes 100 on all 15 prerendered routes after build, and focused @agent-paste/apex, @agent-paste/ui, and @agent-paste/brand tests pass. CI Validate and Postgres smoke are green on this PR.
Highlights
- Skip link + stable
#main-contentlandmarks on every apex page - Transcript demo
aria-labelnow includes the visible URL (https://…) - Command-box labels exposed to assistive tech (no longer
aria-hidden) - New
scripts/lighthouse-apex-a11y.mjsmirrors the dashboard gate pattern with path traversal guards
Non-blocking follow-ups
buttonClasses/ new--primarytokens change default primary buttons on the web dashboard too (vermilion → high-contrast neutral). Intentional for contrast, but worth a quick visual check on web CTAs before release.- Apex a11y gate is local-only today (dashboard gate runs in CI). Fine for this ticket; consider CI wiring later.
No blocking correctness, security, or regression issues found for first-pass approval.
Sent by Cursor Automation: First Pass PR Reviewer
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
2 participants
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
Fix the easy apex accessibility issues found during review and add a repeatable Lighthouse gate so the marketing surface stays at 100 across prerendered routes.
Changes
main-contenttargets across apex pages.pnpm lighthouse:apex-a11yand document the route-wide accessibility check.Risk: LOW
Test plan
pnpm format:docs:checkpnpm verifypnpm lighthouse:apex-a11ypnpm test:coveragepnpm smoke:localpnpm test:coverage:strictpnpm verifyIssue: AP-387
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores